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

Replace unshift where possible for performance improvement #107

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

nosvalds
Copy link

Found this when debugging an application I wrote that needs to query some 50-60MB XML files. Using the Chrome Dev tools CPU profiler I identified one of the longest calls was on .unshift(), following some of the suggestions in this post I replaced the unshift calls that I could with .push() and .reverse(). Seemed to make a marked improvement in the CPU profiler report in that area.

Tests are still passing.

CPU Profile Before (17630ms on res.unshift(tokenValue.pop()) ) :

image

CPU Profile After ( push() and reverse() calls are around ~6200ms, down from 17630ms call to unshift() ) :

image

@JLRishe
Copy link
Collaborator

JLRishe commented Jul 6, 2023

Hello @nosvalds, I'm sorry that I have left this PR unresponded for so long. I would like to incorporate your change, but it looks like you used your master branch as the source branch for this PR, and other, unrelated changes have since been added to that branch.

I believe I could cherry-pick the relevant changes myself, but I would like you to get credit for the PR if you want it. If so, could you modify the PR to have only the changes in question (if that's possible), or create a new PR?

I'm happy to merge the changes myself if it's a hassle for you.

Thanks again!

@nosvalds
Copy link
Author

nosvalds commented Jul 19, 2023

Hey @JLRishe , sorry I'm not super active on GitHub with my current job. Apologies for using the wrong source branch here and no worries on you going ahead making the changes. I don't mind not getting credit.

Thanks,
Nik

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants