Optimize css() #216

lencioni · 2017-03-10T20:21:27Z

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 2750 ms to 900 ms, which is 3.05x faster.

Some of this performance improvement will only be seen when running on the client,
but on the server this should still result in ~2x faster improvement.

This is probably best reviewed commit-by-commit.

Before:

After:

@xymostech

lencioni · 2017-03-10T21:19:31Z

src/generate.js

+                    // the second character to avoid colliding with Moz-prefixed
+                    // styles. Let's find its original style's sort order.
+                    originalStyle = elementNames[i][2].toLowerCase() + elementNames[i].slice(3);
+                }


Looks like coverage is failing branch coverage for the else case here. Any thoughts on how to fix this?

I decided to change it from an else if to an else.

lencioni · 2017-03-10T22:41:36Z

src/ordered-elements.js

@@ -1,5 +1,4 @@
 /* @flow */
-/* global Map */

 export default class OrderedElements {


The branch coverage on this file falls to 75%, but I can't see any missed branches in the HTML report. I'm guessing it might not be getting the coverage right with the Babel transform happening here. Thoughts?

Figured it out. The constructor was never called with arguments, so I removed the args.

coveralls · 2017-03-11T16:52:28Z

Coverage remained the same at 100.0% when pulling 13a7422 on optimize-generateCSSRuleset into c42ae31 on master.

coveralls · 2017-03-11T17:58:10Z

Coverage remained the same at 100.0% when pulling 2325e6d on optimize-generateCSSRuleset into c42ae31 on master.

coveralls · 2017-03-13T19:02:39Z

Coverage decreased (-0.3%) to 99.675% when pulling 1ab7400 on optimize-generateCSSRuleset into c42ae31 on master.

coveralls · 2017-03-13T19:21:47Z

Coverage remained the same at 100.0% when pulling 54dba86 on optimize-generateCSSRuleset into c42ae31 on master.

iancmyers · 2017-03-16T00:07:01Z

Super excited for this!

lencioni · 2017-03-17T15:53:11Z

@xymostech Are there any changes you'd like me to make to this PR before merging?

xymostech · 2017-03-17T17:10:56Z

@lencioni Sorry, I'd like to review this before merging but I've just been terribly busy. I'll review this today or over the weekend.

xymostech · 2017-03-20T16:58:25Z

Hi! My computer broke over the weekend, so I didn't get to reviewing this... It's fixed now, I'll review this today.

lencioni · 2017-03-20T18:55:28Z

@xymostech no worries! I'm sorry to hear about your computer troubles. :( Glad to hear that you are back in action.

xymostech

Wow! This is an insane amount of optimization, thank you so much for doing all of this!

I'm requesting changes because I think there's a few major impacts that this is going to have, and one critical bug that I noticed (the deep merge thing). I have a bunch of general questions and some nits, but overall I'm super happy with the direction of this!

xymostech · 2017-03-10T23:11:14Z

src/generate.js

-    const merged /* : OrderedElements */ = styleTypes.reduce(
-        recursiveMerge,
-        new OrderedElements());
+    const merged = new OrderedElements();


This is no longer doing a recursive merge.

That's right. When I did this, I thought it would be alright since the tests pass, but I think actually this is a gap in the tests. I've fixed this, and in the process I think I found and fixed an existing bug where nested objects are mutated. My new implementation does not feel the cleanest anymore, so it could probably use a little refactoring.

xymostech · 2017-03-20T23:56:25Z

src/generate.js

+                // value that was added by prefixAll. Let's try to figure out where it
+                // goes.
+                let originalStyle;
+                if (elementNames[i][0] === 'W') {


This seems vaguely fragile. Are we really sure that the only things that are not going to be in the original array here are prefixed things, not like, renamed properties or something?

Can you expand on what you mean by renamed properties?

If you look at line 284 you'll see if (!originalElements.hasOwnProperty(elementNames[i])) { which means that this should only be running on properties that were not there before the prefixAll() call but were there after it.

I actually don't remember what I meant by renamed properties.. :P

What I meant in general is that are we sure that the only properties that were not there before the prefixAll() call are ones that start with Webkit, Moz, or ms? Could there be others?

I just looked through the code of inline-style-prefixer and I was unable to find any others. @rofrischmann can you confirm?

xymostech · 2017-03-20T23:57:54Z

src/generate.js

-                // calculation above.
-                return sortOrder[key];
+                if (originalStyle && originalElements.hasOwnProperty(originalStyle)) {
+                    const originalIndex = handledDeclarations.keyOrder.indexOf(originalStyle);


Did your benchmark include any number of things that get prefixed? I'd imagine this might be slower in that case. On the other hand, are people going to be generating a whole bunch of styles that get prefixed?

Yep, my benchmark included some things that are prefixed and some things that are not prefixed. This is faster for both because it gets all of the work done in fewer loops and avoids creating new objects in favor of mutating.

xymostech · 2017-03-20T23:58:43Z

src/generate.js

-        .map(([key, value]) => `${kebabifyStyleName(key)}:${transformValue(key, value)};`)
-        .join("");
+    const rules = [];
+    for (let i = 0; i < handledDeclarations.keyOrder.length; i ++) {


nit: no space before ++

xymostech · 2017-03-20T23:59:36Z

src/generate.js

+            // multiple rules for the same key. Here we flatten to multiple
+            // pairs with the same key.
+            for (let j = 0; j < value.length; j++) {
+                rules.push(transformRule(key, value[j], transformValue));


This is just going to put them in the same order that inline-style-prefixer gave them to us, correct? Is that order in the order that we want it to be?

I think that's correct. At least in the examples I was looking at, this seemed to be the order we wanted. Although I'm not sure if that is always true or if it will remain stable. It might be worth adding a check here to ensure that the original or unprefixed value is last.

xymostech · 2017-03-21T00:09:44Z

src/util.js

@@ -21,9 +21,6 @@ export const mapObj = (
    return mappedObj;
 }

-export const flattenDeep = (list /* : any[] */) /* : any[] */ =>
-    list.reduce((memo, x) => memo.concat(Array.isArray(x) ? flattenDeep(x) : x), []);


oh yeah, I forgot how horribly inefficient this function was. Thanks!

xymostech · 2017-03-21T00:11:24Z

package.json

    "npm-run-all": "^1.7.0",
    "nyc": "^6.4.4",
    "rimraf": "^2.5.2",
    "webpack": "^1.12.2"
  },
  "dependencies": {
    "asap": "^2.0.3",
+    "can-use-dom": "^0.1.0",


how big is this dependency? Can we just replace this with a typeof document !== "undefined" check like we do in other places? (or replace those other checks with this?)

The dependency is very small. Here's the whole thing:

var canUseDOM = !!( typeof window !== 'undefined' && window.document && window.document.createElement ); module.exports = canUseDOM;

I think it is best to prefer dependencies over code duplication, but if you feel strongly about it, I can inline it into this project.

Nope, using that module seems fine! I'm just cautious about including something if it's trying to do something more complicated.

xymostech · 2017-03-21T00:12:51Z

src/prefix.js

+    // prefixed styles.
+    const prefixer = new Prefixer({
+        keepUnprefixed: true,
+        //userAgent: navigator.userAgent,


nit: this is commented out.

xymostech · 2017-03-21T00:15:10Z

src/prefix.js

+
+    // We need keepUnprefixed so our sorting code knows how to order the
+    // prefixed styles.
+    const prefixer = new Prefixer({


Aside from server-side rendering, I know one of the reasons we initially didn't go with this approach is because the code to determine which prefixes were needed (which we're doing a build time in staticPrefixData) took up a lot of space. Can you build aphrodite before and after this change and see how much bigger the resulting dist file is?

Yeah, that's a fair point. I'll look into this when I have a moment.

Looks like this takes the build size from 73 KiB to 134 KiB.

:( That's a pretty big size bump. I'd like to get this landed because of the bug fix in it, but I'm worried about throwing this significant change onto users. Could we maybe pull this change out into a separate PR to discuss?

Sounds good!

xymostech · 2017-03-21T00:15:26Z

tests/prefix_test.js

+        const originalUserAgent = global.navigator && global.navigator.userAgent;
+        let prefix;
+
+        beforeEach(() => {


<3 great tests

lencioni · 2017-03-21T22:09:03Z

src/inject.js

-                    key, [val[key]], selectorHandlers, stringHandlers, false);
-            });
+
+            if (val instanceof OrderedElements) {


I'm not sure if we can change things so that this fork doesn't need to happen. If we can't it might be worth adding a utility function that takes either a plain object or OrderedElements instance and iterates over the keys/values for convenience.

Seems fine for now. Maybe add a TODO for later?

Yes, I will add a TODO.

My main concern is less about this specific piece of code and more about what this means for folks who have added their own string handlers that will also need to account for this.

lencioni · 2017-03-21T22:09:28Z

src/ordered-elements.js

    }

    forEach(callback /* : (string, any) => void */) {
        for (let i = 0; i < this.keyOrder.length; i++) {
-            callback(this.keyOrder[i], this.elements[this.keyOrder[i]]);
+            // (value, key) to match Map's API
+            callback(this.elements[this.keyOrder[i]], this.keyOrder[i]);


Note: I changed the order of arguments here to match how Map works.

lencioni · 2017-03-21T22:10:41Z

tests/generate_test.js

@@ -134,6 +154,24 @@ ${formatStyles(actual)}
        }], '.foo{color:blue !important;}');
    });

+    it('does not mutate nested objects', () => {


Note: this test exposes a bug that is currently in master that this commit fixes.

This might be #226?

xymostech

These changes seem fine now! And I'd like to get them out soon so we can get that bug fixed. :) I would like to pull out the prefixer change so that we can think about that separately before doubling the size of Aphrodite. Sound good?

Thank you so much for all the work you've been doing. I'm learning a lot about performance optimization!

xymostech · 2017-03-23T01:05:03Z

src/generate.js

+                // value that was added by prefixAll. Let's try to figure out where it
+                // goes.
+                let originalStyle;
+                if (elementNames[i][0] === 'W') {


I actually don't remember what I meant by renamed properties.. :P

What I meant in general is that are we sure that the only properties that were not there before the prefixAll() call are ones that start with Webkit, Moz, or ms? Could there be others?

xymostech · 2017-03-23T01:07:22Z

package.json

@@ -49,7 +49,6 @@
    "caniuse-api": "^1.5.3",
    "chai": "^3.3.0",
    "coveralls": "^2.12.0",
-    "es6-shim": "^0.35.3",


Oh, interesting. I guess if it's available in node 4, it's probably fine for us to remove it.

xymostech · 2017-03-23T01:12:21Z

tests/inject_test.js

@@ -369,11 +369,11 @@ describe('String handlers', () => {
            css(sheet.animate);
            flushToStyleTag();

-            assertStylesInclude('@keyframes keyframe_1kmnkfo');
+            assertStylesInclude('@keyframes keyframe_tmjr6');


why did this change?

I don't remember exactly anymore, but I think it was because previously the nested objects were plain objects and part of my change (making things actually recursive) caused them to be OrderedElements objects, so they serialized differently. This is related to the code I commented on here: https://github.com/Khan/aphrodite/pull/216/files#r107288563

xymostech · 2017-03-23T01:17:41Z

src/ordered-elements.js

        }
    }

    set(key /* : string */, value /* : any */) {
        if (!this.elements.hasOwnProperty(key)) {
            this.keyOrder.push(key);
        }
+


It seems non-intuitive that .set() does a deep merge of the elements. Could we maybe pull this deep merge into a separate method, or if this isn't being used anywhere else, rename this?

Yeah this makes sense to me. I'll try to look into this soon.

xymostech · 2017-03-23T01:23:18Z

src/inject.js

-            stringHandlers, useImportant);
-
-        injectGeneratedCSSOnce(key, generated);
+    if (alreadyInjected[key]) {


Nah, that's fine, I was just curious. :)

xymostech · 2017-03-23T01:24:25Z

package.json

    "npm-run-all": "^1.7.0",
    "nyc": "^6.4.4",
    "rimraf": "^2.5.2",
    "webpack": "^1.12.2"
  },
  "dependencies": {
    "asap": "^2.0.3",
+    "can-use-dom": "^0.1.0",


Nope, using that module seems fine! I'm just cautious about including something if it's trying to do something more complicated.

xymostech · 2017-03-23T01:28:26Z

src/prefix.js

+
+    // We need keepUnprefixed so our sorting code knows how to order the
+    // prefixed styles.
+    const prefixer = new Prefixer({


:( That's a pretty big size bump. I'd like to get this landed because of the bug fix in it, but I'm worried about throwing this significant change onto users. Could we maybe pull this change out into a separate PR to discuss?

xymostech · 2017-03-23T01:31:30Z

src/inject.js

-                    key, [val[key]], selectorHandlers, stringHandlers, false);
-            });
+
+            if (val instanceof OrderedElements) {


Seems fine for now. Maybe add a TODO for later?

lencioni

I think I'm going to split this up into a few separate PRs so we can focus on the specific changes a bit better.

lencioni · 2017-03-23T17:08:40Z

src/generate.js

+                // value that was added by prefixAll. Let's try to figure out where it
+                // goes.
+                let originalStyle;
+                if (elementNames[i][0] === 'W') {


I just looked through the code of inline-style-prefixer and I was unable to find any others. @rofrischmann can you confirm?

lencioni · 2017-03-23T17:09:48Z

src/inject.js

-                    key, [val[key]], selectorHandlers, stringHandlers, false);
-            });
+
+            if (val instanceof OrderedElements) {


Yes, I will add a TODO.

My main concern is less about this specific piece of code and more about what this means for folks who have added their own string handlers that will also need to account for this.

lencioni · 2017-03-23T17:10:28Z

src/ordered-elements.js

        }
    }

    set(key /* : string */, value /* : any */) {
        if (!this.elements.hasOwnProperty(key)) {
            this.keyOrder.push(key);
        }
+


Yeah this makes sense to me. I'll try to look into this soon.

lencioni · 2017-03-23T17:10:40Z

src/prefix.js

+
+    // We need keepUnprefixed so our sorting code knows how to order the
+    // prefixed styles.
+    const prefixer = new Prefixer({


Sounds good!

lencioni · 2017-03-23T17:12:20Z

tests/inject_test.js

@@ -369,11 +369,11 @@ describe('String handlers', () => {
            css(sheet.animate);
            flushToStyleTag();

-            assertStylesInclude('@keyframes keyframe_1kmnkfo');
+            assertStylesInclude('@keyframes keyframe_tmjr6');


I don't remember exactly anymore, but I think it was because previously the nested objects were plain objects and part of my change (making things actually recursive) caused them to be OrderedElements objects, so they serialized differently. This is related to the code I commented on here: https://github.com/Khan/aphrodite/pull/216/files#r107288563

lencioni · 2017-03-23T17:18:31Z

Once #233 is merged, I'll rebase this again and it will be only 2 commits.

SamyPesse · 2017-04-13T11:02:20Z

Thanks @lencioni 🍻

Any chance someone from Khan can review it ? (@xymostech) This PR fixes #231.

We only use this in one place, so I decided to rewrite this for the specific use-case to improve performance. While I was at it, I found and fixed a bug that causes nested objects to be mutated. I decided to change the signature of OrderedElements's forEach callback to match that of Map's forEach. In my benchmark this brings css() down from 1470ms to 1350ms.

In my profiling, calling flattenDeep in here takes 1437ms out of 3418ms, and that's with already flattened arrays. By refactoring this code, we can actually avoid a lot of the work done here and make this faster. This change drops css() down from 3418ms to 2045ms.

lencioni

@xymostech I've rebased this but didn't make any code changes.

lencioni · 2017-04-20T18:44:33Z

src/inject.js

+            // utility method that can iterate over either a plain object, an
+            // instance of OrderedElements, or a Map, and then use that here and
+            // elsewhere.
+            if (val instanceof OrderedElements) {


This is the bit that I am mostly worried about. But, it looks like string handlers can't be added by extensions, so maybe this is safe?

elischutze · 2017-04-27T15:28:46Z

this looks great I support this

lencioni · 2017-04-27T15:39:31Z

I have @xymostech's approval for these changes, so I'm going to merge this. But @xymostech or someone else with access will either need to publish a new version or give me access (I'm lencioni on npm).

jlfwong · 2017-04-27T18:49:17Z

@lencioni Just gave you access! The version bump should happen with npm version, then the result committed and pushed

lencioni · 2017-04-27T20:28:04Z

Great! I should have some time this afternoon to take care of this.

lencioni force-pushed the optimize-generateCSSRuleset branch 2 times, most recently from 51cf81d to 5c6106b Compare March 10, 2017 20:50

lencioni commented Mar 10, 2017

View reviewed changes

lencioni force-pushed the optimize-generateCSSRuleset branch 2 times, most recently from aa483df to 187056b Compare March 10, 2017 22:39

lencioni commented Mar 10, 2017

View reviewed changes

lencioni force-pushed the optimize-generateCSSRuleset branch from 187056b to 678f11e Compare March 10, 2017 22:50

lencioni changed the title ~~Optimize generateCSSRuleset~~ Optimize css() Mar 10, 2017

lencioni force-pushed the optimize-generateCSSRuleset branch from 678f11e to 13a7422 Compare March 11, 2017 16:50

lencioni force-pushed the optimize-generateCSSRuleset branch from 24ab852 to 2325e6d Compare March 11, 2017 17:56

lencioni mentioned this pull request Mar 11, 2017

Optimize flattenDeep #214

Closed

lencioni requested a review from xymostech March 13, 2017 16:36

xymostech suggested changes Mar 21, 2017

View reviewed changes

lencioni force-pushed the optimize-generateCSSRuleset branch 2 times, most recently from ebf5d3a to a54fd13 Compare March 21, 2017 22:02

lencioni commented Mar 21, 2017

View reviewed changes

lencioni mentioned this pull request Mar 22, 2017

Bug: Pseudo rules (:after) are incorrectly combined at runtime #231

Closed

xymostech approved these changes Mar 23, 2017

View reviewed changes

lencioni mentioned this pull request Mar 23, 2017

Exclude tests directory from test coverage, update .gitignore and .eslintignore #232

Merged

lencioni commented Mar 23, 2017

View reviewed changes

lencioni mentioned this pull request Mar 23, 2017

Optimize css() some #233

Merged

lencioni force-pushed the optimize-generateCSSRuleset branch from a54fd13 to 893386a Compare March 23, 2017 17:17

lencioni force-pushed the optimize-generateCSSRuleset branch from 893386a to 7831e48 Compare March 23, 2017 17:20

lencioni and others added 2 commits April 20, 2017 11:40

lencioni force-pushed the optimize-generateCSSRuleset branch from 7831e48 to 0700a57 Compare April 20, 2017 18:41

lencioni commented Apr 20, 2017

View reviewed changes

lencioni merged commit cd1069b into master Apr 27, 2017

lencioni deleted the optimize-generateCSSRuleset branch April 27, 2017 15:39

Optimize css() #216

Optimize css() #216

Conversation

lencioni commented Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 11, 2017

coveralls commented Mar 11, 2017

coveralls commented Mar 13, 2017

coveralls commented Mar 13, 2017

iancmyers commented Mar 16, 2017

lencioni commented Mar 17, 2017

xymostech commented Mar 17, 2017

xymostech commented Mar 20, 2017

lencioni commented Mar 20, 2017

xymostech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xymostech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lencioni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lencioni commented Mar 23, 2017

SamyPesse commented Apr 13, 2017

lencioni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elischutze commented Apr 27, 2017

lencioni commented Apr 27, 2017

jlfwong commented Apr 27, 2017

lencioni commented Apr 27, 2017

lencioni commented Mar 10, 2017 •

edited

Loading