Skip to content

Commit

Permalink
Avoid deoptimization in generateCSS() (#204)
Browse files Browse the repository at this point in the history
I was profiling the css() function and Chrome raised a flag on this
function:

> Not optimized: Bad value context for arguments value

More info on this warning:
GoogleChrome/devtools-docs#53 (comment)

Looking at the warning and the compiled version of this code, it seems
to do some things with `arguments` when using the default values
here, which is causing this deoptimization.

```js
var selectorHandlers = arguments.length <= 2 || arguments[2] === undefined ? [] : arguments[2];
var stringHandlers = arguments.length <= 3 || arguments[3] === undefined ? {} : arguments[3];
var useImportant = arguments.length <= 4 || arguments[4] === undefined ? true : arguments[4];
```

By removing the default values for the arguments, the deoptimization
disappears. I thought about adding logic that would provide values for
these arguments if they aren't defined, but since the only thing that
relies on that is tests I decided to just update the tests to always
pass all of the arguments.

In my benchmark, this does not seem to make much of a difference but it
still seems like a good idea to avoid things that the browser tells us
is deoptimized.
  • Loading branch information
lencioni authored and xymostech committed Mar 6, 2017
1 parent 7c1e609 commit 41b3e17
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 5 deletions.
6 changes: 3 additions & 3 deletions src/generate.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,9 @@ export const defaultSelectorHandlers = [
export const generateCSS = (
selector /* : string */,
styleTypes /* : SheetDefinition[] */,
selectorHandlers /* : SelectorHandler[] */ = [],
stringHandlers /* : StringHandlers */ = {},
useImportant /* : boolean */ = true
selectorHandlers /* : SelectorHandler[] */,
stringHandlers /* : StringHandlers */,
useImportant /* : boolean */
) /* : string */ => {
const merged = styleTypes.reduce(recursiveMerge);

Expand Down
4 changes: 2 additions & 2 deletions tests/generate_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ describe('generateCSSRuleset', () => {
});
});
describe('generateCSS', () => {
const assertCSS = (className, styleTypes, expected, selectorHandlers,
stringHandlers, useImportant) => {
const assertCSS = (className, styleTypes, expected, selectorHandlers = [],
stringHandlers = {}, useImportant = true) => {
const actual = generateCSS(className, styleTypes, selectorHandlers,
stringHandlers, useImportant);
assert.equal(actual, expected.split('\n').map(x => x.trim()).join(''));
Expand Down

0 comments on commit 41b3e17

Please sign in to comment.