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

Avoid creating a holey array in makeNimstrLit for JS target #16461

Merged
merged 4 commits into from
Dec 27, 2020

Conversation

jonahsnider
Copy link
Contributor

@jonahsnider jonahsnider commented Dec 24, 2020

This is a subtle, but important performance improvement in the makeNimstrLit proc for targeting JS.

The previous code would accept a string and construct an array of character codes. The way it did this was be creating an array with new Array(n). This creates a holey array (a sparse array) which V8 is very bad at optimizing compared to a packed array with no holes.

V8 makes this distinction because operations on packed arrays can be optimized more aggressively than operations on holey arrays. For packed arrays, most operations can be performed efficiently. In comparison, operations on holey arrays require additional checks and expensive lookups on the prototype chain. - Elements kinds in V8

And later in that same page:

const array = new Array(3);
// The array is sparse at this point, so it gets marked as
// `HOLEY_SMI_ELEMENTS`, i.e. the most specific possibility given
// the current information.

There are other instances where new Array(length) is used, which for lengths 1 and above means holey arrays. I could spend some more time going around replacing all instances of those with optimized versions if requested.

@timotheecour
Copy link
Member

can you write a benchmark to see the performance difference?

@jonahsnider
Copy link
Contributor Author

Yeah, I can do that later today.

@jonahsnider
Copy link
Contributor Author

jonahsnider commented Dec 27, 2020

It took me longer than I was hoping as I had to ask around about some interesting behavior with the benchmark.
You can run the benchmark online here.

The benchmarks consistently show that despite not creating a holey array, the newer code is slower. This is because it resizes the array as more elements are added, while the old code initializes the array to the proper size once.

I would argue that the new version should be preferred as the performance penalties of creating a holey array are worth sacrificing a microsecond to resize the array.

@Araq Araq merged commit fa1a041 into nim-lang:devel Dec 27, 2020
@timotheecour
Copy link
Member

timotheecour commented Dec 27, 2020

@pizzafox IMO the real savings will come from timotheecour#483, ie using typed array Uint8Array instead of dynamically typed Array; Uint8Array was introduced in js for this purpose among other things

the newer code is slower.

indeed, when I run this benchmark, the code after this PR is slower; shouldn't this warrant more investigation instead of merging this? I'm not sure what you mean by just "microseconds"; if something is 1.35x slower, this slowdown can be significant depending on application.

image

@jonahsnider jonahsnider deleted the patch-1 branch December 27, 2020 09:10
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
…#16461)

* Avoid creating a holey array in makeNimstrLit
* Use array index instead of push
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
…#16461)

* Avoid creating a holey array in makeNimstrLit
* Use array index instead of push
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