Skip to content

Avoid checks before _free calls. NFC #24416

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
May 28, 2025
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented May 27, 2025

Free is defined to do nothing if NULL is passed so its harmless to just call it.

Also, make more use of _realloc.

Both these changes save on code size.

info.bufferSize = data.length;
info.buffer = _malloc(data.length);
}
info.buffer = _realloc(info.buffer, data.length);
Copy link
Member

Choose a reason for hiding this comment

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

Does realloc do the equivalent check that the buffer size is < the desired length? (i.e. does it avoid the actual reallocation in the case where the existing buffer is big enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, thats exactly what it does

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think in practice it also never shrinks, but I think that is fine for all these cases.

@sbc100 sbc100 force-pushed the realloc branch 2 times, most recently from a2398ba to 1fe4c5c Compare May 27, 2025 23:02
@@ -269,7 +269,7 @@ var LibraryStackTrace = {
} else {
name = wasmOffsetConverter.getName(pc);
}
if (_emscripten_pc_get_function.ret) _free(_emscripten_pc_get_function.ret);
_free(_emscripten_pc_get_function.ret | 0);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Collaborator Author

@sbc100 sbc100 May 27, 2025

Choose a reason for hiding this comment

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

Because in wasm64 we cannot pass undefined in place 0/NULL :(

It generates TypeError: Cannot convert undefined to a BigInt

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, right... I keep forgetting about BigInts there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But it allows 0? Will it convert anything that Number.isSafeInteger() returns true for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have wrappers that convert pointer argument to bigint automatically by doing BigInt(arg) for each of the pointer arguments. This does indeed work for all numbers, including zero, but it does not work for undefined or null.

 node
Welcome to Node.js v20.19.2.
Type ".help" for more information.
> BigInt(0)
0n
> BigInt(undefined)
Uncaught TypeError: Cannot convert undefined to a BigInt
    at BigInt (<anonymous>)
> 

We don't really want to increase the size of all wrappers by handling this case (I think).

Free is defined to do nothing if NULL is passed so its harmless to just
call it.

Also, make more use of `_realloc`.

Both these changes save on code size.
@sbc100 sbc100 merged commit 45da5f0 into emscripten-core:main May 28, 2025
28 of 30 checks passed
@sbc100 sbc100 deleted the realloc branch May 28, 2025 17:44
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.

4 participants