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(nm): apply hoisting algorithm on aliased dependencies #4237

Merged
merged 3 commits into from
Mar 18, 2022

Conversation

larixer
Copy link
Member

@larixer larixer commented Mar 17, 2022

What's the problem this PR addresses?

The nm linker did not run a hoisting algorithm inside aliased packages.

Fixes: #4236

How did you fix it?

I have updated conditions inside nm hoister, so that it applied hoisting algorithm inside aliased packages.

In this PR I have also removed hoisted form: output in NM_DEBUG_LEVEL=2 since it pollutes the output and it doesn't help in any way with hoisting troubleshooting.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@larixer larixer marked this pull request as ready for review March 17, 2022 17:57
merceyz
merceyz previously approved these changes Mar 17, 2022
@larixer larixer changed the title Apply hoisting algorithm on aliased dependencies fix(nm): apply hoisting algorithm on aliased dependencies Mar 17, 2022
@larixer larixer marked this pull request as draft March 17, 2022 18:36
@larixer larixer marked this pull request as ready for review March 18, 2022 13:53
@larixer larixer requested a review from merceyz March 18, 2022 14:59
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

Would it be possible to get a test for 577f66e as well?

@larixer
Copy link
Member Author

larixer commented Mar 18, 2022

Would it be possible to get a test for 577f66e as well?

That's what I wanted to do, but unfortunately, it seems very difficult. I suspect that quite a large dependency tree is needed to showcase the necessity of this condition's presence. I can observe that this condition is needed on one of my large projects, but not all the projects need it.

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

Alright, LGTM!

@larixer larixer merged commit 16c27b6 into master Mar 18, 2022
@larixer larixer deleted the larixer/nm-aliases-hoisting branch March 18, 2022 16:00
merceyz pushed a commit that referenced this pull request May 12, 2022
* Apply hoisting algorithm on aliased dependencies

* Adds a unit test to check hoisting of the aliased packages

* Updates conditions to detect hoisting loops to account for aliases
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.

[Bug]: Suboptimal hoisting when using aliases
2 participants