Skip to content
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

fix: use gin pageallocator for v8 allocations #27629

Closed
wants to merge 3 commits into from

Conversation

deepak1556
Copy link
Member

@deepak1556 deepak1556 commented Feb 4, 2021

Description of Change

Based on #26331

With macOS 11.2 on mac arm64, loading wasm modules crashes node process, this has been narrowed down to be an issue with the default page allocator from v8, using the partition alloc page allocator works fine and given we have already switched the default allocator in 12-x-y and above. This change helps apps on 11-x-y running on mac arm64 to load wasm modules in node processes. Also given the Paritition-alloc everywhere move https://bugs.chromium.org/p/chromium/issues/detail?id=1121427 this should align with it too.

Refs microsoft/vscode#115646

Checklist

Release Notes

Notes: fix crash with loading wasm modules in ELECTRON_RUN_AS_NODE processes on mac arm64

@deepak1556 deepak1556 requested a review from a team as a code owner February 4, 2021 23:15
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Feb 4, 2021
@deepak1556 deepak1556 requested review from MarshallOfSound and a team February 4, 2021 23:15
@deepak1556 deepak1556 added 11-x-y backport-check-skip Skip trop's backport validity checking labels Feb 4, 2021
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Feb 4, 2021
@codebytere codebytere added the semver/patch backwards-compatible bug fixes label Feb 4, 2021
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

I initially didn't backport the original PR due to it's inherent risky nature (literally changing the entire memory allocator for the main process). I'm not sure I can confidently say this is safe or it's semver/patch (it changes codesigning behavior for instance as outlined in the original PR)

I won't hold this back if everyone is on board, but I feel like I need to point out this is risky and not a simple PR behind the scenes, the impact is quite far reaching.

@deepak1556
Copy link
Member Author

deepak1556 commented Feb 4, 2021

literally changing the entire memory allocator for the main process

The allocator has an impact only for allocations from v8 and not allocations from other components in the process (which are already using partition allocator in the main process). And v8 has been using this allocator in the renderer process alongside node. Also to add this change not only impacts main process but also any node process spawned with ELECTRON_RUN_AS_NODE.

the impact is quite far reaching.

Can you expand ?

I'm not sure I can confidently say this is safe or it's semver/patch (it changes codesigning behavior for instance as outlined in the original PR)

Agree that this is not semver/patch

@deepak1556 deepak1556 added semver/minor backwards-compatible functionality and removed semver/patch backwards-compatible bug fixes semver/minor backwards-compatible functionality labels Feb 4, 2021
@MarshallOfSound
Copy link
Member

the impact is quite far reaching.

It swaps out things like the arraybuffer allocator that we give to node, which transitively impacts memory allocations which native modules have to deal with (just as a single example). How exactly this impacts certain advanced use cases has yet to be seen (because this change was only made in Electron 12). Can we make this conditional and put it behind a fuse or flag or someething? Then it would be semver/patch and opt-in?

@deepak1556
Copy link
Member Author

It swaps out things like the arraybuffer allocator that we give to node, which transitively impacts memory allocations which native modules have to deal with (just as a single example)

Don't we do that even in the renderer with nodeIntegration enabled ?

Can we make this conditional and put it behind a fuse or flag or someething? Then it would be semver/patch and opt-in?

Yup, can make this change. Will add it behind fuse.

@deepak1556
Copy link
Member Author

Fuse is not available in 11-x-y, so added it behind buildflag

@deepak1556 deepak1556 added the semver/patch backwards-compatible bug fixes label Feb 5, 2021
@deepak1556
Copy link
Member Author

Closing in favor of #27684

@deepak1556 deepak1556 closed this Feb 10, 2021
@deepak1556 deepak1556 deleted the robo/fix_node_allocator_patch branch February 10, 2021 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
11-x-y backport-check-skip Skip trop's backport validity checking semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants