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

Introducing PeepholeCombineArrayPush #3338

Closed

Conversation

cshung
Copy link
Contributor

@cshung cshung commented Apr 18, 2019

Fixes issue #3212

@brad4d brad4d self-assigned this Apr 18, 2019
@tjgq
Copy link
Contributor

tjgq commented Apr 18, 2019

Created Google internal issue b/130801558.

Assigning to @brad4d for review.

@tjgq tjgq added 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. labels Apr 18, 2019
@cshung cshung force-pushed the dev/andrewau/PeepholeCombineArrayPush branch from 5b8e2c5 to 02f8be4 Compare April 19, 2019 05:23
@brad4d
Copy link
Contributor

brad4d commented Apr 19, 2019

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. Do you have a use-case in which the code size savings from this optimization will justify the time spent in looking for places where it can be done?

@cshung
Copy link
Contributor Author

cshung commented Apr 20, 2019

Overall this optimization doesn't strike me as a very useful one.

I agree. I was hunting for an issue to contribute to a compiler related project, and this one seems like a challenge that I would like to get started on, so I did it. It was marked help wanted and was commented as a good first issue, so I took it on, without a second thought. In retrospect, I probably should have considered the cost/benefit for the optimization.

Do you have a use-case in which the code size savings from this optimization will justify the time spent in looking for places where it can be done?

@dan1wang, can you help us to understand why do we want this optimization?

@cshung cshung force-pushed the dev/andrewau/PeepholeCombineArrayPush branch from 0f89b7a to 45c753d Compare April 20, 2019 03:19
@dan1wang
Copy link

I think the optimization is useful only if the original array is pretty large. I don't know of a use-case where that's the case (I did find this ugly code, though: https://github.com/nemac/seldon/blob/master/areas.js).

I think it would be more worthwhile to address issue #3211 (string.concat & array.concat) because it's more likely one of the string/array is fairly large.

@brad4d
Copy link
Contributor

brad4d commented Apr 24, 2019

Sounds like we've agreed to close this PR.
@cshung thanks for submitting it anyway.
Hopefully you still found the experience a valuable one and can apply it to creating another?
:)

@brad4d brad4d closed this Apr 24, 2019
@cshung
Copy link
Contributor Author

cshung commented Apr 24, 2019

Hopefully you still found the experience a valuable one and can apply it to create another?

@brad4d Can you please recommend an issue that is good?

@brad4d
Copy link
Contributor

brad4d commented Apr 24, 2019

@cshung As it happens I've just been looking at #3343 which would probably be a good first PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes 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

Successfully merging this pull request may close these issues.

None yet

5 participants