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

Use a shared null object for all Stores. #1860

Closed
wants to merge 1 commit into from

Conversation

zherczeg
Copy link
Contributor

No description provided.

@sbc100
Copy link
Member

sbc100 commented Mar 11, 2022

What is the goal of this change? Is it an optimization or something about being able to share this object between stores?

If its an optimization then I would ask why such micro-optimizations are needed? Are you huge number of stores?

@zherczeg
Copy link
Contributor Author

It is a simple optimization. If you think this is unnecessary, I can close the PR. I would like to help improving the interpreter, is there any task which is useful for you? I would focus on garbage collection and memory, but I can help with other things.

@sbc100
Copy link
Member

sbc100 commented Mar 12, 2022

It is a simple optimization. If you think this is unnecessary, I can close the PR. I would like to help improving the interpreter, is there any task which is useful for you? I would focus on garbage collection and memory, but I can help with other things.

If the optimization is to avoid the bytes needed to represent NULL in each store I do think it might be premature, yes. I imagine this is a couple of words of memory per-store? But I don't know if anyone who uses the wabt interpreter on a scale where that would matter.

If you are interested in helping out wabt in general what would be great! Let me go through the open issues and prioritize some of them and get back to you.

@sbc100
Copy link
Member

sbc100 commented Mar 13, 2022

If are you interested in helping out with GC in general we could use some help workon and merging the work that @binji started in to get the GC proposal implemented: #1257

@zherczeg
Copy link
Contributor Author

Thank you for the suggestion. At first sight it looks like it adds a new struct / array related code. The work looks quite a big, and does a lot of different things. Is it possible to split this into well defined subtasks? I see references to draft documents. Is there a final version available somewhere?

@sbc100
Copy link
Member

sbc100 commented Mar 14, 2022

Thank you for the suggestion. At first sight it looks like it adds a new struct / array related code. The work looks quite a big, and does a lot of different things. Is it possible to split this into well defined subtasks? I see references to draft documents. Is there a final version available somewhere?

The GC proposal is still in flight but recently reached stage 2 so is less likely to have huge changes going forward.

But yes, I think it pretty large amount of work. There are certainly many smaller issues in wabt that we could have you look at.

@zherczeg
Copy link
Contributor Author

I am ok working on this but I would prefer to do incremental changes rather than a huge patch, Something like introducing the support of some opcodes, and adding tests for them. I am still new here and needs to learn a lot.

This document mentions two prerequisites:
https://github.com/WebAssembly/gc/blob/master/proposals/gc/MVP.md

Are these supported? If not, and not too complicated, maybe I could try to do them. But I am open to any ideas.

@sbc100
Copy link
Member

sbc100 commented Mar 15, 2022

Another useful task that could be more reasonable in size would be to update the testsuite repo (using this script: https://github.com/WebAssembly/testsuite/blob/main/update-testsuite.sh) and then update the submodule dependency in wabt, and then run ./test/update-spec-tests.py here in wabt, and fix any issues that come out of that.

That process bascially ensure that wabt stays to-to-date with any specs that it purports to support.

@keithw
Copy link
Member

keithw commented Feb 13, 2023

Closing as dormant (it looks like we decided not to do this).

@keithw keithw closed this Feb 13, 2023
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