Avoids emitting the same output from multiple processes of the same process pool #3531
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 latest updates on your projects. Learn more about Vercel for Git ↗︎
|
crates/turbopack-node/src/pool.rs
Outdated
| let mut shared = shared.lock().unwrap(); | ||
| shared.insert((line.clone(), occurances)) | ||
| }; | ||
| if new_line { |
There was a problem hiding this comment.
So if I'm understanding this correctly, a new line gets logged whenever there are more than the previous amount of occurrences?
This also means if one execution logs something 2 times and the next logs it 3 times, it will only output 1 time for the next execution? That seems like confusing behavior that might potentially hide information necessary for debugging
There was a problem hiding this comment.
Yeah, it could be a bit confusing in some edge cases... Do you have a better idea?
There was a problem hiding this comment.
maybe do it for the whole output and not line by line
Benchmark for 81e2498
Click to view full benchmark
|
Benchmark for 2f063d8
Click to view full benchmark
|
|
jridgewell
suggested changes
Jan 30, 2023
| } | ||
| let line = Arc::from(take(&mut buffer).into_boxed_slice()); | ||
| let occurance_number = *own_output | ||
| .entry(Arc::clone(&line)) |
There was a problem hiding this comment.
Can we store a hash instead of the raw bytes? Wouldn't that save memory overall?
There was a problem hiding this comment.
That work work (with a small risk of collisions), but that would loose the ability to read the logged output from the browser ui in future.
| .or_insert(0); | ||
| let new_line = { | ||
| let mut shared = shared.lock().unwrap(); | ||
| shared.insert((line.clone(), occurance_number)) |
There was a problem hiding this comment.
Why do we need occurance_number? Wouldn't just a HashSet work?
There was a problem hiding this comment.
I wanted to allow logging lines multiple times from a process.
There was a problem hiding this comment.
I initially planned to use HashMap<line: String, count: u32> but that won't preserve the order of logging. So it's a IndexSet<(line: String, occurance_number: u32)> which preserves all logging in the correct order.
We can display that in the browser UI for inspecting by iterating over the IndexSet. That will give all logging in the right order. Merged from all processes of the pool.
Benchmark for 9c2ea3aClick to view benchmark
|
alexkirsz
reviewed
Jan 31, 2023
| Ok(_) => {} | ||
| } | ||
| let line = Arc::from(take(&mut buffer).into_boxed_slice()); | ||
| let occurance_number = *own_output |
There was a problem hiding this comment.
Suggested change
| let occurance_number = *own_output | |
| let occurrence_number = *own_output |
| .or_insert(0); | ||
| let new_line = { | ||
| let mut shared = shared.lock().unwrap(); | ||
| shared.insert((line.clone(), occurance_number)) |
There was a problem hiding this comment.
Suggested change
| shared.insert((line.clone(), occurance_number)) | |
| shared.insert((line.clone(), occurrence_number)) |
| stream: impl AsyncRead + Unpin, | ||
| shared: SharedOutputSet, | ||
| mut final_stream: impl AsyncWrite + Unpin, | ||
| ) { |
There was a problem hiding this comment.
If stream A reports:
error A:
shared line
non-shared line (A)
while stream B reports:
error B:
shared line
non-shared line (B)
We would end up with:
error A:
shared line
non-shared line (A)
error B:
non-shared line (B)
I expect this to also happen with empty lines printed for formatting purposes.
Should we instead only dedupe the start of a stream, up until the first different character?
Benchmark for ba8c683Click to view benchmark
|
sokra
deleted the
sokra/web-521-duplicate-logging-from-the-nodejs-pooled
branch
mehulkar
pushed a commit
that referenced
this pull request
Feb 1, 2023
…rocess pool (#3531) It uses a `shared` set of `(line: [u8], occurences: u32)` to avoid emitting lines that has been emitted by other processes already. It still supports emitting the same line multiple times from one process. The `shared` data might be later used for showing logging from a worker pool. It contains the merged output of all processes. Fixes WEB-489
timneutkens
pushed a commit
to vercel/next.js
that referenced
this pull request
Feb 10, 2023
…49736102+kodiakhq[bot]@users.noreply.github.com> # New Features - vercel/turbo#3540 - vercel/turbo#3549 - vercel/turbo#3465 - vercel/turbo#3550 - vercel/turbo#3495 - vercel/turbo#3624 - vercel/turbo#3600 - vercel/turbo#3676 - vercel/turbo#3689 # Fixes - vercel/turbo#3437 - vercel/turbo#3542 - vercel/turbo#3531 - vercel/turbo#3552 - vercel/turbo#3551 - vercel/turbo#3597 - vercel/turbo#3644 - vercel/turbo#3623 - vercel/turbo#3634 - vercel/turbo#3574 - vercel/turbo#3673 - vercel/turbo#3675 - vercel/turbo#3723 - vercel/turbo#3677 - vercel/turbo#3717 - vercel/turbo#3701 # Performance Improvements - vercel/turbo#3361 - vercel/turbo#3619 --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
|
Why did you merge this with 0 approvals and 1 request for change? |
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.
It uses a
sharedset of(line: [u8], occurences: u32)to avoid emitting lines that has been emitted by other processes already.It still supports emitting the same line multiple times from one process.
The
shareddata might be later used for showing logging from a worker pool. It contains the merged output of all processes.Fixes WEB-489