Skip to content

Remove VM module - #1619

Draft
mauricioszabo wants to merge 3 commits into
pulsar-edit:masterfrom
mauricioszabo:remove-vm-module
Draft

mauricioszabo wants to merge 3 commits into
pulsar-edit:masterfrom
mauricioszabo:remove-vm-module

Conversation

@mauricioszabo

Copy link
Copy Markdown
Contributor

Before moving with this PR, some explanations:

I found that the usage of vm in the renderer can be the cause that makes Pulsar crash sometimes, especially in newer Electron versions (like now that we're on 30-something).

I also tried some benchmarks (on Linux only, because it's the only machine I have available) and found no difference in having the "native cache" toggled on or off - I believe that this code, being present since the early days of Atom, existed because the original require was too slow.

So I imagine that removing that won't cause any problem; still, I would love if someone with a Windows machine (and somebody with a Mac one) could test to see if they found any difference in loading performance (native cache should affect only the first load of Pulsar)

@savetheclocktower

Copy link
Copy Markdown
Contributor

@mauricioszabo, against what version of Electron have you been testing this change? I know you usually run on the bleeding edge.

I ask because Claude thinks that the time could soon be right to remove the native compile cache, but not quite yet:

Node itself gained a built-in, very similar feature — the module.enableCompileCache() / NODE_COMPILE_CACHE API, added in Node 22.1 (and Electron ships whatever Node version it's built against). That does essentially the same job (persist V8 code cache across runs, keyed by V8 version) but is maintained upstream instead of via monkey-patching Module.prototype._compile. Whether it's usable here depends entirely on which Electron/Node version Pulsar currently targets — worth checking electron's pinned version in package.json against when NODE_COMPILE_CACHE landed before concluding this file is redundant.

It then checked for me and saw I was on Electron 32.3.3 locally (having bumped it in #1600 and still testing it locally) and continued:

Electron 32 bundles Node 20.18, and NODE_COMPILE_CACHE/module.enableCompileCache() didn't land until Node 22.1. So on your current Electron version, the built-in Node API isn't available yet — native-compile-cache.js is still doing work Node can't do for you. Unlike module-cache.js, I wouldn't call this one a good obsolescence candidate right now; that would change if/when Pulsar moves to an Electron build with Node ≥22.1.

So if I took that at face value, this would be a promising PR that might not quite be ripe for landing. But if you've been running it against Electron 30 or 32 with no observed performance difference, I want to take that seriously as well!

@savetheclocktower

Copy link
Copy Markdown
Contributor

(Also, I did briefly get this mixed up with src/module-cache.js when discussing this PR on Discord. That's another ocean-boiling system that we might just be able to live without, but I'll save that battle for another PR!)

@mauricioszabo

Copy link
Copy Markdown
Contributor Author

That's a good catch, I did run in Electron 41.7.1.

But - this warning is happening since our old Electron 12, so I don't even think that vm will be removed soon (or maybe at all). I only wanted to take a swing on that because I saw some places mentioning that its usage does cause some crashes... so I wanted to see if that was true (especially considering that this could be less code for us to keep, and if it's not causing any speedup on loading the editor, then it's just "making things more unstable and complicated for no reason").

But, to be honest, I opened up the PR so that it's simpler for others to download a version without the cache, so they could benchmark things :)

@savetheclocktower

Copy link
Copy Markdown
Contributor

Yeah, I'm not so much worried about the vm module being removed. If it were removed, we know we could disable the native compile cache and still have a working Pulsar, even if it affected performance.

And I agree — I would only want to keep the native compile cache around if it actually improved performance by a measurable amount over not having it. Once we bump to Electron 35 we should be able to compare it to the version of the cache implemented in Node itself; I'd expect Node's version to be superior, but even if it was just equivalent or slightly worse I'd still want to use it over ours.

@savetheclocktower savetheclocktower added this to the 1.134.0 milestone Sep 10, 2026
@savetheclocktower

Copy link
Copy Markdown
Contributor

Giving this the 1.134.0 milestone so I remember to revisit it. Even for 1.134.0 it might be premature, but if so we can just kick it to the next milestone.

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.

2 participants