Since updating to version 7.x-1.3 we noted our website's response times doubled, and at times the server dragged to a halt. After examining code traces from the slowdowns, we were able to track the slowdown to theme_flickr_photo(). I have tracked it further to the getimagesize() request section, found in flickr.module at Line 320. 

  // If it is not a square.
  if (!isset($width)) {
    // Get the real width of the image.
    list($width) = getimagesize($img_url);
  }

In our case, we are using a single view block containing eight (8) photos. In every case the photo's size is 'm', and the width is 240 pixels. Since the old switch statement worked fine to determine that, it's difficult to understand why we need to make a size request to Flickr.com.

Yet, if I eliminate the size request so that $width is entirely missing, I get three "Undefined variable" notices, but the images renders perfectly and in the correct size.

Is there any way we can eliminate this little getimagesize() call once again? Based on our experience over the last two months, it makes a huge difference in the performance and stability of our website.

CommentFileSizeAuthor
#30 faster_api_response-2227669-30.patch20.56 KBlolandese
#29 faster_api_response-2227669-29.patch19.54 KBlolandese
#28 faster_api_response-2227669-28.patch18.15 KBlolandese
#23 block_cache_api_implementation-2227669-23.patch36.48 KBlolandese
#21 block_cache_api_implementation-2227669-20.patch8.93 KBlolandese
#16 turn_block_cache_on-2227669-16.patch472 byteslolandese
#3 gets_image_width_faster-2227669-3.patch2.04 KBlolandese

Comments

tomogden’s picture

tomogden commented
Issue summary: View changes
lolandese’s picture

lolandese commented

See https://drupal.org/comment/8376255#comment-8376255 for possible improvement and the rest of that issue on the reasons behind the implementation.

Can you create a patch based on that info?

Thanks.

lolandese’s picture

lolandese commented
Status: Active » Needs review
StatusFileSize
new gets_image_width_faster-2227669-3.patch2.04 KB

Let me know if this improves the response times sufficiently.

Thanks.

lolandese’s picture

lolandese commented
Version: 7.x-1.4 » 7.x-1.x-dev
lolandese’s picture

lolandese commented

Moving the function to flickr.inc makes sense as it is a helper function.

tomogden’s picture

tomogden commented

I tried #3 in three different sites, but there was no time improvement over what I was getting for version 1.4.

In the meantime, I'll keep working on a possible patch solution. I'll switch to working with 1.x-dev.

lolandese’s picture

lolandese commented

Patches are always welcome. Usually I test them instantly and always commit with attribution.

Thanks.

tomogden’s picture

tomogden commented

Yes, @lolandese, I've noticed your diligent response to everything that gets posted. You are to be commended. :)

Getting familiar with the environment, I've noted that the slowdowns are not actually due to queries at Flickr, since everything is cached. But parsing the very large caches does seem to take a lot of time.

I've also found that Flickr images from tokens in the text (using Flickr Filter) seem to have no calls to flickr_request() at all (unless the cache expires or is cleared), and there is no latency – nice and snappy. I'm using all vertical test images, and width issues come up with "size=-", but all other sizes come out with the correct caption width.

lolandese’s picture

lolandese commented
Status: Needs review » Needs work
Related issues: +#2229253: Recent Photo Block - Slow to Access, +#2163647: Unnecessary white space next to image

I've noted that the slowdowns are not actually due to queries at Flickr, since everything is cached. But parsing the very large caches does seem to take a lot of time.

That indeed explains why changing the method doesn't have any impact. Good info.

Do you notice a difference between blocks with 'random' images versus for example 'recent'? #2229253: Recent Photo Block - Slow to Access suggests we have the problem affecting also square images in blocks. I don't mark it as a duplicate of this issue yet. Do you experience that as well?

I've also found that Flickr images from tokens in the text (using Flickr Filter) seem to have no calls to flickr_request() at all (unless the cache expires or is cleared), and there is no latency – nice and snappy.

