Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve DEV performance in Chrome #7483

Merged
merged 2 commits into from
Aug 12, 2016
Merged

Improve DEV performance in Chrome #7483

merged 2 commits into from
Aug 12, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 12, 2016

This consistently improves DEV performance in Chrome.

Before this change:

create 10k rows: 7861
update 1k rows: 1405
update 1k rows: 1337
update 1k rows: 1373
update 1k rows: 1340
update 1k rows: 1381
clear 10k rows: 1603

create 10k rows: 7799
update 1k rows: 1391
update 1k rows: 1339
update 1k rows: 1324
update 1k rows: 1338
clear 10k rows: 1567

After this change:

create 10k rows: 6460
update 1k rows: 1312
update 1k rows: 1237
update 1k rows: 1231
update 1k rows: 1263
update 1k rows: 1248
clear 10k rows: 1083

create 10k rows: 6326
update 1k rows: 1298
update 1k rows: 1233
update 1k rows: 1245
update 1k rows: 536
update 1k rows: 1058
update 1k rows: 1299
clear 10k rows: 1104

This only seems to help DEV in Chrome. I did not observe any significant difference in PROD in Chrome, or in either build in Firefox or Safari.

Normally it would be too much dogscience but I feel the wins for Chrome DEV (most common use case) are significant, they are consistently reproducible, and it kinda makes sense to use numbers anyway.

@gaearon gaearon changed the title Improve monomorphism Improve DEV performance in Chrome Aug 12, 2016
@ghost ghost added the CLA Signed label Aug 12, 2016
@gaearon gaearon added this to the 15-next milestone Aug 12, 2016
@ghost ghost added the CLA Signed label Aug 12, 2016
@vitkarpov
Copy link

vitkarpov commented Aug 12, 2016

I did not observe any significant difference in PROD

Kinda weird. Is Uglifyjs used for building or some another tool?

@sophiebits
Copy link
Collaborator

sophiebits commented Aug 12, 2016

Sounds good to me. Agree it's weird that prod mode is no faster.

@vitkarpov
Copy link

it kinda makes sense to use numbers anyway.

@gaearon Do you think it really makes sense? Actually, folks always use prod version in the wild.

@ghost ghost added the CLA Signed label Aug 12, 2016
@gaearon
Copy link
Collaborator Author

gaearon commented Aug 12, 2016

Kinda weird. Is Uglifyjs used for building or some another tool?

Yes, I uglified during the tests.

@gaearon Do you think it really makes sense? Actually, folks always use prod version in the wild.

As I said earlier this doesn’t have any downsides in production so I’m not sure what you are worried about.

I’m saying it “makes sense” because both these fields are numbers anyway so 0 makes more sense than null for resetting them. It’s nice that fixing this also improves perf in DEV mode in Chrome.

@gaearon gaearon merged commit 4b734f7 into facebook:master Aug 12, 2016
@gaearon gaearon deleted the better-monomorphism branch August 12, 2016 17:34
@vitkarpov
Copy link

these fields are numbers anyway so 0 makes more sense than null for resetting them

That's point! Agreed.

@zpao zpao modified the milestones: 15.3.1, 15-next Aug 12, 2016
zpao pushed a commit that referenced this pull request Aug 12, 2016
* Ensure this._domID is always a number

* Ensure this._rootNodeID is always a number

(cherry picked from commit 4b734f7)
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.

4 participants