Skip to content

Move emscripten_get_heap_size to native code #13695

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 18, 2021
Merged

Move emscripten_get_heap_size to native code #13695

merged 1 commit into from
Mar 18, 2021

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Mar 17, 2021 •
edited
Loading

This is good for standalone mode and is a useful precursor to #13688

@sbc100 sbc100 requested a review from kripken March 17, 2021 22:15
@sbc100 sbc100 force-pushed the move_heap_size_to_native branch from bf789eb to 3b6e53e Compare March 17, 2021 22:18
@sbc100 sbc100 force-pushed the move_heap_size_to_native branch 5 times, most recently from ed30241 to b598191 Compare March 18, 2021 15:54
@sbc100 sbc100 mentioned this pull request Mar 18, 2021
Closed
@sbc100 sbc100 force-pushed the move_heap_size_to_native branch from b598191 to e3855c3 Compare March 18, 2021 15:56
@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 18, 2021

I ended up moving emscripten_resize_heap to native code too to avoid the reverse dependency. PTAL.

I'm open to better names for emscripten_heap_resize_internal. Although it mostly and implementation detail it will effect those trying to re-implement emscripten's ABI. Maybe emscripten_heap_grow is better?

kripken
kripken approved these changes Mar 18, 2021
View reviewed changes
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

resize_heap_internal sounds fine as we don't intend this for users to call directly. Maybe it should also have a __ prefix? not sure if we do that anywhere else.

@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 18, 2021

resize_heap_internal sounds fine as we don't intend this for users to call directly. Maybe it should also have a __ prefix? not sure if we do that anywhere else.

If we go with the __ prefix I could just call it __emscripten_resize_heap maybe?

All symbols that get imported from JS do end up in the global linker symbol table (even if they are not in header files like this one), so I do think that all JS imports should have some kind of name prefix (either emscripten or __). Would you agree? I'm not sure how consistent we are about this in general with out JS APIs.

@kripken
Copy link
Member

kripken commented Mar 18, 2021

Good point. Yeah, then __emscripten_resize_heap sounds good.

@sbc100 sbc100 force-pushed the move_heap_size_to_native branch 2 times, most recently from 14a1442 to 820a8a6 Compare March 18, 2021 17:23
@sbc100 sbc100 enabled auto-merge (squash) March 18, 2021 17:31
@sbc100 sbc100 force-pushed the move_heap_size_to_native branch 2 times, most recently from a95b4a8 to 018dd42 Compare March 18, 2021 19:31
@sbc100 sbc100 force-pushed the move_heap_size_to_native branch from 018dd42 to 540e28c Compare March 18, 2021 19:35
@sbc100 sbc100 merged commit e1e682f into main Mar 18, 2021
@sbc100 sbc100 deleted the move_heap_size_to_native branch March 18, 2021 20:15
sbc100 added a commit that referenced this pull request Mar 19, 2021
@sbc100
Fix emscripten_heap_resize
09fdaad 
This is a partial revert of #13695 which broke
test_zzz_zzz_emmalloc_4gb.

The `oldSize` argument was being interpreted by signed by JS.
Simpler just to read it directly rather than use the wrapper
function.
@sbc100 sbc100 mentioned this pull request Mar 19, 2021
Merged
sbc100 added a commit that referenced this pull request Mar 19, 2021
@sbc100
Fix emscripten_heap_resize
24c4950 
This is a partial revert of #13695 which broke
test_zzz_zzz_emmalloc_4gb.

The `oldSize` argument was being interpreted by signed by JS.
Simpler just to read it directly rather than use the wrapper
function.
sbc100 added a commit that referenced this pull request Mar 19, 2021
@sbc100
Fix emscripten_heap_resize
4b40bd6 
This is a partial revert of #13695 which broke
test_zzz_zzz_emmalloc_4gb.

The `oldSize` argument was being interpreted by signed by JS.
Simpler just to read it directly rather than use the wrapper
function.
sbc100 added a commit that referenced this pull request Mar 19, 2021
@sbc100
Fix emscripten_heap_resize
8a9bbaa 
This is a partial revert of #13695 which broke
test_zzz_zzz_emmalloc_4gb.

The `oldSize` argument was being interpreted by signed by JS.
Simpler just to read it directly rather than use the wrapper
function.
sbc100 added a commit that referenced this pull request Mar 19, 2021
@sbc100
Fix emscripten_heap_resize (#13716)
3af9d8d 
This is a partial revert of #13695 which broke
test_zzz_zzz_emmalloc_4gb.

The `oldSize` argument was being interpreted by signed by JS.
Simpler just to read it directly rather than use the wrapper
function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kripken kripken kripken approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sbc100 @kripken