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

ember-cli shares broccoli trees across builders #1112

Closed
ef4 opened this issue Feb 6, 2022 · 1 comment · Fixed by #1115
Closed

ember-cli shares broccoli trees across builders #1112

ef4 opened this issue Feb 6, 2022 · 1 comment · Fixed by #1115

Comments

@ef4
Copy link
Contributor

ef4 commented Feb 6, 2022

After @krisselden fixed cleanup of the OneShot builders in #1084, it exposed the fact that we were leaking broccoli trees across the boundary between different broccoli builders via cacheKeyForTree. I fixed that in #1100.

But now I have also discovered that ember-cli itself stores a tree in module scope:

https://github.com/ember-cli/ember-cli/blob/a7c0d3dd932fe78369fd538bb2cb00c7329bd0a6/lib/broccoli/merge-trees.js#L6

This makes ember-cli unsafe to use when more than one broccoli builder can exist in the same process, as we do with OneShot.

This is the source of the test failure in #1085 in lts_3_28-engines ubuntu. I don't fully understand the sequence of events that causes the bug to trigger under that particular set of dependencies, but you can make it go away by doing any of these things:

So it's pretty clear that we're sharing the tree between the main builder and the OneShot builders, and that's blowing up.

@ef4
Copy link
Contributor Author

ef4 commented Feb 6, 2022

I'm not sure exactly which direction to take this.

@thoov or @rwjblue are there known blockers to enabling BROCCOLI_ENABLED_MEMOIZE for everybody? If I just make embroider always turn that on, would I regret it?

@krisselden you have mentioned that the NerfHeimdal stuff is another reason to keep OneShot. What is the memory impact if we dropped OneShot and moved everything into one broccoli builder?

ef4 added a commit that referenced this issue Feb 8, 2022
This closes #1112 by removing our OneShot optimization hack in favor of turning on broccoli memoization by default.

Broccoli memoization should absolutely be broccoli's default. It's not yet, but we can turn the flag on here unless users have explicitly turned it off.
@ef4 ef4 closed this as completed in #1115 Feb 8, 2022
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 a pull request may close this issue.

1 participant