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

Prevent redundant toTree wrapping for macros #1213

Merged

Conversation

raycohen
Copy link
Contributor

@raycohen raycohen commented Jun 3, 2022

In an app with many addons, we are seeing this wrapping get applied a significant number of times, eventually leading to "Maximum call stack size exceeded" errors when toTree is finally executed.

When I copy the stack trace out of visual code just before the "Maximum call stack size exceeded" error, there are 7364 lines in the stack that match node_modules/@embroider/macros/src/ember-addon-main.js:56

I think this wrapper only needs to be applied once per app instance. Is there something else I should be doing to prevent @embroider/macros included method from getting hit so much?

Screen Shot 2022-06-03 at 12 15 34 PM

Screen Shot 2022-06-03 at 12 19 33 PM

@raycohen raycohen marked this pull request as ready for review June 3, 2022 16:25
@simonihmig simonihmig added the bug Something isn't working label Jun 3, 2022
Copy link
Collaborator

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

LGTM!

Is there something else I should be doing to prevent @embroider/macros included method from getting hit so much?

No, I don't think so. You are getting called (not literally you of course 😉), by EmberCLI, so there is nothing to change that on this side. However I am quite puzzled that include() is called that much! (>7000x 😱)

@ef4
Copy link
Contributor

ef4 commented Jun 3, 2022

I've seen some pretty crazy things like that in big apps. ember-cli creates separate addon instances per consumer.

If you have an addon that uses embroider/macros, and that addon is used by 10 other addons, and those addons are each used by 10 other addons, and those addons are used by 10 other addons, embroider/macros will get instantiated 1000 times.

@raycohen
Copy link
Contributor Author

raycohen commented Jun 3, 2022

ember-cli creates separate addon instances per consumer.

Yeah that seems to be the situation I'm seeing. I've got ~175 addons recognized by the project, but the recursive included calls from

https://github.com/ember-cli/ember-cli/blob/1519391eb562c446123e6b63fa400ed7b9e6b77a/lib/models/addon.js#L769

blow things up. We've got 37 addons that declare a dependency on ember-classic-decorator which depends on @embroider/macros

@rwjblue
Copy link
Collaborator

rwjblue commented Jun 14, 2022

This will still happen once per @embroider/macros version, right (because the module scoped boolean or set would be per-version)? Unless there is some sort of highlander logic going on (which I don't recall having here),

@ef4
Copy link
Contributor

ef4 commented Jun 14, 2022

I think that's correct, but that's probably not enough to blow up the stack limit.

@ef4 ef4 merged commit 68d1768 into embroider-build:main Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants