Reduce memory pressure from compat layer by disabling Heimdall node gathering during OneShotPlugin #915
Conversation
rwjblue
force-pushed
the
disable-heimdall-gathering-for-one-shots
branch
2 times, most recently
from
6bf3dc4
to
6ee6651
Compare
rwjblue
changed the title
rwjblue
changed the title
| declare module 'heimdalljs' { | ||
| export default any | ||
| } |
There was a problem hiding this comment.
Yes, this is bad and I feel bad. We should fix the types publishing of heimdalljs@0.2.x if we actually want good types here.
I'm not sure I want to shave that particular yak at the moment...
There was a problem hiding this comment.
I mean, the existence of OneShot also seems bad and I feel bad. 😅
Do you have any sense for whether BROCOLLI_ENABLED_MEMOIZE is going to get widely enabled? It would let us drop OneShot.
We could also profile how much OneShot is really saving us on rebuilds. I think it mattered when I added it, but a lot has changed since then.
There was a problem hiding this comment.
Do you have any sense for whether BROCOLLI_ENABLED_MEMOIZE is going to get widely enabled? It would let us drop OneShot.
The main problem is that we need to provide a reasonable escape hatch that isn't "restart the server". @stefanpenner and I have chatted about a few options, I think that the best one was to add a way to trigger a full rebuild (e.g. ignoring the memoization for that specific rebuild). The two ways we thought of doing that were something like touch tmp/perform-full-rebuild (just touch some file that we could monitor) or sending a signal to the running process (e.g. SIGUSR1) that would force it to full rebuild.
We could also profile how much OneShot is really saving us on rebuilds. I think it mattered when I added it, but a lot has changed since then.
I suspect it still saves us quite a bit of time, and will until we start moving the needle on usage of @embroider/addon-shim. But you are totally right, we should profile to see impact (both memory and build time for full builds and rebuilds).
rwjblue
force-pushed
the
disable-heimdall-gathering-for-one-shots
branch
2 times, most recently
from
d9b6c44
to
1f7916b
Compare
| current Broccoli builder's heimdall node graph (e.g. the one that is calling | ||
| OneShotPlugin; **not** the one that the OneShotPlugin internally creates). | ||
| */ | ||
| oneshotHeimdallNode.remove(); |
There was a problem hiding this comment.
we should allow you to disable this based on addon name if you want to see detail
| // Make a heimdall node so that we know for sure, all nodes created during our | ||
| // inner builder can be remove easily | ||
| const oneshotCookie = heimdall.start({ | ||
| name: `@embroider/compat: OneShot (${this.addonName})`, |
There was a problem hiding this comment.
OneShot will already be the name of this broccoli-plugin this will look like OneShot > @embroider/compat: OneShot addonName
This reduces the amount of memory that these one-shot's create by:
- Avoiding creating Heimdall nodes for each broccoli plugin
- Disabling the "re-parenting" process done by Broccoli builder
(which ends up creating **double** the heimdall nodes)
- For manually created heimdall nodes (e.g. ones made by individual
broccoli plugins) remove any child nodes added during the one-shot
process
---
Additionally, this makes broccoli instrumentation output quite a bit
nicer for these `OneShot` instances (adding the addon name as an
annotation). Now (with these changes) debugging _which_ OneShot is
particularly slow, is much easier...
Co-authored-by: Kris Selden <kris.selden@gmail.com>
rwjblue
force-pushed
the
disable-heimdall-gathering-for-one-shots
branch
from
1f7916b
to
6db5bc7
Compare
This reduces the amount of memory that these one-shot's create by:
Additionally, this makes broccoli instrumentation output quite a bit nicer for these
OneShotinstances (adding the addon name as an annotation). Now (with these changes) debugging which OneShot is particularly slow, is much easier...