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

Optimization of Array.push() #3212

Closed
dan1wang opened this issue Jan 25, 2019 · 4 comments
Closed

Optimization of Array.push() #3212

dan1wang opened this issue Jan 25, 2019 · 4 comments
Labels
help wanted internal-issue-created An internal Google issue has been created to track this GitHub issue triage-done Has been reviewed by someone on triage rotation.

Comments

@dan1wang
Copy link

After I reported Issue #3211, I discovered Closure Compiler (ADVANCED_OPTIMIZATIONS) also doesn't optimize push().

I had

      obj.arrayProp.push( offsetSegment(mask, offsetX, 0) );
      obj.arrayProp.push( offsetSegment(mirroredMask, offsetX, 0) );

and I get

a.f.push(offsetSegment(c,d,0)),
a.f.push(offsetSegment(h,d,0)),

instead of
a.f.push(offsetSegment(c,d,0),offsetSegment(h,d,0)),

@tjgq
Copy link
Contributor

tjgq commented Jan 25, 2019

Created Google internal issue b/123406223

@tjgq tjgq added triage-done Has been reviewed by someone on triage rotation. internal-issue-created An internal Google issue has been created to track this GitHub issue labels Jan 25, 2019
@ChadKillingsworth
Copy link
Collaborator

This would be a pretty good first contribution. It can be done with a peephole optimization.

@cshung
Copy link
Contributor

cshung commented Mar 11, 2019

I wanted to attempt this one.

@brad4d
Copy link
Contributor

brad4d commented May 6, 2019

Once I looked more closely at the PR proposed to fix this issue, I realized it wasn't really an optimization we should add.

From the discussion on that PR:

Overall this optimization doesn't strike me as a very useful one.
It's not likely that a human author would put a lot of array.push(), right next to each other,
so I'd think this would only be useful if other optimizations ended up putting them together, which
then allowed this optimization to do something useful. That also seems likely to be a rare thing to me.

Execution time is precious for large projects.

@brad4d brad4d closed this as completed May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted internal-issue-created An internal Google issue has been created to track this GitHub issue triage-done Has been reviewed by someone on triage rotation.
Projects
None yet
Development

No branches or pull requests

5 participants