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 support for funcrefs inside GC objects #9341

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Oct 1, 2024

No description provided.

@fitzgen fitzgen requested a review from a team as a code owner October 1, 2024 00:01
@fitzgen fitzgen requested review from alexcrichton and removed request for a team October 1, 2024 00:01
@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:ref-types Issues related to reference types and GC in Wasmtime labels Oct 1, 2024
Copy link

github-actions bot commented Oct 1, 2024

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types"

Thus the following users have been cc'd because of the following labels:

  • fitzgen: wasmtime:ref-types

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@fitzgen
Copy link
Member Author

fitzgen commented Oct 1, 2024

Rebased to resolve conflicts.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is all reasonable enough, but this is definitely further amplifying my worries about the cost of GC and runtime performance. Every write of a funcref now requires at least a hash table lookup and reads can require taking a read lock on a global RwLock. On one hand I realize that the goal is to get GC working, but on the other hand I also have a hard time seeing how this is going to be a suitable performance profile for anyone actually using GC. I do realize though that improving on this is going to be significantly difficult to, for example, expose the slab to jit code or to implement a GC of the slab entries themselves.

In that sense I'm going to go ahead and approve this as-is since I don't believe there are any correctness issues with it. (one minor worry is that the hash map isn't GC'd right now, but that mirrors the behavior of we don't implement instance-level GC within a Store so they're O(N) the same size more-or-less). Do you think it would be useful though to build up a list of the possible performance optimizations we can think of to keep track of things? I do still agree that this is the best route for getting things implemented and to a point where we can measure performance to see what to optimize, but I'm also afraid of losing context on the myriad of possible ways to optimize things as PRs land incrementally

@fitzgen
Copy link
Member Author

fitzgen commented Oct 1, 2024

Do you think it would be useful though to build up a list of the possible performance optimizations we can think of to keep track of things?

Yeah, I can write up an issue with a list of these.

@fitzgen
Copy link
Member Author

fitzgen commented Oct 1, 2024

I share your concerns. It certainly is not going to be performant as-is. My most-immediate thoughts are that we would do roughly the following to speed this up:

  • Expose the slab to Wasm, allowing id-to-funcref conversion to happen fully within wasm code
  • for funcref-to-id conversion, add an LRU/associative cache to the vmctx (or maybe runtime limits) to cache the results of the libcall and allow the happy path to stay within wasm code. the slow path would still fall back to a libcall however (I do not want to implement hashing in wasm code and try to keep it in sync with the Rust hashing)

My hope is that the above would result in good enough perf for us to not have to revisit this for quite a while.

@fitzgen fitzgen added this pull request to the merge queue Oct 1, 2024
@alexcrichton
Copy link
Member

Two questions actually:

  • On the write side I was hoping we could do something purely in JIT code where it allocates space from the slab itself and the slab slots are managed by the normal GC. Would that be possible? For example during tracing we'd record which slab slots were in use and deallocation would be to iterate over the existing slab slots (and compaction may not be too hard either).
  • On the read side is there any way to avoid the lock? That seems required currently for our threat model, but I'm basically wondering if there's any way we can get away with just an index comparison as opposed to a full subtype check.

Merged via the queue into bytecodealliance:main with commit f07c441 Oct 1, 2024
39 checks passed
@fitzgen fitzgen deleted the funcrefs-in-gc-heap branch October 1, 2024 16:51
@fitzgen
Copy link
Member Author

fitzgen commented Oct 1, 2024

  • On the read side is there any way to avoid the lock?

I think we solve this via the approach that we have talked about previously of having a shared, lock-free arena for the supertypes arrays, so that particular subtyping checks don't need to lock anything.

Will write up an issue about this soon.

@fitzgen
Copy link
Member Author

fitzgen commented Oct 1, 2024

On the write side I was hoping we could do something purely in JIT code where it allocates space from the slab itself and the slab slots are managed by the normal GC. Would that be possible? For example during tracing we'd record which slab slots were in use and deallocation would be to iterate over the existing slab slots (and compaction may not be too hard either).

I think the tracing part is definitely doable. Compaction should also be possible. But the tricky part will be deduplicating funcrefs in the slab (i.e. the reason the hash map is in there now). Without that deduping, I fear that it will be too easy to fill the funcref table, require a GC, and end up thrashing the collector.

@fitzgen
Copy link
Member Author

fitzgen commented Oct 1, 2024

  • On the read side is there any way to avoid the lock?

I think we solve this via the approach that we have talked about previously of having a shared, lock-free arena for the supertypes arrays, so that particular subtyping checks don't need to lock anything.

Will write up an issue about this soon.

#9352

@fitzgen
Copy link
Member Author

fitzgen commented Oct 1, 2024

Do you think it would be useful though to build up a list of the possible performance optimizations we can think of to keep track of things?

Yeah, I can write up an issue with a list of these.

#9351

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:ref-types Issues related to reference types and GC in Wasmtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants