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.

Sign up for GitHub

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

Jump to bottom

feat(turbopack): Implement module walking for side effect optimization #71241

Merged
merged 45 commits into from
Nov 19, 2024
Merged

feat(turbopack): Implement module walking for side effect optimization #71241

merged 45 commits into from
Nov 19, 2024

Conversation

kdy1
Copy link
Member

@kdy1 kdy1 commented Oct 14, 2024 •
edited by padmaia
Loading

What?

Implement side effect optimization without creating a new ModulePart. This PR is partial because this PR does not handle the exclusion of non-existent module perfectly. #72947 will do it.

Why?

#70336 (comment)

How?

Closes PACK-3494

@ijjk ijjk added the created-by: Turbopack team PRs by the Turbopack team. label Oct 14, 2024
This was referenced Oct 14, 2024
Merged
Merged
@kdy1 Graphite App
Copy link
Member Author

kdy1 commented Oct 14, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

This was referenced Oct 14, 2024
Merged
Merged
@kdy1 kdy1 changed the title Prepare select_part feat(turbopack): Implement side effect optimization Oct 14, 2024
@ijjk
Copy link
Member

ijjk commented Oct 14, 2024 •
edited
Loading

Tests Passed

@kdy1 kdy1 force-pushed the kdy1/ts-reexport-all branch 2 times, most recently from a632653 to b44dd1e Compare October 14, 2024 09:38
@kdy1 kdy1 changed the base branch from kdy1/ts-opt-module-eval to kdy1/ts-analysis-part October 14, 2024 09:38
@kdy1 kdy1 mentioned this pull request Oct 14, 2024
Merged
@kdy1 kdy1 force-pushed the kdy1/ts-reexport-all branch from b44dd1e to 5fc8e32 Compare October 14, 2024 09:41
@kdy1 kdy1 force-pushed the kdy1/ts-analysis-part branch from b279bb5 to 9e7ae8c Compare October 14, 2024 11:02
@kdy1 kdy1 force-pushed the kdy1/ts-reexport-all branch from ee4d33a to 6156178 Compare October 14, 2024 11:02
@kdy1 kdy1 force-pushed the kdy1/ts-analysis-part branch from 9e7ae8c to dafce19 Compare October 14, 2024 11:36
@kdy1 kdy1 force-pushed the kdy1/ts-reexport-all branch from 6156178 to 1b6f93a Compare October 14, 2024 11:36
@kdy1 kdy1 force-pushed the kdy1/ts-analysis-part branch from dafce19 to 006967f Compare October 14, 2024 11:38
@kdy1 kdy1 force-pushed the kdy1/ts-reexport-all branch from 1b6f93a to 4fd4165 Compare October 14, 2024 11:38
@kdy1 kdy1 force-pushed the kdy1/ts-analysis-part branch from 006967f to 6201752 Compare October 14, 2024 12:33
@kdy1 kdy1 force-pushed the kdy1/ts-reexport-all branch from 4fd4165 to ac78938 Compare October 14, 2024 12:36
@kdy1 kdy1 force-pushed the kdy1/ts-analysis-part branch from 6201752 to 861424a Compare October 14, 2024 13:35
@kdy1 kdy1 force-pushed the kdy1/ts-reexport-all branch from ac78938 to 54f24bb Compare October 14, 2024 13:38
Base automatically changed from kdy1/ts-analysis-part to canary October 14, 2024 14:28
@kdy1 kdy1 force-pushed the kdy1/ts-reexport-all branch from 54f24bb to d327fef Compare October 14, 2024 14:30
@kdy1 Graphite App
Copy link
Member Author

kdy1 commented Oct 14, 2024

Merge activity

  • Oct 15, 12:02 AM GMT+9: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..

