fix(es/minifier): calc function body cost when inline function #5342

Austaras · 2022-07-30T09:28:19Z

Description:

A follow up of #5253.

BREAKING CHANGE:

Related issue (if exists):

crates/swc_ecma_minifier/tests/terser/compress/functions/issue_2783/output.js

crates/swc_ecma_minifier/src/compress/optimize/inline.rs

crates/swc_ecma_minifier/tests/fixture/issues/moment/1/output.js

kdy1

Seems like lots of unrelated changes are in this PR.
Can you rebase again to drop them?

kdy1 · 2022-08-03T03:52:26Z

Hmm... I'll try vscode extension

kdy1

I think we should count variables or fix #5367 before merging this.
I can work on it if you prefer it

...fier/tests/fixture/next/feedback-util-promisify/chunks/pages/_app-72ad41192608e93a/output.js

..._ecma_minifier/tests/fixture/next/33265/static/chunks/pages/index-cb36c1bf7f830e3c/output.js

kdy1 · 2022-08-03T07:35:17Z

I reverted some rust-debug changes because I couldn't see actually changed files.
I think we can split the PR into analyzer fix and this change, if you want

Austaras · 2022-08-03T07:35:38Z

I could start working on #5367 this evening or tomorrow

Austaras · 2022-08-03T07:36:57Z

I reverted some rust-debug changes because I couldn't see actually changed files. I think we can split the PR into analyzer fix and this change, if you want

Yes, to fix #5367 need to tinker anaylzer anyway..

Austaras · 2022-08-04T10:17:24Z

Why does minifier has a seperate trait Storage

kdy1 · 2022-08-04T10:20:04Z

I made it because I wanted to create more cheaper analyzers without repeating work for ctx

I think it's fine to remove it.

kdy1 · 2022-08-05T14:18:29Z

Will this fix #5404?
I'm asking just because I'm not sure

Austaras · 2022-08-05T15:17:01Z

Will this fix #5404? I'm asking just because I'm not sure

Unlikely. #5404 is because inline not strong enough. Terser can neither properly inline it, only gcc advanced mode would.

Austaras · 2022-09-29T05:56:37Z

All obstacles blocking this have been removed. I could restart this PR this weekend.

kdy1 · 2022-10-13T01:06:32Z

Main:

File: ./benches/full/antd.js
  458111
File: ./benches/full/d3.js
   87613
File: ./benches/full/echarts.js
  321142
File: ./benches/full/jquery.js
   30902
File: ./benches/full/lodash.js
   25119
File: ./benches/full/moment.js
   18626
File: ./benches/full/react.js
    8228
File: ./benches/full/terser.js
  122427
File: ./benches/full/three.js
  158421
File: ./benches/full/typescript.js
  813037
File: ./benches/full/victory.js
  158156
File: ./benches/full/vue.js
   42810

This PR:

File: ./benches/full/antd.js
  458110
File: ./benches/full/d3.js
   87635
File: ./benches/full/echarts.js
  321171
File: ./benches/full/jquery.js
   30902
File: ./benches/full/lodash.js
   25120
File: ./benches/full/moment.js
   18630
File: ./benches/full/react.js
    8228
File: ./benches/full/terser.js
  122427
File: ./benches/full/three.js
  158414
File: ./benches/full/typescript.js
  813366
File: ./benches/full/victory.js
  158152
File: ./benches/full/vue.js
   42796

Austaras · 2022-10-13T07:17:33Z

Regression in moment is because we inline big number literal which is fine, and I haven't investigated Typescipt because of its size.

All other regerssions can be run down to a single issue

export function foo(a) {
  return bar(a) + bar(a)
}

function bar(a) {
   return a + 1
}

where simple functions(in this case, bar) is declared under usage, so in first pass it's only inlined by Finalizer but not invoked.

kdy1 · 2022-10-13T08:28:06Z

Oh... So we need one more pass.

kdy1 · 2022-10-13T08:34:51Z

I'll create a PR to change default pass limit to 3

Austaras · 2022-10-13T08:36:21Z

Maybe not an extra physical pass but something like Finalizer

kdy1 · 2022-10-13T08:41:33Z

I think it's now fine to run a full pass again so I made #6138

kdy1 · 2022-10-13T23:49:27Z

Now #6138 is merged. Can you rebase?

Austaras · 2022-10-14T08:03:05Z

Sure. I will rebase this and test it tonight.

Austaras · 2022-10-14T13:57:49Z

Three passes
main

