perf(parseComments): optimize string allocations in _parseComments

smhg / gettext-parser

Parse and compile gettext po and mo files, nothing more, nothing less

MIT License

158 stars 43 forks source link

perf(parseComments): optimize string allocations in _parseComments #77

Closed JonasBa closed 1 year ago

JonasBa commented 1 year ago

First of all, thanks for the great project, we use it at Sentry via our po-catalog-loader.

We recently started profiling our webpack build processes and noticed that compared to regular parsing of the files, a significant amount of time was spent in the _parseComments fn (all of the red colored frames are calls to _parseComments fn)

At a quick glance, I suspect that this is due to the amount of string allocations being made - full transparency, I have not benchmarked any of this but would be happy to it if it makes for a stronger case for the changes in this PR.

changes:

instead of pushing to array and joining the values in the end, append the strings directly
since keys are known in advance, hardcode them instead of call Object.keys
merge the substring and regexp replace into a single replace call
return earlier from the token loop

smhg commented 1 year ago

@JonasBa Thank you for your contribution! I'd like to merge #76 first and ask you to rebase/merge those changes afterwards as both impact lib/poparser.js. I'll leave a comment when that's done.

smhg commented 1 year ago

@JonasBa The PR I mentioned earlier has been merged. Would you be able to rebase your work on the updated master branch?

JonasBa commented 1 year ago

@smhg sorry about that, the GH notification got buried to 2nd page... I rebased the PR to the re-synced fork, it should be ok to review now.

smhg commented 1 year ago

@JonasBa thank you! Looking at the changes, I would think returning early in the loops makes all the difference? The loop at the end I'd rather keep as it gets a bit verbose now. Are you ok if I revert that part? And I also think it would be nice to define a key-to-regex Map object so we can loop over that. But that's a separate refactor.

smhg commented 1 year ago

@JonasBa I've published a 7.0.1 version which returns early in the outer loop. Could you please try that in your project(s)? This might be the main difference. Other refactors are welcome too, but let's have a look at this first.

JonasBa commented 1 year ago

@smhg thanks so much, your first reply got buried in my notifications and I only saw it after cleaning up my inbox (result of coming back from vacation). Thanks for making the changes and sorry I missed your reply