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

[idea] Construct an array of strings for transactions instead of one big string #1542

Closed
wants to merge 1 commit into from

Conversation

syranide
Copy link
Contributor

Another poo-commit.

It can probably be improved further (performance wise), but it has some interesting results, Chrome: http://imgur.com/pHROMTO (right/red is this PR, left/black is master)

While I find that those perf tests tend to differ between each try, there are some tests that seem to show a significant and stable improvement of 5-8% it seems. I would also assume that this improvement increases for larger DOM updates (especially the initial rendering).

Running it through my old "torture-test" of mounting and unmounting a large tree, IE8 shows 0 difference, but IE9 runs 20% faster and IE10/FF runs 10% faster and IE11/Chrome is around 5% faster (just for mount+unmount, so no rendering involved and not layouting either I think).

I'm assuming this would also put significantly less pressure on the GC as new strings don't have to be created for every concatenation.

Pending your input on whether to proceed (and do it proper) or not.

@syranide
Copy link
Contributor Author

cc @spicyj @petehunt

@sophiebits
Copy link
Collaborator

.push() can take multiple arguments, which might be faster.

@syranide
Copy link
Contributor Author

Updated code and perfs.

IE9: http://i.imgur.com/Wu9518y.png
IE11: http://i.imgur.com/2v9Wb03.png
Chrome: http://i.imgur.com/Of9qEkY.png

Some quite conclusive (but not mindblowing) improvements in some tests it seems.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@sophiebits
Copy link
Collaborator

@jordwalke What do you think of this?

zpao added a commit that referenced this pull request Jun 20, 2014
Add harmony transform support in browser

Fixes #1420. Moved some code around in the merge to account for more
changes in the transform code.

Conflicts:
	vendor/browser-transforms.js
@syranide
Copy link
Contributor Author

Note to self, assigning to an index is considerably faster than push.

@syranide syranide changed the title Construct an array of strings for transactions instead of one big string [RFC] Construct an array of strings for transactions instead of one big string Sep 29, 2014
@syranide syranide changed the title [RFC] Construct an array of strings for transactions instead of one big string [idea] Construct an array of strings for transactions instead of one big string Sep 29, 2014
@sebmarkbage
Copy link
Collaborator

I had a diff that did this previously. It's tricky because if you use the optimizations right, then string concatenation can actually be faster.

I haven't looked at this closely enough to know but I suspect you might have some problems coordinating this with React ART.

I definitely think that we'll probably do something like this but I think we need a bigger refactoring around the contract of the reconciler for things like React ART to plug into. Might not be worth while just doing this on it's own.

@syranide
Copy link
Contributor Author

syranide commented Oct 2, 2014

@sebmarkbage Definitely, as the tests shows (not that I really trust them) there's probably nothing here worth bending backwards over either. If you overdo it just a little it actually becomes slower. This was just an FYI really, feel free to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants