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

perf(es/minifier): check whether a function is pure with O(1) cost #6840

Merged
merged 9 commits into from
Jan 24, 2023

Conversation

hyf0
Copy link
Contributor

@hyf0 hyf0 commented Jan 20, 2023

Description:

We might need to pass a giant pure function list to the SWC minifier as esbuild did here.

The current implementation could slow the minifier.

BREAKING CHANGE:

Related issue (if exists):

@hyf0 hyf0 force-pushed the hyf_improve_minifier_hello_today branch from 1365c19 to 3b8f5ca Compare January 20, 2023 13:41
@kdy1
Copy link
Member

kdy1 commented Jan 20, 2023

Can we have those list also in swc minifier? (itself)

@hyf0
Copy link
Contributor Author

hyf0 commented Jan 20, 2023

Can we have those list also in swc minifier? (itself)

Absolute yes. I would do it in another PR.

We might need to provide an option(enable by default) to disable it. Because users might do some hacky things to the environment.

@kdy1
Copy link
Member

kdy1 commented Jan 20, 2023

We have pristine_globals

@hyf0
Copy link
Contributor Author

hyf0 commented Jan 20, 2023

We have pristine_globals

Get it.

@hyf0 hyf0 marked this pull request as ready for review January 20, 2023 14:15
@hyf0
Copy link
Contributor Author

hyf0 commented Jan 20, 2023

@kdy1 we might need something like HashIgnoreSpan to produce the same hash value for&Exprs with different spans to use HashSet. I'm not sure this is the best solution.

@kdy1
Copy link
Member

kdy1 commented Jan 20, 2023

I think we can use a hash set of (JsWord, JsWord).
Is there more complex expression other than those globals?

@hyf0
Copy link
Contributor Author

hyf0 commented Jan 20, 2023

I think we can use a hash set of (JsWord, JsWord). Is there more complex expression other than those globals?

Yes. Dealing Expr::Member and Expr::Ident should be enough base on the input here.

@kdy1 kdy1 self-assigned this Jan 21, 2023
@hyf0
Copy link
Contributor Author

hyf0 commented Jan 21, 2023

The wasm test due to net work.

Expr::Member(i) => {
Self(&i.obj).hash(state);
if let MemberProp::Ident(prop) = &i.prop {
prop.sym.hash(state);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a bug here that console.log will hash the same as the identifier consolelog. We'd need to hash "." or similar sigil.

Copy link
Member

Choose a reason for hiding this comment

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

Does it matter? Eq will be used if the hash value matches

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhhhhhh.

Expr::Member(i) => {
Self(&i.obj).hash(state);
if let MemberProp::Ident(prop) = &i.prop {
prop.sym.hash(state);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhhhhhh.

Copy link
Member

@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.

Thank you!


swc-bump:

  • swc_ecma_minifier

@kdy1 kdy1 enabled auto-merge (squash) January 24, 2023 05:40
@kdy1 kdy1 modified the milestones: Planned, v1.3.28 Jan 24, 2023
@kdy1 kdy1 merged commit 58208ef into swc-project:main Jan 24, 2023
@swc-project swc-project locked as resolved and limited conversation to collaborators Feb 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants