Skip to content

Use emscripten_stack_set_limits in wasm_worker_initialize.S #22755

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

Merged
merged 1 commit into from
Oct 17, 2024
Merged

Use emscripten_stack_set_limits in wasm_worker_initialize.S #22755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions system/lib/compiler-rt/stack_limits.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,14 @@
#endif

.globaltype __stack_pointer, PTR
.globl __stack_pointer

.section .globals,"",@

# TODO(sbc): It would be nice if these we initialized directly
# using PTR.const rather than using the `emscripten_stack_init`
.globaltype __stack_end, PTR
__stack_end:
.globl __stack_end
.globaltype __stack_base, PTR
.globl __stack_base
__stack_base:

.section .text,"",@
Expand Down
26 changes: 15 additions & 11 deletions system/lib/wasm_worker/wasm_worker_initialize.S
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
.extern __stack_pointer
.extern __stack_base
.extern __stack_end

#ifdef __wasm64__
#define PTR i64
Expand All @@ -13,32 +11,37 @@
#endif

.globaltype __stack_pointer, PTR
.globaltype __stack_end, PTR
.globaltype __stack_base, PTR
.globaltype __tls_size, PTR, immutable
.functype emscripten_stack_set_limits (PTR /*base*/, PTR /*end*/) -> ()

.globl _emscripten_wasm_worker_initialize
_emscripten_wasm_worker_initialize:
.functype _emscripten_wasm_worker_initialize (PTR /*stackLowestAddress*/, i32 /*stackSize*/) -> ()
.local PTR, PTR

// __stack_end = stackLowestAddress + (__builtin_wasm_tls_size() + 15) & -16;
// stack_end = stackLowestAddress + (__builtin_wasm_tls_size() + 15) & -16;
local.get 0
.globaltype __tls_size, PTR, immutable
global.get __tls_size
PTR.add
PTR.const 0xf
PTR.add
PTR.const -0x10
PTR.and
global.set __stack_end
local.set 2

// __stack_base = stackLowestAddress + stackSize;
// stack_base = stackLowestAddress + stackSize;
local.get 0
local.get 1
#ifdef __wasm64__
i64.extend_i32_u
#endif
PTR.add
global.set __stack_base
local.set 3

local.get 3
local.get 2
// emscripten_stack_set_limits(stack_base, stack_end);
call emscripten_stack_set_limits

// TODO: We'd like to do this here to avoid JS side calls to __set_stack_limits.
// (or even better, we'd like to avoid duplicate versions of the stack variables)
Expand All @@ -48,6 +51,7 @@ _emscripten_wasm_worker_initialize:
// .functype __set_stack_limits (PTR, PTR) -> ()
// call __set_stack_limits


// __wasm_init_tls(stackLowestAddress);
local.get 0
.functype __wasm_init_tls (PTR) -> ()
Expand All @@ -57,8 +61,8 @@ _emscripten_wasm_worker_initialize:
// __stack_pointer initialized, and it destroys the value it was set to.
// So we must initialize __stack_pointer only *after* completing __wasm_init_tls:

// __stack_pointer = __stack_base;
global.get __stack_base
// __stack_pointer = stack_base;
local.get 3
global.set __stack_pointer

end_function
8 changes: 4 additions & 4 deletions test/code_size/hello_wasm_worker_wasm.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
"a.js.gz": 455,
"a.ww.js": 115,
"a.ww.js.gz": 127,
"a.wasm": 1894,
"a.wasm.gz": 1077,
"total": 3292,
"total_gz": 2043
"a.wasm": 1879,
"a.wasm.gz": 1066,
"total": 3277,
"total_gz": 2032
Copy link
Collaborator

@juj juj Oct 17, 2024

Choose a reason for hiding this comment

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

This is great - I'm puzzled to understand how this saved code size though..?

Copy link
Collaborator Author

@sbc100 sbc100 Oct 17, 2024

Choose a reason for hiding this comment

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

I'm also a little surprised.

My initial guess was that perhaps emscripten_stack_set_limits was being included prior to this change and so this change reduces overall duplication... but I don't think thats true. I could investigate further but but this seems like a win so I think we just take it.

Copy link
Collaborator

@juj juj Oct 17, 2024

Choose a reason for hiding this comment

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

I wonder if the earlier sizes were out of date, was there still possibility for slack %?

Copy link
Collaborator Author

@sbc100 sbc100 Oct 17, 2024

Choose a reason for hiding this comment

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

I think i double checked that. I normally rebaseline all tests before landing changes like this, just to be sure I'm seeing the real deltas.

}
2 changes: 0 additions & 2 deletions test/core/test_dlfcn_self.exports
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@ __progname_full
__sig_actions
__sig_pending
__signgam
__stack_base
__stack_chk_guard
__stack_end
__threwValue
__timezone
__tzname
Expand Down
Loading