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

Possible perf improvement #89

Closed
wants to merge 1 commit into from
Closed

Conversation

nicholas-l
Copy link

I am not sure as to the performance improvements but for reading a long string, there will be a multiple allocations. This is because strings in Javascript are immutable and adding a character to a string causes a new allocation. Instead this change using an Array to temporary aggregate the characters and then at the end, join them together. There seems to be some change in benchmarks but they varied a lot.

I am not sure as to the performance improvements but for reading a long string, there will be a multiple allocations. This is because strings in Javascript are immutable and adding a character to a string causes a new allocation. Instead this change using an Array to temporary aggregate the characters and then at the end, join them together. There seems to be some change in benchmarks but they varied a lot.
@mourner
Copy link
Member

mourner commented Jun 8, 2018

Good point! We should definitely look into this, although it will likely only pay off for larger strings. String building in JS is tricky — it's fast up until the string is 12 characters, then it gets slow. See this thread https://twitter.com/mourner/status/973664460291411969

We should probably try some more benchmarks (with different string lengths) to see how this affects performance, and also maybe try buffered concatenation like above.

@kjvalencik
Copy link
Collaborator

kjvalencik commented Jun 8, 2018

Agreed. String building is weird in JS. I recently benchmarked some code and was surprised to find that simple string concatenation was nearly twice as fast as allocating a buffer of exactly the correct size and writing into it.

@nicholas-l
Copy link
Author

Yes, I noticed this in reading an attribute in a MVT which is a longer string.

@nicholas-l
Copy link
Author

This is a pretty unscientific test but this is the difference this pull request makes between the two in panning in Openlayers with a layers which include longer strings in a Vector Tile. Looks like a large reduction in memory allocated for the PBF (see readUTF8 at the top of the first list). Also we could include a check if end > 12 use string concatenation otherwise use arrays but this could be a micro-optimization for Chrome...

selection_018

selection_019

@mourner
Copy link
Member

mourner commented Jun 12, 2018

@nicholas-l note that profiling like this can be misleading — e.g. V8 has weird optimizations that can change which function allocations are attributed to, but the total size (~7.5MB) looks about the same.

@mourner
Copy link
Member

mourner commented Nov 12, 2019

This was addressed in #109 — should be much faster for long strings in the browser now.

@mourner mourner closed this Nov 12, 2019
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.

3 participants