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

Improved heap growth check in pthreads with memory growth enabled? #12783

Open
juj opened this issue Nov 15, 2020 · 4 comments
Open

Improved heap growth check in pthreads with memory growth enabled? #12783

juj opened this issue Nov 15, 2020 · 4 comments
Labels

Comments

@juj
Copy link
Collaborator

juj commented Nov 15, 2020

Views to WebAssembly.Memory seem to have this peculiar behavior, that if one .grow()s a memory, all the existing views in the same thread do become zero-length, but all the existing views to the Memory in other threads preserve their unresized lengths. Is that something that is mandated by the Wasm/SAB spec? (CC @lars-t-hansen)

If so, it seems that we could do much better with the whole "need to check in JS if heap was resized before a heap access" scheme.

For example, if a function performs several heap accesses, like

  $writeI53ToI64: function(ptr, num) {
    HEAPU32[ptr>>2] = num;
    HEAPU32[ptr+4>>2] = (num - HEAPU32[ptr>>2])/4294967296;
  },

We would not need to replace all the accesses with GROWABLE_HEAP_U32() checks, just doing the check once in the function start would be enough.

Actually, in many cases, we would not need to do the check in the function writeI53ToI64() at all, but we could lift it to the calling function to have to do the check. That would be beneficial if the calling function does many writeI53ToI64() calls in a row.

Applying that logic transitively, we would need to only emit checks to
a) in the beginning of each JS function that is an import to Wasm that does a heap access, and
b) right after calling any Wasm export function from a JS function that does a heap access (e.g. _malloc())

Would this be something we'd be apply to the acorn optimizer pass? The tricky part is that we would have to transitively analyse program flow to find which functions do perform heap accesses.

Alternatively, we could apply annotation to JS function imports, like e.g.

  glCreateProgram__heapAccess: 'none',
  glCreateProgram: function() {
    var id = GL.getNewId(GL.programs);
    var program = GLctx.createProgram();
    program.name = id;
    GL.programs[id] = program;
    return id;
  }

where maybe we could support none (developer will manually insert any heap growth checks in the JS code if needed), or entry (add a heap check in the beginning of the function), or all (add a heap check to every heap access).

With either such an annotation scheme, or automatically doing control flow analysis, it seems that the performance impact of needing to do manual heap growth checks could be reduced to be negligible?

@lars-t-hansen
Copy link

I need to look at the spec + code to be sure, but that's really strange behavior. IIRC, existing views in the same thread should retain the original length. (Since you say "other threads" I assume we're only talking about shared memories here.)

@juj
Copy link
Collaborator Author

juj commented Nov 15, 2020

Hmm, thanks for clarifying. That prompted me to retry the behavior on the same thread again, and I do see the view staying the same:

test

Maybe I botched something up in the first test..

But good to hear that retaining the view to the unresized length is guaranteed, so we should be able to greatly improve the codegen for heap resizes. @kripken what is your thought?

@kripken
Copy link
Member

kripken commented Nov 16, 2020

It might be even simpler to instrument the wasm imports and exports to do this check once on each entrance/exit from the wasm. We have a few other such instrumentations, see e.g. library_async.js.

@juj juj mentioned this issue Nov 19, 2020
@stale
Copy link

stale bot commented Apr 17, 2022

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Apr 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants