Skip to content

Improve mount performance by bulk attachment#9

Closed
lttb wants to merge 8 commits intomasterfrom
feature/improve-mount-performance
Closed

Improve mount performance by bulk attachment#9
lttb wants to merge 8 commits intomasterfrom
feature/improve-mount-performance

Conversation

@lttb
Copy link
Copy Markdown
Member

@lttb lttb commented Apr 23, 2017

We have some overhead on detach and attach().link() each mount, so when we have a lot of components that we need to mount at the same time (like https://github.com/A-gambit/CSS-IN-JS-Benchmarks), there some issues.

This fix make significant performance improvement by bulk updating (more then 20x in example above).

TODO:

  • try asap, it may be faster

asap and raf have similar performance in bench above, but asap was little bit faster and had more stable results
UPDATE: but it seems that better to use raf for loops

@lttb lttb changed the title Improve mount performance by bulk Improve mount performance by bulk attachment Apr 23, 2017
Comment thread package.json
"jss": "^7.1.0",
"jss-preset-default": "^2.0.0"
"jss-preset-default": "^2.0.0",
"raf": "^3.3.0"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think raf polyfill should be responsibility of the user, like any other new features.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I would just use requestAnimationFrame, its well implemented right now anyways. All required polyfills should be in the readme

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use just window.requestAnimationFrame, because we run this code not only in the browser (tests, ssr) - so we need polyfill

But we can make it customizable

Comment thread src/index.js Outdated
raf(function update() {
if (dynamicCounter) {
dynamicCounter = 0
dynamicSheet.attach().link()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better call link() first

@kof
Copy link
Copy Markdown
Member

kof commented Apr 24, 2017

Tests should implement a mock if they need it.

@lttb
Copy link
Copy Markdown
Member Author

lttb commented Apr 24, 2017

Tests should implement a mock if they need it.

you are right, but the main point is ssr :)

@kof
Copy link
Copy Markdown
Member

kof commented Apr 24, 2017

mock it for ssr or don't use it when on the server

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-4.2%) to 86.047% when pulling 625e8ed on feature/improve-mount-performance into 8fbe4f9 on master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-5.7%) to 84.615% when pulling d1e7818 on feature/improve-mount-performance into 8fbe4f9 on master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+1.3%) to 91.579% when pulling 8f9005d on feature/improve-mount-performance into 8fbe4f9 on master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+1.7%) to 92.0% when pulling 6c9c36a on feature/improve-mount-performance into 8fbe4f9 on master.

@lttb lttb force-pushed the feature/improve-mount-performance branch 2 times, most recently from 47496c7 to 441f337 Compare April 25, 2017 11:55
@lttb lttb force-pushed the feature/improve-mount-performance branch from 441f337 to 972d102 Compare April 25, 2017 12:00
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+2.9%) to 93.137% when pulling 972d102 on feature/improve-mount-performance into 8fbe4f9 on master.

Comment thread src/createStyled.js

const addRule = (name: string, style: ComponentStyleType, data: Object) => {
if (data) {
dynamicSheet.detach().addRule(name, style)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a comment, its because of a bug, right?

raf(listen)
}

export default {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its not really an observer ... also it might be confused with a real Observer .

@lttb
Copy link
Copy Markdown
Member Author

lttb commented Apr 26, 2017

The main problem of this fix is async styles attaching, that may cause FUC (flash of unstyled content).
So we can return to this later.

Problem with mount performance solved in #12

@lttb lttb closed this Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants