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

chore: bump oxc to v0.21.0 #1656

Merged
merged 11 commits into from
Jul 18, 2024
Merged

chore: bump oxc to v0.21.0 #1656

merged 11 commits into from
Jul 18, 2024

Conversation

Dunqing
Copy link
Member

@Dunqing Dunqing commented Jul 18, 2024

Description

The changelog is here. Most of the snapshots changed due to codegen printing double quotes by default.

Also, in this version, we have aligned the ts ast scope with TypeScript. Many ignored ts tests may have been resolved.

Copy link

netlify bot commented Jul 18, 2024

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 2ae1567
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/6698cf6c8ff1190008c7f1ad

Copy link

github-actions bot commented Jul 18, 2024

Benchmarks Rust

group                                                        pr                                     target
-----                                                        --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol              1.00     47.5±0.61ms        ? ?/sec    1.05     49.7±1.19ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap    1.00     58.0±1.20ms        ? ?/sec    1.03     59.9±0.99ms        ? ?/sec
bundle/bundle@rome-ts                                        1.00     91.7±1.09ms        ? ?/sec    1.03     94.8±1.17ms        ? ?/sec
bundle/bundle@rome-ts-sourcemap                              1.00    110.8±2.66ms        ? ?/sec    1.02    113.0±1.27ms        ? ?/sec
bundle/bundle@threejs                                        1.00     27.3±0.60ms        ? ?/sec    1.01     27.6±0.82ms        ? ?/sec
bundle/bundle@threejs-sourcemap                              1.01     37.1±0.67ms        ? ?/sec    1.00     36.7±0.72ms        ? ?/sec
bundle/bundle@threejs10x                                     1.00    306.3±5.97ms        ? ?/sec    1.01    307.9±3.87ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                           1.00    396.7±6.59ms        ? ?/sec    1.00    396.1±6.00ms        ? ?/sec
remapping/remapping                                          1.02     50.0±0.53ms        ? ?/sec    1.00     49.2±0.14ms        ? ?/sec
remapping/render-chunk-remapping                             1.06    116.5±0.39ms        ? ?/sec    1.00    109.9±0.78ms        ? ?/sec
scan/scan@rome-ts                                            1.00     72.1±0.78ms        ? ?/sec    1.07     77.3±0.79ms        ? ?/sec
scan/scan@rome-ts-sourcemap                                  1.00     73.4±0.63ms        ? ?/sec    1.05     77.2±1.08ms        ? ?/sec
scan/scan@threejs                                            1.00     20.4±0.31ms        ? ?/sec    1.01     20.6±0.46ms        ? ?/sec
scan/scan@threejs-sourcemap                                  1.00     19.9±0.18ms        ? ?/sec    1.05     20.9±0.25ms        ? ?/sec
scan/scan@threejs10x                                         1.00    204.5±1.98ms        ? ?/sec    1.04    212.3±3.09ms        ? ?/sec
scan/scan@threejs10x-sourcemap                               1.00    203.6±1.79ms        ? ?/sec    1.04    212.7±2.13ms        ? ?/sec

Copy link

codspeed-hq bot commented Jul 18, 2024

CodSpeed Performance Report

Merging #1656 will improve performances by 6.1%

Comparing bump/oxc-0.21.0 (2ae1567) with main (68ee262)

Summary

⚡ 3 improvements
✅ 13 untouched benchmarks

Benchmarks breakdown

Benchmark main bump/oxc-0.21.0 Change
bundle@rome-ts 499.8 ms 475.9 ms +5.02%
scan@rome-ts 415.4 ms 391.6 ms +6.1%
scan@rome-ts-sourcemap 414.9 ms 391.9 ms +5.86%

c()?.foo(x);
d().foo?.(x);
e()?.foo?.(x);
(a()).foo;
Copy link
Contributor

Choose a reason for hiding this comment

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

bad case.

@IWANABETHATGUY
Copy link
Contributor

Overall it looks good except some bad cases in call expr chain and ObjectExpr with shorthand, could you track all the bad cases upstream?

IWANABETHATGUY
IWANABETHATGUY previously approved these changes Jul 18, 2024
Cargo.toml Outdated Show resolved Hide resolved
@Dunqing
Copy link
Member Author

Dunqing commented Jul 18, 2024

The node tests are difficult to find out why it fails, and I'm wondering why we don't print out the Vitest error.

@hyf0
Copy link
Member

hyf0 commented Jul 18, 2024

The node tests are difficult to find out why it fails, and I'm wondering why we don't print out the Vitest error.

Looks like tests on vitest are all passed. It's just one failed in rollup tests: https://github.com/rolldown/rolldown/actions/runs/9985933904/job/27597619752?pr=1656.

