search

ibmruntimes / yieldable-json

Asynchronous JSON parser and stringify APIs that make use of generator patterns
Other
145 stars 22 forks source link

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

Closed lukas8219 closed 3 days ago

lukas8219 commented 2 months ago

Fixes https://github.com/ibmruntimes/yieldable-json/issues/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

lukas8219 commented 1 month ago

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?

lukas8219 commented 3 weeks ago

@gireeshpunathil Any updates here?

gireeshpunathil commented 3 weeks ago

@lukas8219 - thanks for the ping. apologies, I am stuck with multiple high priority items this month. I need one or two weeks more,

gireeshpunathil commented 6 days ago

@lukas8219 - could you pls squash all the commits into one?

lukas8219 commented 5 days ago

@gireeshpunathil Only those with write access to this repository can merge pull requests.

Or do you mean squash all commits locally and force-push a single commit?

gireeshpunathil commented 5 days ago

yes - the second (squash everything locally into one and force push)

lukas8219 commented 4 days ago

@gireeshpunathil Done

gireeshpunathil commented 3 days ago

thanks @lukas8219 for the contribution! much appreciated!