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

fix(yieldable-parser): parseAsync sometimes blocks for a *long* time #36 #37

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

Conversation

lukas8219
Copy link

Fixes #36

Diagnosis:

  1. Run node --prof parse-text.js
  2. With the generated file, run node --prof-process <file.name> >> benchmark.txt
  3. Check the code path that is being executed the most.

I've encapsulated the possible aggressors into processOne, processTwo functions so the profiler could gather more precise information... and re-run the same diagnosis steps - which lead me to this benchmark results bench-indexOf.txt

It's pretty clear that the main aggressor was while loop that had synchronous indexOf operations, causing performance hits in long strings.

Solution:

Split the indexOf method into it's own generator-style - yielding at each N sized chunk.

Results:

Using the same script shared by the issue author, event-loop was unblocked. But there was some performance hit, expected due to generators yielding.

Using master code: 1.92s user 3.03s system 109% cpu 4.534 total

Using new code: 5.17s user 3.95s system 134% cpu 6.785 total

test/async-index-of.js Outdated Show resolved Hide resolved
yieldable-parser.js Outdated Show resolved Hide resolved
@lukas8219
Copy link
Author

Updates:

Finally got some time to run it on a non-virtualized environment. Just ran the same test on GCP using Linux ebpf-playground 5.15.0-1060-gcp #68~20.04.1-Ubuntu SMP Wed May 1 14:35:27 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

And as expected, performance wasn't a huge deal.

Noticed a small increase of 2 seconds between approaches.

Captura de Tela 2024-08-25 às 19 09 42 Captura de Tela 2024-08-25 às 19 07 54

@gireeshpunathil Would you consider this a NO-GO?

Copy link
Author

Choose a reason for hiding this comment

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

This one may require attention into the JSON info. I copied and pasted from the shared file in the issue

utils/index-of-generator.js Outdated Show resolved Hide resolved
@lukas8219 lukas8219 changed the title fix: parseAsync Sometimes blocks for a *long* time #36 fix(yieldable-parser): parseAsync sometimes blocks for a *long* time #36 Sep 3, 2024
@lukas8219 lukas8219 marked this pull request as ready for review September 3, 2024 10:31
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.

parseAsync Sometimes blocks for a *long* time
1 participant