search

ResponsiveImagesCG / wp-tevko-responsive-images

Fully responsive image plugin for wordpress
449 stars 53 forks source link

Optimization: Reduce calls to WP image functions #207

Closed jaspermdegroot closed 9 years ago

jaspermdegroot commented 9 years ago

I did an A/B performance test again, using the same test as I used for https://github.com/ResponsiveImagesCG/wp-tevko-responsive-images/pull/144#issuecomment-136429727.

Server:

Tool:

WordPress:

A: RICG responsive images plugin deactivated; no srcset and sizes attributes B: RICG responsive images plugin with the code from this PR (branch "optimizations-jasper"); uses content filtering to insert the srcset and sizes attributes in the markup.

All images in this test have 3 sources available for the srcset (medium, large, and full). Images are placed between text paragraphs (except for test post "Heavy" that almost only contains images).

The results, based on 3 runs:

Test post "Light" with 1271 words and 5 images:

Difference: 7 ms Average: 1.4 ms / image

Test post "Heavy" with 18 words and 25 images:

Difference: 31 ms Average: 1.24 ms / image

Test post "Very heavy" with 5020 words and 50 images:

Difference: 60 ms Average: 1.2 ms / image

TODO: test with PHP 5.2

joemcgill commented 9 years ago

I've merged #205 and #206. Feel free to rebase.

jaspermdegroot commented 9 years ago

@joemcgill

I rebased the branch. Can you review the first two commits? If you're ok with those changes we could cherry-pick those two into dev already.

joemcgill commented 9 years ago

I left inline comments on the first commit. The second is ready to merge and is already included in the code that was merged into WP.

I'd like to iterate on 52b12cf a bit because—while it definitely does what we're looking to do—I'm not crazy about the function signature this creates. I was thinking about creating an attachment object that extends WP_Post and includes all the metadata and we can pass that into the first parameter instead of an $attachment_id. It would be a bit more standard and easier to cache across all our functions.

jaspermdegroot commented 9 years ago

@joemcgill

I updated the first commit with inline docs corrections per your comments and cherry picked it together with the second commit (check if wp_get_attachment_metadata returns an array) into dev. See commits https://github.com/ResponsiveImagesCG/wp-tevko-responsive-images/commit/2008dd9196b375c4bb55d66829065124477ba76c and https://github.com/ResponsiveImagesCG/wp-tevko-responsive-images/commit/eb8b5c8bf8d973f388ebfc30ffad601d591e5b37.

Regarding the one remaining commit. I was "coding out loud" to see what we can do to reduce calls to WP image functions but I agree with you that the function signature isn't pretty. Your solution sounds neat. I'll close this PR but will keep the branch alive for now.