File: ./benches/full/antd.js
459448
File: ./benches/full/d3.js
87351
File: ./benches/full/echarts.js
319813
File: ./benches/full/jquery.js
30766
File: ./benches/full/lodash.js
24907
File: ./benches/full/moment.js
18604
File: ./benches/full/react.js
8230
File: ./benches/full/terser.js
122430
File: ./benches/full/three.js
158065
File: ./benches/full/typescript.js
807256
File: ./benches/full/victory.js
157789
File: ./benches/full/vue.js
42429

inline

File: ./benches/full/antd.js
459422
File: ./benches/full/d3.js
87349
File: ./benches/full/echarts.js
319769
File: ./benches/full/jquery.js
30766
File: ./benches/full/lodash.js
24888
File: ./benches/full/moment.js
18606
File: ./benches/full/react.js
8230
File: ./benches/full/terser.js
122430
File: ./benches/full/three.js
158065
File: ./benches/full/typescript.js
807284
File: ./benches/full/victory.js
157785
File: ./benches/full/vue.js
42416

The only regression is in moment and typescript

kdy1 · 2022-10-14T14:43:44Z

crates/swc_ecma_minifier/tests/fixture/next/syncfusion/933-e9f9a6bf671b96fc/output.js

                            break;
                        case 'bottomleft':
-                            setPosx(getElementLeft(), pos), setPosy(getElementBottom(), pos);
+                            setPosx(getElementLeft(), pos), value7 = getElementBottom(), pos.top = value7;


This looks like a regresison. Are we considering the size of the member property correctly?

We count pos.top = value as size 7, as both pos and value can be mangled, and function call cost is 3 + param_count * 2 + 14 / callee_count larger than 7, so it got inlined. This regression is caused by value cannot be inlined, as pos.top may have side effect. However we cannot know if an argument has side effect at the time of inlining since we only know it's declaration.

jridgewell · 2022-10-15T03:44:12Z

crates/swc_ecma_minifier/src/compress/optimize/inline.rs

