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

wasm-interp: Handle refs_ correctly #2484

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SoniEx2
Copy link
Collaborator

@SoniEx2 SoniEx2 commented Oct 5, 2024

This fills in some TODOs, and cleans up some other issues, relevant for GC later on.

We tried our best to make this efficient, hence the custom wabt opcodes.

@SoniEx2
Copy link
Collaborator Author

SoniEx2 commented Oct 7, 2024

@sbc100 (rebased, ptal)

@SoniEx2
Copy link
Collaborator Author

SoniEx2 commented Oct 7, 2024

oh yeah, we should probably bring it up: an alternative to the funny stuff with locals and refs_ would be to mark ref locals at function start, which would be decidedly faster if slightly more annoying to set up. but then again, we weren't really going for maximum performance with this.

@SoniEx2 SoniEx2 marked this pull request as draft January 19, 2025 05:03
@SoniEx2 SoniEx2 marked this pull request as ready for review March 2, 2025 01:27
@SoniEx2
Copy link
Collaborator Author

SoniEx2 commented Mar 2, 2025

we don't know how complete the interpreter GC (not to be confused with the GC proposal and its implementation) is (how do you even test something like that), but this "should" mark refs correctly now. we think. we're pretty sure, but we wouldn't mind a second opinion.

@SoniEx2 SoniEx2 requested review from keithw and sbc100 March 2, 2025 01:31
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Are there any tests we can now pass after this change?

@SoniEx2
Copy link
Collaborator Author

SoniEx2 commented Mar 4, 2025

as we were saying, we're not sure if there's a (good) way to test this?

as far as we can tell, Store::Collect is never called while the interpreter is running, so it kinda just... leaks?

@sbc100
Copy link
Member

sbc100 commented Mar 4, 2025

So are you proposing landing this without tests? Or is this still a draft PR with more changes coming maybe?

@SoniEx2
Copy link
Collaborator Author

SoniEx2 commented Mar 4, 2025

we suppose we can try making the GC run regularly and see if it blows up... (but actually enabling the GC to run on its own should be a separate change)

@SoniEx2
Copy link
Collaborator Author

SoniEx2 commented Mar 4, 2025

running GC every instruction does indeed blow up.

@SoniEx2 SoniEx2 changed the title wasm-interp: Handle Refs wasm-interp: Handle refs_ correctly Mar 4, 2025
@SoniEx2
Copy link
Collaborator Author

SoniEx2 commented Mar 4, 2025

we can't currently add tests (actually this doesn't need new tests as much as it needs a way to run the existing tests with actual collection enabled), blocked on fixing a nasty issue with spectest-interp, but these changes do fix a bunch of assertion failures when running store_.Collect() for every instruction. we would indeed like to land this without tests (for now).

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.

2 participants