Optimize css() some #233
Merged
Optimize css() some #233
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After the 1.2.0 release, I noticed that most of my optimizations were completely offset by the ordering bugfixes that were added. After some profiling, I noticed that generateCSSRuleset was ripe for optimization. My approach here is to reduce the amount of work that needs to be done to a bare minimum. I accomplish this by being smarter about when to loop and how many loops to run. In my benchmark, this PR reduces the runtime of css() from 3200s to 2150 ms, which is 1.49x faster.
This is a micro-optimization. We can eek out a tiny bit more performance by moving the function declaration up to the module level. I decided to put it next to the related functions in the util module.
This is a tiny optimization that helps us avoid this check from happening inside the loop.
I noticed that we loop over every declaration in this method, seeing if there is a stringHandler available for it. However, it is more likely that there will be fewer stringHandlers than declarations, so we can speed this up by inverting the loop and check here. While I was at it, I decided to prevent this method from creating a new OrderedElements object every time and instead to just mutate it. I believe this will be safe and should buy us more performance. This means we no longer need the .map method on this class. This change reduces the time spent in runStringHandlers from 8.6% of css() runtime to 0.26% and brings the runtime of my benchmark down from 2150ms to 1950ms.
Merged
|
@xymostech this PR should be pretty straightforward and unblocks some of the improvements I wanted to do on #216. Any chance you are able to take a look soon? |
|
Yeah that's right, but I'm a little nervous about my other PR because of
how it affects plugins and how some of the logic feels a bit spread out. It
might be good to take a step back and see if we can do it more cleanly.
I'm out of town until Tuesday so I probably won't be very responsive until
then.
…On Fri, Apr 14, 2017, 4:37 PM Emily Eisenberg ***@***.***> wrote:
***@***.**** approved this pull request.
Looks good to me! Sorry this took so long to get to, work's been busy.
Merge away, and I'll take a look at #216
<#216> again.
Just to make sure, this doesn't fix #231
<#231>, correct? It's fixed in
the extra commits of #216 <#216>?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#233 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAL7zkR4w5txezydJkyBJxDHtjCqkGO_ks5rwANKgaJpZM4MnFLN>
.
|
|
To close the loop a bit, v1.2.1 just went to production for Airbnb, and it reduced our server rendering times by ~15% (v1.2.0: 285ms, v1.2.1: 245ms). Obviously, this will also help on the client side as well, but the numbers are a little more challenging to consistently measure there. |
|
Thank you so much for pushing this forwards, @lencioni! Glad to hear that it improved performance! :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I am pulling these commits out of #216 because I think they are pretty straightforward and should be easier to merge in this way.