Add streaming 'jsonl' parser #3831
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current build setup doesn't seem to have a concept for benchmark scripts, so for now you'll have to run it with something like ts-node.
|
Perahps add a minor comment about why |
esbena
approved these changes
Nov 19, 2024
aeisenberg
reviewed
Nov 19, 2024
| await handler(JSON.parse(buffer)); | ||
| } catch (e) { | ||
| reject(e); | ||
| return; |
There was a problem hiding this comment.
Minor: If you move the logger.log statement and the resolve() call into the try block, you won't need a return here.
|
I think it's fine to check the benchmark script in. |
aeisenberg
reviewed
Nov 19, 2024
There was a problem hiding this comment.
Can you add a file comment, or a README describing what the benchmark does and that it's not being run on a regular basis? Also, it would be nice to include the results that you have in the PR description.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replaces the jsonl parser with a streaming version that is at least as fast as the sync version. Also see original PR against the hackathon branch.
Thanks to @esbena for the initial streaming version used for the hackaton. That version used the
readlinelibrary to split lines, which turned out to be a bottleneck, however, making the streaming parser slower than the original sync version. So I wrote one that doesn't usereadline.Running the benchmark on a 21 MB logfile:
readJsonlReferenceImpl: 172.4 ms (original non-streaming version)readJsonlFile: 283.3 ms (streaming version based onreadline)readJsonlFile2: 151.3 ms (new version withoutreadline)justReadline: 187.5 ms (consumes the file withreadlineand nothing else)On a 520 MB logfile:
readJsonlReferenceImpl: out of memoryreadJsonlFile: 6439.4 msreadJsonlFile2: 3538.4 msjustReadline: 3664.3 msI've added the benchmark script although the project doesn't seem to have much infrastructure for benchmark scripts. At the moment you'll have to run it with something like
ts-nodeand there's no tests to ensure the benchmark script keeps working (but it will be checked for compilation errors). I'm on the fence about whether it should be committed.