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

Update README.emscripten #651

Closed
wants to merge 1 commit into from
Closed

Conversation

spaghettisalat
Copy link
Contributor

Closes #650

@ivmai
Copy link
Owner

ivmai commented Aug 1, 2024

@juj Do you agree with the readme change?

Repository owner deleted a comment from jul Aug 1, 2024
@juj
Copy link
Contributor

juj commented Aug 1, 2024

I would recommend expanding on the comment a little bit.. right now it might give a misimpression that adding the spill-pointers pass or scanning the stack would be unconditionally required - which it isn't, if user only triggers a GC at points when it is known there are no managed objects on the stack.

So I would recommend stating the readme in a way that would illustrate that there are these two strategies that one can use, and also illustrate about the drawbacks of each. The challenge with using --spill-pointers is that it greatly increases generated code size on disk, and there may also be a small % performance penalty to doing so.

@spaghettisalat
Copy link
Contributor Author

Do you have some example code for the strategy where one triggers the GC only at specific points which we can link to in the README? I guess this requires some modifications of the bdwgc source code to avoid the automatic GC runs, right?

At the moment, your description of how this strategy works is somewhat cryptic to me and therefore it is hard for me to write a good summary for the README.

@ivmai
Copy link
Owner

ivmai commented Aug 2, 2024

GC_disable() might be useful to avoid automatic GC

@juj
Copy link
Contributor

juj commented Aug 2, 2024

Yeah, GC_disable(), and then one can trigger an asynchronous GC e.g. by using emscripten_set_timeout() function that calls to a callback that manually runs GC_collect(); (I forget if bdwgc requires one to call GC_enable() before calling GC_collect() manually?)

@ivmai
Copy link
Owner

ivmai commented Aug 2, 2024

I forget if bdwgc requires one to call GC_enable() before calling GC_collect() manually?

Yes, GC_enable() is needed otherwise GC_collect() is no-op.

Add a note about the strategies than can be employed in order for the
gc to work correctly.
@spaghettisalat
Copy link
Contributor Author

Alright, I have extended the description in the README file a bit. I hope that should do it.

@juj
Copy link
Contributor

juj commented Aug 3, 2024

Looks really good!

ivmai pushed a commit that referenced this pull request Aug 15, 2024
PR #651 (bdwgc).

Update README.emscripten adding a note about the strategies that could
be employed in order for the collector to work correctly.

* docs/platforms/README.emscripten: Add info about strategies to deal
with Emscripten limitation of scanning data roots on stack; update
how-to-build instruction.
@ivmai
Copy link
Owner

ivmai commented Aug 15, 2024

Merged.

@ivmai ivmai closed this Aug 15, 2024
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.

Emscripten support fundamentally broken?
3 participants