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

Add Properties To Memory for Each View #815

Closed
wants to merge 1 commit into from

Conversation

willemneal
Copy link
Contributor

Add U8, I8, ... properties to memory that wrap the internal buffer with the corresponding view.

@MaxGraey
Copy link
Member

MaxGraey commented Sep 4, 2019

Hmm, interesting. But I worry about performance. Could you please make some perf measurements for before and after this changes? And also how this affect to size especially for minified loader

@willemneal
Copy link
Contributor Author

willemneal commented Sep 4, 2019

Is there a benchmark that I could use? Or do you just want a simple one. I mostly changed all the internal views, e.g. new Uint32Array(memory.buffer) to use the property, which is a small function call, which I'm sure will be optimized by the runtime. So for now is it okay to just run the tests w/ and w/o?

I'm also unsure what you mean for minified loader? I didn't know that it was. Non-minified it added 800 bytes. But this functionality was previously available and users will probably be fine with the size.

@MaxGraey
Copy link
Member

MaxGraey commented Sep 4, 2019

I checked size.
Before: The minified output (6432 bytes, saved 56.91%)
After: The minified output (6751 bytes, saved 57.14%)

I used this tool: https://skalman.github.io/UglifyJS-online

So this changes add extra 319 bytes and probably could slowdown code (while jit not warmsed up)

@dcodeIO
Copy link
Member

dcodeIO commented Sep 5, 2019

One of the learnings in #810 was that wrapping a buffer in new TypedArray seems to have no particular performance implications in modern VMs as one might expect by using new (like an allocation happening or similar), and as such is preferable over providing convenient views while also eliminating the need to check for growing memory. So one important question is whether returning a new TypedArray has other performance implications? And ofc if the convenience introduced outweighs the code size increase in typical cases.

@willemneal
Copy link
Contributor Author

willemneal commented Sep 5, 2019

Here's an interesting test I made comparing new TypedArray vs TypedArray.from. I'll try adding a caching test next.

Oops! here: https://jsperf.com/uint8array-new-vs-from

@MaxGraey
Copy link
Member

MaxGraey commented Sep 5, 2019

@willemneal thanks for sharing!

Chrome (good)

new Uint8Array(buffer): 36,229,083 ops/sec
Cache Test p = 0:       37,854,347 ops/sec

Firefox (worst)

new Uint8Array(buffer): 5,935,337 ops/sec
Cache Test p = 0:       34,366,746 ops/sec

Which pretty expected that's why we decide remove memcheck helper

@willemneal
Copy link
Contributor Author

Firefox 68:

Test Ops/sec
x = new Uint8Array(buffer); 2,341,094 ±4.14% 81% slower
x = Uint8Array.from(buffer); 1,458,705 ±5.03% 88% slower
getArray(0) 12,451,138 ±4.61% fastest
getArray(0.25) 11,004,172 ±5.79% 13% slower
getArray(0.5) 9,533,480 ±5.57% 24% slower
getArray(0.75) 9,510,627 ±6.15% 25% slower
getArray(1) 9,366,052 ±4.75% 25% slower

So weird!

@MaxGraey
Copy link
Member

MaxGraey commented May 16, 2020

Due to some bad results in FF I'm closing this

@MaxGraey MaxGraey closed this May 16, 2020
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