+        usage: &VarUsageInfo,
+    ) -> bool {
+        let param_cost = param_count * 2;
+        // if it's passed as value but not called, the function expr cannot be removed


If this is the case, couldn't we just return false and skip the work?

function foo() { g() } foo() module.exports = foo

In this case foo cannot be removed, but still can be inlined

kdy1

I'm still looking at changes, so this is just a bump comment

swc-bump:

swc_ecma_minifier

swc-bot

Automated review comment generated by auto-rebase script

Austaras force-pushed the inline branch from fc8c9b6 to 00eb921 Compare July 30, 2022 09:34

Austaras commented Jul 30, 2022

View reviewed changes

crates/swc_ecma_minifier/tests/terser/compress/functions/issue_2783/output.js Outdated Show resolved Hide resolved

Austaras commented Jul 30, 2022

View reviewed changes

crates/swc_ecma_minifier/src/compress/optimize/inline.rs Show resolved Hide resolved

Austaras mentioned this pull request Jul 30, 2022

Minifier would generate invalid lhs #5343

Closed

kdy1 added this to the Planned milestone Aug 1, 2022

Austaras force-pushed the inline branch 2 times, most recently from 7f2fda0 to 9252b01 Compare August 1, 2022 09:04

Austaras marked this pull request as ready for review August 1, 2022 11:49

kdy1 reviewed Aug 2, 2022

View reviewed changes

crates/swc_ecma_minifier/tests/fixture/issues/moment/1/output.js Outdated Show resolved Hide resolved

Austaras mentioned this pull request Aug 2, 2022

fix(es/minifier): workaround for inline #5365

Merged

Austaras force-pushed the inline branch from 9e52fd5 to 083dd3d Compare August 2, 2022 16:36

kdy1 requested changes Aug 3, 2022

View reviewed changes

Austaras force-pushed the inline branch from 083dd3d to 3eae986 Compare August 3, 2022 03:46

Austaras requested a review from kdy1 August 3, 2022 03:47

Austaras force-pushed the inline branch from 3eae986 to f020bcb Compare August 3, 2022 03:47

kdy1 requested changes Aug 3, 2022

View reviewed changes

...fier/tests/fixture/next/feedback-util-promisify/chunks/pages/_app-72ad41192608e93a/output.js Outdated Show resolved Hide resolved

..._ecma_minifier/tests/fixture/next/33265/static/chunks/pages/index-cb36c1bf7f830e3c/output.js Outdated Show resolved Hide resolved

Austaras force-pushed the inline branch from 3b37f28 to 3b87a3a Compare August 6, 2022 05:04

Austaras marked this pull request as draft August 6, 2022 05:04

Austaras force-pushed the inline branch 2 times, most recently from 1d85d96 to a093045 Compare October 2, 2022 10:24

kdy1 self-assigned this Oct 2, 2022

Austaras force-pushed the inline branch from 2014ee3 to efe1646 Compare October 14, 2022 13:55

kdy1 requested review from jridgewell and kdy1 October 14, 2022 14:04

kdy1 reviewed Oct 14, 2022

View reviewed changes

jridgewell previously approved these changes Oct 15, 2022

View reviewed changes

kdy1 reviewed Oct 15, 2022

View reviewed changes

kdy1 previously approved these changes Oct 15, 2022

View reviewed changes

Austaras and others added 2 commits October 15, 2022 13:48

fix(es/minifier): calc function body cost when inline function

18f2762

Rebase

d230e17

kdy1 dismissed stale reviews from jridgewell and themself via d230e17 October 15, 2022 04:52

kdy1 force-pushed the inline branch from efe1646 to d230e17 Compare October 15, 2022 04:52

kdy1 enabled auto-merge (squash) October 15, 2022 04:53

kdy1 approved these changes Oct 15, 2022

View reviewed changes

Merge branch 'main' into inline

5a427e6

swc-bot reviewed Oct 15, 2022

View reviewed changes

kdy1 merged commit b40d486 into swc-project:main Oct 15, 2022

Austaras deleted the inline branch October 15, 2022 06:13

kdy1 modified the milestones: Planned, v1.3.9 Oct 17, 2022

swc-project locked as resolved and limited conversation to collaborators Nov 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(es/minifier): calc function body cost when inline function #5342

fix(es/minifier): calc function body cost when inline function #5342

Austaras commented Jul 30, 2022

kdy1 left a comment

kdy1 commented Aug 3, 2022

kdy1 left a comment

kdy1 commented Aug 3, 2022 •

edited

Austaras commented Aug 3, 2022

Austaras commented Aug 3, 2022

Austaras commented Aug 4, 2022

kdy1 commented Aug 4, 2022 •

edited

kdy1 commented Aug 5, 2022

Austaras commented Aug 5, 2022

Austaras commented Sep 29, 2022

kdy1 commented Oct 13, 2022

Austaras commented Oct 13, 2022

kdy1 commented Oct 13, 2022

kdy1 commented Oct 13, 2022

Austaras commented Oct 13, 2022 •

edited

kdy1 commented Oct 13, 2022

kdy1 commented Oct 13, 2022

Austaras commented Oct 14, 2022

Austaras commented Oct 14, 2022

kdy1 Oct 14, 2022 •

edited

Austaras Oct 14, 2022 •

edited

jridgewell Oct 15, 2022

Austaras Oct 15, 2022

kdy1 left a comment

swc-bot left a comment

fix(es/minifier): calc function body cost when inline function #5342

fix(es/minifier): calc function body cost when inline function #5342

Conversation

Austaras commented Jul 30, 2022

kdy1 left a comment

Choose a reason for hiding this comment

kdy1 commented Aug 3, 2022

kdy1 left a comment

Choose a reason for hiding this comment

kdy1 commented Aug 3, 2022 • edited

Austaras commented Aug 3, 2022

Austaras commented Aug 3, 2022

Austaras commented Aug 4, 2022

kdy1 commented Aug 4, 2022 • edited

kdy1 commented Aug 5, 2022

Austaras commented Aug 5, 2022

Austaras commented Sep 29, 2022

kdy1 commented Oct 13, 2022

Austaras commented Oct 13, 2022

kdy1 commented Oct 13, 2022

kdy1 commented Oct 13, 2022

Austaras commented Oct 13, 2022 • edited

kdy1 commented Oct 13, 2022

kdy1 commented Oct 13, 2022

Austaras commented Oct 14, 2022

Austaras commented Oct 14, 2022

kdy1 Oct 14, 2022 • edited

Choose a reason for hiding this comment

Austaras Oct 14, 2022 • edited

Choose a reason for hiding this comment

jridgewell Oct 15, 2022

Choose a reason for hiding this comment

Austaras Oct 15, 2022

Choose a reason for hiding this comment

kdy1 left a comment

Choose a reason for hiding this comment

swc-bot left a comment

Choose a reason for hiding this comment

kdy1 commented Aug 3, 2022 •

edited

kdy1 commented Aug 4, 2022 •

edited

Austaras commented Oct 13, 2022 •

edited

kdy1 Oct 14, 2022 •

edited

Austaras Oct 14, 2022 •

edited