Avatar Optimization
Review Request #176 - Created Sept. 17, 2011 and submitted
Information | |
---|---|
Adrian Budau | |
infoarena | |
Reviewers | |
hackers | |
freak93 |
This is an optimization for managing avatars on the server. Because there are only 5 sizes used on all the site it's more useful to resize all of the images to those 5(thus increasing the space usage by less than 100%) and when a client asks for a size give it directly rather then resizing all the time. The 5 sizes are tiny (old L16x16), small(old L32x32), normal(old L50x50), forum(old L75xL75, used only on the forum at the moment), big(old 100x100) and full(the original picture). To get the avatar on a size from the server simply use the page avatar/%some_size%/%some_user where %some_user% is a username and %some_size$ is a one the 5 sizes. In case the user doesn't exists or he doesn't have an avatar we use a placeholder image. If the size given is not from the list or is omitted the server return a classic 404 Error(File not found) The script which resizes the image for the current users is called make-avatar-folder found in the scripts folder. It requires the attach folder to have permission for read and write. The setup has also been modified so fresh installes will work without calling the script. I have done a mysql dump. I just need to know how to ship it with the rest of the files.
Issues
- 1
- 23
- 0
- 24
Description | From | Last Updated |
---|---|---|
ob_end_clean() discards everything in the buffer cache. This means that even if resizing is successful, you will not return anything ... | Bogdan-Cristian Tătăroiu |
Description: |
|
---|
Haven't yet looked at the diff, but from the description I don't really understand how this approach optimizes anything, everytime you resize an image, it is stored in trunk/cache/ in the form or {attachment_id}_L{width}x{height}.
I think this is a good start for the diff. More exactly, the script that resizes the images and stores them is good. However, as Bogdan mentioned this doesn't really help as the resized images were already cached. The point of this task was that we didn't want to go through php anymore to serve images. Image links should be like www.infoarena.ro/path/to/image.(png|jpg|etc). There is a .htaccess file in the repo. The point of that file is to tell apache what to do with a certain url. Example (this is from my mind, exact code might be different but you get the point): infoarena.ro/account (.htaccess tells apache to run index.php) infoarena.ro/static/js/kkt.js (.htaccess tells apache this is a static file, and apache should go to trunk/www/static/kkt.js, take the file content and send it back to where the request came from) infoarene.ro/static/images/user/size/avatar.jpg (same thing). Right now when we request the avatar image for a user, our requests fall in the first category because we need to do a db query to get the file path. However if we arrange our images into a folder tree like /static/images/username/image-size/avatar.(jpg|png|plm) then we can make our avatar requests fall into the third category, which would be a lot faster.
Also, this is available for stan too, when you create the patch, you can do like this: svn status (this will give you a list of the modified files) svn diff file1 file2 file3 ... (make the patch only with the files you modified and you know that should be included in the diff) After the diff is accepted svn commit file1 file2 file3 ... It's not a very clean solution, but it works. You can also see how svn ignore works, as far as I remember it wasn't so easy to use.
Can we get this done and shipped before girls camp? we need it :)
Change Summary:
This should be all of it. It took me some time because i modified the setup and db.sql files(The template/userinfo had to be modified) Permissions look ok. All it needs now is to be shipped.
Diff: |
Revision 4 (+246 -19) |
---|
Change Summary:
Sorry about the last file. It contained the db.sql diff. Just tell me how i can ship it with the db.sql file too.
Diff: |
Revision 5 (+232 -11) |
---|
Description: |
|
---|
Looks pretty good, we're getting close. Fix what I said and send it back for another iteration.
-
trunk/scripts/make-avatar-folder (Diff revision 6) -
This should go into the setup script, where we set up the config and .htaccess files.
-
trunk/scripts/make-avatar-folder (Diff revision 6) -
no more than 80 chars! There is no way I can read this unless you break it in multiple lines.
-
trunk/scripts/make-avatar-folder (Diff revision 6) -
use db_fetch_all, it will return an array with the table rows. It's better in this case than db_query and mysql_result.
-
trunk/scripts/make-avatar-folder (Diff revision 6) -
What if the avatar folder exists but the size folders don't
-
trunk/scripts/make-avatar-folder (Diff revision 6) -
tabs
-
trunk/www/controllers/account.php (Diff revision 6) -
} else {
-
trunk/www/controllers/account.php (Diff revision 6) -
80 chars? Also this should be a constant somewhere
-
trunk/www/controllers/avatar.php (Diff revision 6) -
where is this used? Also this should be in common. Controllers are supposed to be the main entry point of a request, gather info and execute_view. Functions that deal with the data we have stored (mysql, hard-disk or any other type of storage system) go into /common.
-
trunk/www/format/format.php (Diff revision 6) -
assert(is_valid_size_type(size_type))
-
trunk/www/macros/macro_userimage.php (Diff revision 6) -
always put { ... } even if it is only one instruction.
Looks good, smallish comments apart from the one about copy pasting code. I'm thinking you should make sure that the make-avatar-folder script is able to be run multiple times, so that we can run it whenever there is a problem with missing avatars or something. This is similar to how we run the check-attachments script every once in a while.
-
trunk/common/avatar.php (Diff revision 8) -
$ret is a bad name, $img_info is better. avatar_resize is a misleading name, it makes me think it wants me to pass the size. Something like avatar_cache_resized seems better imo.
-
trunk/common/avatar.php (Diff revision 8) -
Fix your whitespace please... Replace all tabs with spaces. Search for \t in vim.
-
trunk/common/avatar.php (Diff revision 8) -
Copy pasting code is bad. If you ever need to do the same thing in 2 parts of the codebase, make a separate function. Otherwise, whenever we need to make a change to these resizing code, we'll have to change 2 places at once and in a year we won't even remember that it's in 2 places.
-
trunk/scripts/make-avatar-folder (Diff revision 8) -
Put yourself as a user if that's the only problem. If you have a system for remembering versions in place, use it even for this kind of stuff. Updating the text of a revision without updating it's timestamp is bad for caching purposes.
-
trunk/scripts/make-avatar-folder (Diff revision 8) -
It's good to use higher level abstractions rather than doing raw SQL. Whenever you need something like this, search common/{object}.php and common/db/{object}.php for methods that do it, and if you don't find anything, add it there. In this case, I found a get_user_list() method. It's written like crap, but that's a different problem. :)
-
trunk/scripts/make-avatar-folder (Diff revision 8) -
if ({condition}) { {single instruction} }
-
trunk/scripts/make-avatar-folder (Diff revision 8) -
Do a log_warn() if file does not exist, but is in the DB. This should never happen.
-
trunk/scripts/make-avatar-folder (Diff revision 8) -
log_warn() again
-
trunk/scripts/setup (Diff revision 8) -
Does it fit in 80 characters?
Description: |
|
---|
Slightly pedantic comments, but I hope you'll agree with what I pointed out. To answer your questions: - It's better if you update the sample db.sql as a separate commit. Makes it easier to browse through revision diffs. - scripts/setup should not be run on live, it's only made so that you can have a quick and easy sandbox working. There are more tweaks to config.php made on live, so I'll just update it manually once committed. Don't add the configuration updating back into scripts/make-avatar-folder.
-
trunk/common/attachment.php (Diff revision 10) -
This code block makes assumptions about how you are calling the function. die_http_error is probably the worst way of reporting an error we could use. :) The function should return something like true / false on success / error or use Exceptions and then the controller can choose to die_http_error or take any other action. Secondly, if you don't supply any filepath it will output the image directly to the client. We use output buffering in the resize controller to set the proper headers before we send the response body. In this function you have ob_end_clean(), but you do not have an ob_start() at the beginning. You have to either move both of them in the function or remove both of them (i would say this is preferred). Please write a comment at the beginning of the function that explains what it does and what each parameter means as well. It's a practice we have not sticked to in the past, but we'll start now. :) Examples in http://reviewboard.infoarena.ro/r/177/diff/ in scripts/recompute-time-limits on line 22 and 46.
-
trunk/common/avatar.php (Diff revision 10) -
Add comment as I said for the image_resize function
-
trunk/common/avatar.php (Diff revision 10) -
Why not foreach ($resize_sizes as $resize_size)?
-
trunk/scripts/make-avatar-folder (Diff revision 10) -
I would like it if you would prompt the user asking him if this is required, because it's very likely that we will need to call this function in the future to reconstruct an avatar folder, but the template in the database would probably be updated already.
-
trunk/scripts/make-avatar-folder (Diff revision 10) -
What's this for?
-
trunk/www/controllers/account.php (Diff revision 10) -
Delete empty line
Description: |
|
||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+269 -53) |
http://sometimesigetangry.files.wordpress.com/2011/06/fuck_yea_in_hd_by_crusierpl_re_sharenator_is_growing-s900x773-1208111.png Please fix the ob_end_clean() issue and ship it! Good job :D
-
trunk/common/attachment.php (Diff revisions 10 - 11) -
Whenever it's slightly unclear what a parameter means, it's good to add a description as well.
-
trunk/scripts/make-avatar-folder (Diff revisions 10 - 11) -
Default false, it's only going to be true on the first run
-
trunk/www/controllers/image_attachment.php (Diff revisions 10 - 11) -
ob_end_clean() discards everything in the buffer cache. This means that even if resizing is successful, you will not return anything because you cleared the buffer. Do ob_end_clean() only on failure