We might consider excluding the width request for images in a block, just like you suggested. That means that each caption would have the same width, independent from the image width. Some (not me) might even prefer a regular grid over the current kind of masonry behaviour. For the text filter we could leave the current behaviour to avoid the problem described in [#8376255].

tomogden’s picture

tomogden commented

Do you notice a difference between blocks with 'random' images versus for example 'recent'?

Surprisingly, no. Swapping the Recents block with the Random block does not affect load times. That would seem to support the supposition that this is a parsing issue and not a Flickr loading issue.

#2229253: Recent Photo Block - Slow to Access suggests we have the problem affecting also square images in blocks. I don't mark it as a duplicate of this issue yet. Do you experience that as well?

I increased my sample block to 20 photos and ran more tests, I found:

  • The number of photos matters, increasing load times in a predictable linear progression.
  • Size matters, although not quite as much. Square photos of 150px take 20% longer than 75px.
  • Shape doesn't seem to matter. Photos of similar sizes take the same amount of time, regardless of whether they are square.

That means that each caption would have the same width, independent from the image width.

I would hope that would not be necessary. The code in Flickr Filter is so much simpler than everywhere else, and yet the caption width always correctly matches the photo. I wonder if we couldn't standardize a little more and provide the correct width in the other more complex rendering procedures?

lolandese’s picture

lolandese commented

Conclusions so far:

Shape doesn't seem to matter. Photos of similar sizes take the same amount of time, regardless of whether they are square.

That means getimagesize() is not the culprit as square images don't make use of it. It also explains why the #3 patch (alternative to getimagesize) does not give improvement. Change the issue title to something more appropriate if you agree.

Swapping the Recents block with the Random block does not affect load times.

Oops! If block cache is turned on that means it doesn't depend on the dynamic (first time) rendering of a block. In other words, it doesn't dependent on the module code.
To test that create a block with the plain HTML from a module generated block and compare the load times. If the results are identical, it only depends on the remote image URL that loads slow. It still doesn't explain why this occurs particularly with blocks, so next step would be to paste it into a node body and run the same test again.

I wonder if we couldn't standardize a little more and provide the correct width in the other more complex rendering procedures?

Both Flickr Filter and Flickr Block use the same code from the main module to render sets (the functions theme_flickr_photo and theme_flickr_photoset).

FYI:
I am working on an extension module that turns all image info from a user, group or set into nodes, except for the image file itself (only the URL). That means that blocks can then be created with Views:

  • giving more flexibility to the site builder to customize and extend
  • so that arguments can be used easier to make the blocks context sensitive to fulfill some feature request (see #2024133: Display photos based on Flickr tags)
  • making it more reliable and stable as data queries are done by something made for it
  • making it better maintainable as data queries are handled outside of the module itself.

So chances are that eventually Flickr Block as a submodule will become obsolete.

tomogden’s picture

tomogden commented
Title: Width request bloats response times. » flickr_request() bloats response times.

That means getimagesize() is not the culprit as square images don't make use of it. It also explains why the #3 patch (alternative to getimagesize) does not give improvement.

Yes, I wondered about that too. In my test block getimagesize() does NOT result in a flickr_request(), while in my production website's block it does. The two are very different implementations, I suppose. To the point, it seems we are looking to reduce calls to flickr_request().

In other words, it doesn't dependent on the module code.
To test that create a block with the plain HTML from a module generated block and compare the load times.

I'll need to get back to you on that one. I can tell you that I commented out the caching code temporarily and found that load times more than doubled, so at least we know that caching is still valuable where it's at.

Both Flickr Filter and Flickr Block use the same code from the main module to render sets (the functions theme_flickr_photo and theme_flickr_photoset).

I've been comparing the code between the different modules, and I'm finding what look like quite a few differences, many of which are necessary, I'm sure, but there might be some places to standardize. Specifically, I am looking at:

  • theme_flickr_photo($variables) in flickr.module for Flickr Block
  • theme_flickrfield_field_formatter($element) in flickrfield.module for Flickr Field
  • flickr_filter_callback_photo($matches) in flickr_filter.module for for Flickr Filter

Anyway, it's an interesting exercise for myself in trying to get to know the way things work.

I am working on an extension module that turns all image info from a user, group or set into nodes, except for the image file itself (only the URL).

That sounds brilliant and sounds like the logical next step. I think others have tried this before, have they not?
My one concern with this approach is that it mingles the Flickr entities with all of the other nodes. My personal inclination is to keep it as a separate sort of entity. But that's pure philosophy at this point.

tomogden’s picture

tomogden commented

In other words, it doesn't dependent on the module code.
To test that create a block with the plain HTML from a module generated block and compare the load times.

I copied the HTML from the same 20-photo random photo block and inserted it into a new test block. Predictably, the resulting page refreshed in less than a second. To be certain, I double-checked the <img> tags for their src values. They all specified the Flickr farm, such as:

<img class=" flickr-photo-img" height="67" width="100" typeof="foaf:Image" src="https://farm3.staticflickr.com/2323/5809676123_190980d436_t.jpg" alt="U.S. Secretary of State Hillary Rodham Clinton">

lolandese’s picture

lolandese commented

Predictably, the resulting page refreshed in less than a second.

Then somehow the blocks are not sufficiently cached. At least this narrows down the probable cause of our issue. Can you try comparing the load times between being logged in and as an anonymous visitor?

Thanks for your help with this.

tomogden’s picture

tomogden commented

I ran the tests again, and found load times are exactly the same.

lolandese’s picture

lolandese commented
StatusFileSize
new turn_block_cache_on-2227669-16.patch472 bytes

Does this help?

Try on both random and not random blocks, clear cache after applying and reload the same page to be sure a block gets cached.

tomogden’s picture

tomogden commented

Now that makes a difference. The random block times out the same, averaging about 20 seconds, but the recents block averages only 3.5 seconds for all 20 photos.

I also see that the calls to flickr_request() were reduced in that block photos only had a single "search" call for the whole block, instead of a call for each photos.

tomogden’s picture

tomogden commented
Title: flickr_request() bloats response times. » Calls to flickr_request() bloat response times.

We are still seeing some slowdowns, though. To compare with 7.x-1.2, I cleared the caches and ran the same tests.
Random block of 20 => 5.9 seconds (20 seconds under 7.x-dev)
Recent block of 20 => about .5 seconds (3.5 under 7.x-dev)

I have found that although the number of calls to flickr_request() have remained the same for Flickr Filter and Flickr Field, Flickr Block photos have one more request than previously.

lolandese’s picture

lolandese commented

Working on a patch to extend a caching mechanism to random blocks and possibly also the others. Comparing load times with previous versions is useful, but after applying the upcoming patch as up to now it seems that effective Flickr block caching was practically absent.

Meanwhile I want to answer some points of #12 that I somehow missed.

.. there might be some places to standardize.

Working on that in #2227019: Reduce code, break up big functions and move markup to theme functions/templates. I will look into the functions you mentioned.

.. I think others have tried this before, have they not?

The Flickr Rippr modules is a bit similar but does not offer integration with the Flickr module. Besides it didn't have any active development for more than three years. See https://drupal.org/node/130843/commits.

My personal inclination is to keep it as a separate sort of entity.

I looked into the "nodes vs entity" matter. Most found "rule of thumb" is that if it is content, using 'node' is appropriate. In any case to build the module based on nodes is a lot easier as a lot of Drupal mechanisms are already in place for them. For entities a lot of additional things need to be set up. Think about e.g. taxonomy that works out-of-the-box for nodes. If in a later stage it results that 'entities' give us advantages over 'nodes', we can always release a 2 version, although it might be hard to supply an upgrade path.

In any case, it will be a separate module as we even consider doing the same with the sub-modules that are now present inside the Flickr module. One might use 'Flickr Filter' but never have intentions to use 'Flickr Field'. It doesn't make sense to force them to download a whole package. Furthermore, splitting it up gives us a better insight on the usage. In any case, if this will happen it will be a 7.x-2.x version. On this point I will open a new issue soon.

Thanks for thinking with me on a broader scale. Your interest is appreciated.

tomogden’s picture

tomogden commented

I look forward to generating nodes for Flickr photos with the new extension module! It sounds like a real winner of a module.

After that last patch, I did a bunch more testing and found the remaining time difference is made up when I switch to square thumbnails (as opposed to commenting out the list($width) = getimagesize($img_url); in theme_flickr_photo()). That seems to be the last remaining time sync.

For our own production website, we will consider switching to square thumbnails.

lolandese’s picture

lolandese commented
Status: Needs work » Needs review
StatusFileSize
new block_cache_api_implementation-2227669-20.patch8.93 KB

Attached patch should improve load times for the random blocks. They will get refreshed once a day. We can make that configurable, but I think for most users that will be okay.

Once confirmed a subsequent patch will follow for the other blocks. Please register the current load time for one of those, so we can see how it improves after applying the upcoming patch.

Also we have to exclude cache for the 'User profile random photos' to avoid displaying the wrong (other user's) images.

Background info at http://www.lullabot.com/blog/article/beginners-guide-caching-data-drupal-7

tomogden’s picture

tomogden commented
Status: Needs review » Reviewed & tested by the community

Got it. I got a 5% improvement on random photos, but we have a very large collection. Samples had very similar times, but it did seem to improve.

It seems we've done all we can on this issue.

lolandese’s picture

lolandese commented
Title: Calls to flickr_request() bloat response times. » Use the cache API for Flickr blocks (loads them quicker)
Component: flickr (main module) » flickr_block
Category: Bug report » Feature request
StatusFileSize
new block_cache_api_implementation-2227669-23.patch36.48 KB

Applying the attached patch:

  • extends caching to all the Flickr blocks
  • once cached, a page with Flickr blocks loads in a fraction of what it used to
  • makes the refresh rate configurable for random and other blocks from 1 to 999 hours
  • uses dynamic block titles.

In any case it is recommended to use cache warming.

  • Commit a4f13f4 on 7.x-1.x by lolandese: 
    Issue #2227669 by tomogden: Use the cache API for Flickr blocks (loads...
lolandese’s picture

lolandese commented
Status: Reviewed & tested by the community » Fixed

http://drupalcode.org/project/flickr.git/commitdiff/a4f13f4

Thanks Tom, for reporting and testing.

lolandese’s picture

lolandese commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Fixed » Patch (to be ported)
mholve’s picture

mholve commented

.

lolandese’s picture

lolandese commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Needs review
StatusFileSize
new faster_api_response-2227669-28.patch18.15 KB

Good point. :)

As most sites don't use a form of cache warming, it is important to also reduce load times after the cache has been cleared.

Attached patch reduces the number of photos returned on an API request (instead of 500), depending on the minimum number required to produce usable results for an album. The default limit for random and popularity sort can be set on the Flickr configuration page at admin/config/media/flickr.

lolandese’s picture

lolandese commented
StatusFileSize
new faster_api_response-2227669-29.patch19.54 KB

Correction.

lolandese’s picture

lolandese commented
Status: Needs review » Fixed
StatusFileSize
new faster_api_response-2227669-30.patch20.56 KB

Correct counter numbers (totals).

lolandese’s picture

lolandese commented

http://cgit.drupalcode.org/flickr/commit/?id=0c7c374

  • Commit 0c7c374 on 7.x-1.x by lolandese: 
    Issue #2227669 by tomogden: Reduce load times of albums after the cache...
lolandese’s picture

lolandese commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Fixed » Patch (to be ported)
lolandese’s picture

lolandese commented
Status: Patch (to be ported) » Closed (won't fix)

See #2367755: D6 maintenance.

lolandese’s picture

lolandese commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Closed (won't fix) » Closed (fixed)

Setting this back to D7 and being 'fixed (closed)'.

Main reason is the fact that links to this issue show the status on hover without revealing the version (6 or 7). This might give the impression that the mentioned issue will not be solved, while in fact it is already fixed for the D7 version. 'Fixed (closed)' reflects the current status correctly for D7. 'Closed (won't fix)' is only valid for the minimally maintained D6 version of the module.