@hyf0 hyf0 dismissed IWANABETHATGUY’s stale review July 18, 2024 05:23

No git URL on cargo.toml

@Dunqing
Copy link
Member Author

Dunqing commented Jul 18, 2024

@hyf0
Copy link
Member

hyf0 commented Jul 18, 2024

Looks like tests on vitest are all passed. It's just one failed in rollup tests: https://github.com/rolldown/rolldown/actions/runs/9985933904/job/27597619752?pr=1656.

Please see https://github.com/rolldown/rolldown/actions/runs/9985756953/job/27597155926

Error: Failed in ./fixtures/output/hashFileNames/_config.ts

Run just test-node rolldown --update for this one.

Failed in ./fixtures/tree-shake/module-side-effects-regex/_config.ts

This one needs to be investigate. I'll take look now.

@Dunqing
Copy link
Member Author

Dunqing commented Jul 18, 2024

Error: Failed in ./fixtures/output/hashFileNames/_config.ts

Run just test-node rolldown --update for this one.

Failed in ./fixtures/tree-shake/module-side-effects-regex/_config.ts

This one needs to be investigate. I'll take look now.

I already fixed it. 51701d3

@hyf0
Copy link
Member

hyf0 commented Jul 18, 2024

I found the cause of the failed rollup test. Not sure how to describle it.

@Dunqing
Copy link
Member Author

Dunqing commented Jul 18, 2024

I found the cause of the failed rollup test. Not sure how to describle it.

Could you tell me if it is caused by oxc?

@hyf0
Copy link
Member

hyf0 commented Jul 18, 2024

let o = {
	f() {
		assert.ok(this !== o);
	}
};
(1, o.f)();
(1, o.f)``;

(true && o.f)();
(true && o.f)``;

(true ? o.f : false)();
(true ? o.f : false)``;

is bundled to

let o = {f() {
	assert.ok(this !== o);
}};
(1, o.f)();
(1, o.f)``;
o.f();
o.f``;
o.f();
o.f``;
o.f``;

is not the same as

(true && o.f)``;

We used

RemoveDeadCode::new(fields.allocator).build(fields.program);

to simplify expressions. I guess this will cause

(true && o.f)``;

become

(o.f)``;

which is still correct and has the same semantic as edited: this is also wrong, see the following output of esbuild

(1, o.f)``;

.

But this PR oxc finally printed to

o.f``;

which is wrong. Not sure if we should fix it in the RemoveDeadCode or this is a bug of oxc_codegen.


See the output of esbuld

let o={f(){assert.ok(this!==o)}};(0,o.f)(),(0,o.f)``,(0,o.f)(),(0,o.f)``,(0,o.f)(),(0,o.f)``;

https://esbuild.github.io/try/#dAAwLjIzLjAALS1taW5pZnkAbGV0IG8gPSB7CglmKCkgewoJCWFzc2VydC5vayh0aGlzICE9PSBvKTsKCX0KfTsKKDEsIG8uZikoKTsKKDEsIG8uZilgYDsKCih0cnVlICYmIG8uZikoKTsKKHRydWUgJiYgby5mKWBgOwoKKHRydWUgPyBvLmYgOiBmYWxzZSkoKTsKKHRydWUgPyBvLmYgOiBmYWxzZSlgYDs

@hyf0
Copy link
Member

hyf0 commented Jul 18, 2024

This is the main branch output

let o = {f() {
	assert.ok(this !== o);
}};
(1, o.f)();
(1, o.f)``;
(true && o.f)();
(true && o.f)``;
(true ? o.f : false)();
(true ? o.f : false)``;

So it seems we passed this rollup test because we didn't support simplify these expressions before. So they keep the semantics.

@hyf0
Copy link
Member

hyf0 commented Jul 18, 2024

This is a classic one. Esbuild has the issue before. evanw/esbuild#2610

@Dunqing
Copy link
Member Author

Dunqing commented Jul 18, 2024

From the description, it looks like the bug comes from RemoveDeadCode.

This is a classic one. Esbuild has the issue before. evanw/esbuild#2610

Haha, The issue author is also you.

Let's fix it in the next patch

@hyf0 hyf0 enabled auto-merge July 18, 2024 08:21
Copy link
Member

@hyf0 hyf0 left a comment

Choose a reason for hiding this comment

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

Nice!

@hyf0 hyf0 added this pull request to the merge queue Jul 18, 2024
Merged via the queue into main with commit fc5d0f8 Jul 18, 2024
22 checks passed
@hyf0 hyf0 deleted the bump/oxc-0.21.0 branch July 18, 2024 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants