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

feat(minifier): constant addition expression folding #882

Merged
merged 11 commits into from
Sep 11, 2023

Conversation

DonIsaac
Copy link
Collaborator

@DonIsaac DonIsaac commented Sep 9, 2023

Fold constant addition expressions. Handles string concatenation and addition, both with implicit casting.

For example,

let x = 1 + 1
let y = "hello " + "world"

now becomes

let x = 2
let y = "hello world"

Extra Goodies

  • test(minifier): add test_snapshot helper to perform snapshot tests with insta
  • up(hir): implement std::ops::Add for NumericValue
  • up(span): impl TryFrom<Cow<'_, &str>> for Atom

@DonIsaac DonIsaac added the A-minifier Area - Minifier label Sep 9, 2023
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 10, 2023

CodSpeed Performance Report

Merging #882 will not alter performance

Comparing don/feat/minifier/const-add-folding (75a9b3b) with main (a9d36f1)

Summary

✅ 18 untouched benchmarks

@DonIsaac
Copy link
Collaborator Author

DonIsaac commented Sep 10, 2023

This PR causes several Test262 benchmarks to fail since messages in Test262Error are getting concatinated. The minifier is just checked by minifying twice, then comparing the output. Not sure what to do about this one.

Example:

expected:
if(isNaN(Number.NaN+1)!==!0)throw new Test262Error('#1: NaN + 1 === Not-a-Number. Actual: '+NaN+1);if(isNaN(1+Number.NaN)!==!0)throw new Test262Error('#2: 1 + NaN === Not-a-Number. Actual: '+1+NaN);if(isNaN(Number.NaN+Number.POSITIVE_INFINITY)!==!0)throw new Test262Error('#3: NaN + Infinity === Not-a-Number. Actual: '+NaN+Infinity);if(isNaN(Number.POSITIVE_INFINITY+Number.NaN)!==!0)throw new Test262Error('#4: Infinity + NaN === Not-a-Number. Actual: '+Infinity+NaN);if(isNaN(Number.NaN+Number.NEGATIVE_INFINITY)!==!0)throw new Test262Error('#5: NaN + Infinity === Not-a-Number. Actual: '+NaN+Infinity);if(isNaN(Number.NEGATIVE_INFINITY+Number.NaN)!==!0)throw new Test262Error('#6: Infinity + NaN === Not-a-Number. Actual: '+Infinity+NaN)

actual:
if(isNaN(Number.NaN+1)!==!0)throw new Test262Error('#1: NaN + 1 === Not-a-Number. Actual: NaN1');if(isNaN(1+Number.NaN)!==!0)throw new Test262Error('#2: 1 + NaN === Not-a-Number. Actual: 1NaN');if(isNaN(Number.NaN+Number.POSITIVE_INFINITY)!==!0)throw new Test262Error('#3: NaN + Infinity === Not-a-Number. Actual: NaNInfinity');if(isNaN(Number.POSITIVE_INFINITY+Number.NaN)!==!0)throw new Test262Error('#4: Infinity + NaN === Not-a-Number. Actual: InfinityNaN');if(isNaN(Number.NaN+Number.NEGATIVE_INFINITY)!==!0)throw new Test262Error('#5: NaN + Infinity === Not-a-Number. Actual: NaNInfinity');if(isNaN(Number.NEGATIVE_INFINITY+Number.NaN)!==!0)throw new Test262Error('#6: Infinity + NaN === Not-a-Number. Actual: InfinityNaN')

When both outputs are run through prettier, I get

// expected
if (isNaN(Number.NaN + 1) !== !0)
    throw new Test262Error('#1: NaN + 1 === Not-a-Number. Actual: ' + NaN + 1)
if (isNaN(1 + Number.NaN) !== !0)
    throw new Test262Error('#2: 1 + NaN === Not-a-Number. Actual: ' + 1 + NaN)
if (isNaN(Number.NaN + Number.POSITIVE_INFINITY) !== !0)
    throw new Test262Error(
        '#3: NaN + Infinity === Not-a-Number. Actual: ' + NaN + Infinity
    )
if (isNaN(Number.POSITIVE_INFINITY + Number.NaN) !== !0)
    throw new Test262Error(
        '#4: Infinity + NaN === Not-a-Number. Actual: ' + Infinity + NaN
    )
if (isNaN(Number.NaN + Number.NEGATIVE_INFINITY) !== !0)
    throw new Test262Error(
        '#5: NaN + Infinity === Not-a-Number. Actual: ' + NaN + Infinity
    )
if (isNaN(Number.NEGATIVE_INFINITY + Number.NaN) !== !0)
    throw new Test262Error(
        '#6: Infinity + NaN === Not-a-Number. Actual: ' + Infinity + NaN
    )

// actual
if (isNaN(Number.NaN + 1) !== !0)
    throw new Test262Error('#1: NaN + 1 === Not-a-Number. Actual: NaN1')
if (isNaN(1 + Number.NaN) !== !0)
    throw new Test262Error('#2: 1 + NaN === Not-a-Number. Actual: 1NaN')
if (isNaN(Number.NaN + Number.POSITIVE_INFINITY) !== !0)
    throw new Test262Error(
        '#3: NaN + Infinity === Not-a-Number. Actual: NaNInfinity'
    )
if (isNaN(Number.POSITIVE_INFINITY + Number.NaN) !== !0)
    throw new Test262Error(
        '#4: Infinity + NaN === Not-a-Number. Actual: InfinityNaN'
    )
if (isNaN(Number.NaN + Number.NEGATIVE_INFINITY) !== !0)
    throw new Test262Error(
        '#5: NaN + Infinity === Not-a-Number. Actual: NaNInfinity'
    )
if (isNaN(Number.NEGATIVE_INFINITY + Number.NaN) !== !0)
    throw new Test262Error(
        '#6: Infinity + NaN === Not-a-Number. Actual: InfinityNaN'
    )

This is, however, expected behavior.
image

@Boshen
Copy link
Member

Boshen commented Sep 10, 2023

Terser has the evaluate option for the compressor https://github.com/terser/terser#compress-options

To get this PR merged, we can add this option and turn if off for the conformance test for now.

We can figure out how to make the algorithm pass the test, or change the test later.

Copy link
Member

@Boshen Boshen left a comment

Choose a reason for hiding this comment

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

The code itself is good.

I'm not sure about adding snapshot testing because this deviates from our current testing strategy, which will cause future contributors confused on how to test the minifier. But I can fix this later.

The only blocker is adding the evaluate option to get the conformance test passing.

@Boshen
Copy link
Member

Boshen commented Sep 10, 2023

The test is still failing 🤔

Copy link
Member

@mysteryven mysteryven left a comment

Choose a reason for hiding this comment

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

Just two nitpicks.

crates/oxc_minifier/src/compressor/fold.rs Show resolved Hide resolved
@DonIsaac DonIsaac enabled auto-merge (squash) September 10, 2023 21:32
@DonIsaac DonIsaac merged commit 027a67d into main Sep 11, 2023
17 checks passed
@DonIsaac DonIsaac deleted the don/feat/minifier/const-add-folding branch September 11, 2023 02:38
eryue0220 added a commit to eryue0220/oxc that referenced this pull request Sep 11, 2023
…t/no-redeclare

* 'feat/no-redeclare' of github.com:eryue0220/oxc: (22 commits)
  deps: remove default-features from codspeed-criterion-compat
  chore(deps): bump the dependencies group with 4 updates (oxc-project#893)
  chore(deps): bump actions/checkout from 3 to 4 (oxc-project#894)
  feat(minifier): constant addition expression folding (oxc-project#882)
  chore(benchmark): turn on all lints (oxc-project#892)
  feat(linter): eslint-plugin-import(no-cycle) (oxc-project#890)
  chore: fix typo (oxc-project#889)
  perf(linter): early bail out if not jest fn (oxc-project#885)
  feat(linter): add typescript/no-explicit-any (oxc-project#881)
  feat(website): Hide error panel when query view is shown (oxc-project#884)
  fix(website): fix run_query call arguments (oxc-project#880)
  feat(linter): eslint-plugin-import/no-self-import (oxc-project#878)
  feat(linter): implement re-exports (oxc-project#877)
  ci(benchmark): install toolchain first
  refactor(resolver): clean up `load_alias` (oxc-project#875)
  fix(parser): parse [+In] in object binding initializer (oxc-project#874)
  chore(fuzz): add --sanitizer none command
  chore(fuzz): add nightly instructions
  refactor: clean up fuzzer, move it to repo root (oxc-project#872)
  perf(resolver): avoid double hashing by memoizing the hash (oxc-project#871)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-minifier Area - Minifier
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants