Skip to content

Remove a static allocation from Fetch JS code #12040

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

Merged
merged 1 commit into from
Aug 25, 2020
Merged

Remove a static allocation from Fetch JS code #12040

merged 1 commit into from
Aug 25, 2020

Conversation

kripken
Copy link
Member

@kripken kripken commented Aug 25, 2020

@kripken kripken requested a review from sbc100 August 25, 2020 22:19
@kripken kripken merged commit 9712c09 into master Aug 25, 2020
@kripken kripken deleted the nostat-fet branch August 25, 2020 22:58
@juj
Copy link
Collaborator

juj commented Aug 26, 2020

-1 for this and #12039 :( I think this is significantly worse direction in terms of code-gen. It regresses code size, but also carries a runtime branch á la style of static C function local initialization.

Is there some other way we could keep the original feature? It is very useful: eagerly doing the needed work once up front at startup is much better than lazily checking every time whether the needed thing was done yet.

@sbc100
Copy link
Collaborator

sbc100 commented Aug 26, 2020

In the case of the allocation being on the JS side I agree this is not ideal and C-style globals would be better.

In other places we have moved these static allocations into .c or .s files so that the the linker (wasm-ld) can include them. Perhaps we can do that in this case too? Although I'm not in this particular example if makes sense since fetching is already a very heavy operation, no? Is it worth fixing in this particular case?

sbc100 added a commit that referenced this pull request Aug 26, 2020
This avoids the runtime allocation on the JS side.

See: #12040
@sbc100
Copy link
Collaborator

sbc100 commented Aug 26, 2020

Actually in this case it looks like maybe there is simple fix: #12049

@kripken
Copy link
Member Author

kripken commented Aug 26, 2020

@juj

The c++ exceptions fix was also a correctness fix (to avoid a possible race condition), so it's unavoidable I think.

I agree that things like this one could be better, as in #12049 , thanks @sbc100 !

In general I think it's ok to take a tiny step back in some cases as in this PR. The benefits of getting rid of static allocations from JS are going to be worth it - once we get rid of all of them, we can remove DYNAMICTOP_PTR entirely from JS, for example, which I hope to land soon. That both simplifies the code and makes it smaller.

And, fundamentally these changes tend to make it easier to move code from JS to C, because they remove special JS magic (e.g. #12052 and #12055), so they are incremental steps towards further code size improvements.

sbc100 added a commit that referenced this pull request Aug 28, 2020
This avoids the runtime allocation on the JS side.

See: #12040
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