@kdy1 kdy1 force-pushed the kdy1/ts-reexport-all branch from 6535bf7 to a9a4f2e Compare October 16, 2024 02:11
@kdy1 kdy1 changed the base branch from canary to kdy1/ts-follow-exports October 16, 2024 02:12
kdy1
kdy1 commented Nov 18, 2024
View reviewed changes
turbopack/crates/turbopack-ecmascript/src/references/mod.rs Outdated
@@ -607,7 +612,9 @@ pub(crate) async fn analyse_ecmascript_module_internal(
}
ImportedSymbol::Symbol(name) => Some(ModulePart::export((&**name).into())),
ImportedSymbol::PartEvaluation(part_id) => {
evaluation_references.push(i);
if !is_side_effect_free {
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this change, all next.js tests pass (CI) but the side-effect-optimization unit test fails because of non-existing.js.

@kdy1 kdy1 requested a review from sokra November 18, 2024 03:11
@kdy1 kdy1 marked this pull request as ready for review November 18, 2024 03:11
@kdy1 kdy1 marked this pull request as draft November 18, 2024 03:38
@kdy1 kdy1 marked this pull request as ready for review November 18, 2024 03:49
@kdy1 kdy1 marked this pull request as draft November 18, 2024 04:18
@kdy1
Copy link
Member Author

kdy1 commented Nov 18, 2024

Investigation of CI failure:

# Module 2
var TracesSamplerValues;
export { TracesSamplerValues as a };

# Module 3
import { a as TracesSamplerValues } from "__TURBOPACK_PART__" with {
    __turbopack_part__: -2
};
(function(TracesSamplerValues) {
    TracesSamplerValues["AlwaysOff"] = "always_off";
    TracesSamplerValues["AlwaysOn"] = "always_on";
    TracesSamplerValues["ParentBasedAlwaysOff"] = "parentbased_always_off";
    TracesSamplerValues["ParentBasedAlwaysOn"] = "parentbased_always_on";
    TracesSamplerValues["ParentBasedTraceIdRatio"] = "parentbased_traceidratio";
    TracesSamplerValues["TraceIdRatio"] = "traceidratio";
})(TracesSamplerValues || (TracesSamplerValues = {}));

Module 3 is skipped because it was treated as a side effect.

@kdy1 kdy1 marked this pull request as ready for review November 18, 2024 07:05
sokra
sokra requested changes Nov 18, 2024
View reviewed changes
Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

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

enable the test case for the side effects optimization

@kdy1 kdy1 force-pushed the kdy1/ts-reexport-all branch 2 times, most recently from 9a209c3 to a4ee926 Compare November 19, 2024 07:25
@kdy1 kdy1 changed the title feat(turbopack): Implement side effect optimization feat(turbopack): Implement side effect optimization partially Nov 19, 2024
@kdy1 kdy1 mentioned this pull request Nov 19, 2024
Draft
@kdy1 kdy1 requested a review from sokra November 19, 2024 08:01
kdy1
kdy1 commented Nov 19, 2024
View reviewed changes
Copy link
Member Author

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

@sokra I extracted changes required to enable the tree shaking cargo test case to #72947

@kdy1 kdy1 changed the title feat(turbopack): Implement side effect optimization partially feat(turbopack): Implement module walking for side effect optimization Nov 19, 2024
@kdy1 kdy1 merged commit a8f1c70 into canary Nov 19, 2024
215 checks passed
@kdy1 kdy1 deleted the kdy1/ts-reexport-all branch November 19, 2024 09:53
wyattjoh pushed a commit that referenced this pull request Nov 28, 2024
@kdy1 @wyattjoh
feat(turbopack): Implement module walking for side effect optimization (
9a5a199 
#71241)

### What?

Implement side effect optimization without creating a new `ModulePart`. This PR is `partial` because this PR does not handle the exclusion of a non-existent module perfectly. #72947 will do it.

### Why?

#70336 (comment)
@github-actions github-actions bot added the locked label Dec 3, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sokra sokra sokra approved these changes

Assignees

@kdy1 kdy1

Labels
CI Bypass Graphite Optimization Ignore Graphite CI optimizations, run the full CI suite. https://graphite.dev/docs/stacking-and-ci created-by: Turbopack team PRs by the Turbopack team. locked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kdy1 @ijjk @sokra