Skip to content

Conversation

@juj
Copy link
Collaborator

@juj juj commented Dec 15, 2021

Micro-optimize performance and code size generated for longjmp in Emscripten invokes to avoid string comparisons.

Something that came to mind while reviewing #15788.


_emscripten_throw_longjmp__sig: 'v',
_emscripten_throw_longjmp: function() { throw 'longjmp'; },
_emscripten_throw_longjmp: function() { throw Infinity; },
Copy link
Collaborator Author

@juj juj Dec 15, 2021

Choose a reason for hiding this comment

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

I wonder if the existence of this whole function should be gated behind

#if SUPPORT_LONGJMP == 'emscripten'

?

Though the function

void emscripten_longjmp(uintptr_t env, int val) {
  setThrew(env, val);
  _emscripten_throw_longjmp();
}

in emscripten_setjmp.c is not gated behind a #ifndef __USING_WASM_SJLJ__, which seems to be at odds against the choice with line 709 in preamble.js (#if SUPPORT_LONGJMP == 'emscripten')?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Those both seem like reasonable suggestions. Neither should save any code size benefit I guess because neither should ever be referenced in that case.. but its still good for correctness to prevent accidental references.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Great!

I hadn't realized what (e != e+0) was doing exactly until read your comments here..

@juj juj enabled auto-merge (squash) December 16, 2021 10:38
@juj juj force-pushed the micro-optimize-longjmp branch from ca0add6 to 3f37b5e Compare December 16, 2021 10:38
…cripten invokes to avoid string comparisons

Gate code that is only used for Emscripten EH and not Wasm EH behind `#if SUPPORT_LONGJMP == 'emscripten'` / `#ifndef __USING_WASM_SJLJ__`
@juj juj force-pushed the micro-optimize-longjmp branch from 4baf1fb to 929d3e5 Compare February 23, 2022 12:12
@juj juj disabled auto-merge February 23, 2022 15:17
@juj juj merged commit 133aafc into emscripten-core:main Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants