Speed up streamed-proto query output by distributing work to multiple threads #12
Conversation
|
Do you think this is something we could add behind a flag to start off? I agree it's useful to have, but it also comes with misc subtleties that are hard to reason about all at once, so having an escape hatch in case we get it wrong the first time would be nice. |
|
I had actually been thinking of more of an opt-in that possibly makes it more clear it's experimental, depending on how solid we can get it on the first try, then eventually flip the default. The whole parallel-streaming-query thing gets tricky when we need to get exception handling involved, an opt-in will let us get code in and iterate with less risk of breakages. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses issue bazelbuild#24304 by speeding up the bazel query ... command when used with the --output=streamed_proto option. The changes introduce multi-threading to distribute the work of writing the proto output, resulting in a reported 54% reduction in wall time for a fully warm query. The original implementation was single-threaded, and this change leverages a ForkJoinPool to parallelize the writing of target proto buffers to the output stream.
Highlights
- Performance Improvement: The primary goal of this PR is to improve the performance of
bazel querywith--output=streamed_protoby distributing the workload across multiple threads. - Multi-threading Implementation: A
ForkJoinPoolis used to parallelize the process of convertingTargetobjects to proto buffers and writing them to the output stream. The number of threads defaults to the number of available processors. - Chunking and Queuing: The input
Iterable<Target>is partitioned into chunks of 500 targets each. These chunks are then submitted to theForkJoinPoolvia aLinkedBlockingQueueto be processed in parallel. - Error Handling: WrappedIOException and WrappedInterruptedException are introduced to handle exceptions thrown during the parallel processing and re-thrown as RuntimeExceptions.
Changelog
- src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedProtoOutputFormatter.java
- Introduces multi-threading using
ForkJoinPoolto parallelize proto output writing. - Partitions the input targets into chunks for parallel processing.
- Uses a
LinkedBlockingQueueto manage the chunks to be processed by the thread pool. - Adds
WrappedIOExceptionandWrappedInterruptedExceptionto handle exceptions during parallel processing. - Implements
writeTargetsDelimitedToByteArraysto convert a list of targets to byte arrays in parallel. - Implements
toPrototo convert a target to a proto object. - Implements
writeDelimitedto write a proto object to a byte array in a delimited format. - Adds constants for
MAX_CHUNKS_IN_QUEUEandTARGETS_PER_CHUNKto control the chunking and queuing behavior.
- Introduces multi-threading using
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A query's slow pace,
Threads now race, time and space,
Swift proto takes flight.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OutputFormatterCallback
participant ForkJoinPool
participant BlockingQueue
participant OutputStream
Client->>OutputFormatterCallback: processOutput(targets)
OutputFormatterCallback->>ForkJoinPool: Submit chunked serialization tasks
ForkJoinPool-->>BlockingQueue: Place futures for each chunk
loop For each chunk future in order
BlockingQueue->>OutputFormatterCallback: Take next completed future
OutputFormatterCallback->>OutputStream: Write serialized bytes
end
OutputFormatterCallback-->>Client: Completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Code Review
This pull request introduces multi-threading to the streamed proto output formatter, significantly improving query performance. The approach of using a ForkJoinPool and a blocking queue seems reasonable for distributing the work. However, there are a few areas that could be improved, particularly around error handling and resource management.
Summary of Findings
- Error Propagation from Executor: The code uses
RuntimeExceptionto wrap exceptions from the executor, losing valuable information about the original exception. It's crucial to propagate the original exception to provide better debugging insights. - Resource Management of ForkJoinPool: While the
ForkJoinPoolis created within a try-with-resources block, ensuring proper shutdown, the tasks submitted to the pool might still be running after the block completes if they are long-running or blocked. Consider explicitly shutting down the executor and awaiting termination to ensure all tasks are completed before proceeding. - Wrapped Exception Classes: The
WrappedIOExceptionandWrappedInterruptedExceptionclasses are simple wrappers around checked exceptions. Consider whether these wrappers are truly necessary, or if the exceptions can be handled directly.
Merge Readiness
The performance improvements introduced by this pull request are significant, making it a valuable contribution. However, the error handling and resource management aspects need to be addressed before merging. I recommend resolving the issues raised in the review comments, particularly regarding exception propagation and ForkJoinPool shutdown. I am unable to approve this pull request, and other reviewers should review and approve this code before merging.
| // TODO: figure out what might be in here and propagate | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
This TODO indicates that the exception handling here is incomplete. It's important to propagate the original ExecutionException's cause to provide more context about the failure. Consider using e.getCause() to get the underlying exception and wrap it in a more specific exception type, or rethrow it if appropriate.
Also, consider logging the exception with sufficient context to aid debugging.
// TODO: figure out what might be in here and propagate
Throwable cause = e.getCause();
if (cause instanceof IOException) {
throw new WrappedIOException((IOException) cause);
} else if (cause instanceof InterruptedException) {
throw new WrappedInterruptedException((InterruptedException) cause);
} else {
throw new RuntimeException("Error during target processing", cause);
}| // TODO: propagate | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
Similar to the previous TODO, this one also requires proper exception propagation. The ExecutionException should be unwrapped to get the root cause and either rethrown as a more specific exception or wrapped in a RuntimeException with the cause attached. Logging the exception is also recommended.
} catch (ExecutionException e) {
// TODO: propagate
Throwable cause = e.getCause();
if (cause instanceof IOException) {
throw new WrappedIOException((IOException) cause);
} else if (cause instanceof InterruptedException) {
throw new WrappedInterruptedException((InterruptedException) cause);
} else {
throw new RuntimeException("Error during target processing", cause);
}| stillAddingTargetsToQueue.set(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
After setting stillAddingTargetsToQueue to false, it's good practice to explicitly shutdown the executor and wait for its termination. This ensures that all submitted tasks are completed before proceeding. Otherwise, there is a risk that the executor will not shutdown cleanly, and resources may be leaked. Consider adding executor.shutdown() and executor.awaitTermination() calls within the finally block.
} finally {
stillAddingTargetsToQueue.set(false);
executor.shutdown(); // Initiate shutdown
try {
if (!executor.awaitTermination(60, TimeUnit.SECONDS)) { // Wait for tasks to complete
System.err.println("ForkJoinPool did not terminate in the specified timeout.");
// Optionally, log the state of the tasks that are still running.
}
} catch (InterruptedException ie) {
// (Re-)Cancel if current thread also interrupted
executor.shutdownNow();
// Preserve interrupt status
Thread.currentThread().interrupt();
}
}| @Override | ||
| public IOException getCause() { | ||
| return (IOException) super.getCause(); |
There was a problem hiding this comment.
Consider whether wrapping the IOException in a RuntimeException is truly necessary. If the calling code can handle IOException directly, it might be better to avoid the wrapping. If wrapping is necessary, ensure that the original exception is accessible via getCause().
@Override
public IOException getCause() {
return cause;
}| public InterruptedException getCause() { | ||
| return (InterruptedException) super.getCause(); | ||
| } |
There was a problem hiding this comment.
Similar to WrappedIOException, evaluate the necessity of wrapping InterruptedException. If the calling code can handle it directly, avoid wrapping. If wrapping is necessary, ensure the original exception is accessible via getCause().
@Override
public InterruptedException getCause() {
return cause;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedProtoOutputFormatter.java (1)
92-96: Propagate checked exceptions instead of wrapping them in genericRuntimeException
writeAllTargetsFuture.get()can surfaceWrappedIOException/WrappedInterruptedException. By catchingExecutionExceptionand blindly re-wrapping, callers lose the ability to handleIOExceptionor honour the method’s declaredInterruptedException.Suggestion:
- } catch (ExecutionException e) { - // TODO: propagate - throw new RuntimeException(e); + } catch (ExecutionException e) { + Throwable cause = e.getCause(); + if (cause instanceof WrappedIOException ioe) { + throw ioe.getCause(); // preserves declared throws IOException + } + if (cause instanceof WrappedInterruptedException ie) { + Thread.currentThread().interrupt(); + throw ie.getCause(); + } + throw new RuntimeException(cause); }This keeps the public contract intact and avoids masking interruption.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedProtoOutputFormatter.java(2 hunks)
🔇 Additional comments (1)
src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedProtoOutputFormatter.java (1)
63-90: Potential producer/consumer termination race
stillAddingTargetsToQueueis set tofalseonly in thefinallyblock after the producer loop. IfpartialResultis empty, the queue remains empty and the consumer immediately blocks ontake()forever, preventingwriteAllTargetsFuturefrom completing.Consider signalling completion explicitly:
// after producing chunks targetQueue.put(POISON_PILL_FUTURE); // Future that returns an empty list ... while (true) { Future<List<byte[]>> f = targetQueue.take(); if (f == POISON_PILL_FUTURE) break; ... }or a
BlockingQueuewithnullsentinel, ensuring graceful shutdown even for empty inputs.
| new ForkJoinPool( | ||
| Runtime.getRuntime().availableProcessors(), | ||
| ForkJoinPool.defaultForkJoinWorkerThreadFactory, | ||
| null, | ||
| // we use asyncMode to ensure the queue is processed FIFO, which maximizes | ||
| // throughput | ||
| true)) { | ||
| var targetQueue = new LinkedBlockingQueue<Future<List<byte[]>>>(MAX_CHUNKS_IN_QUEUE); | ||
| var stillAddingTargetsToQueue = new AtomicBoolean(true); | ||
| writeAllTargetsFuture = | ||
| executor.submit( | ||
| () -> { | ||
| try { | ||
| while (stillAddingTargetsToQueue.get() || !targetQueue.isEmpty()) { | ||
| Future<List<byte[]>> targets = targetQueue.take(); | ||
| for (byte[] target : targets.get()) { | ||
| out.write(target); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ForkJoinPool worker is blocking on take()/get() – high risk of thread starvation
The consumer task runs inside the same ForkJoinPool that processes the producer tasks yet:
- performs unmanaged blocking (
targetQueue.take()andFuture.get()), - uses a bounded queue that may fill up, causing the producer thread (caller of
processOutput) to block onput().
Because ForkJoinPool counts blocked workers toward parallelism, the pool can dead-lock or under-utilise CPUs when many long-running producer tasks occupy the limited workers while the single consumer waits, or vice-versa.
Recommended approaches:
- Start the consumer on a dedicated thread (e.g.
Executors.newSingleThreadExecutor) outside the FJP, or - Wrap blocking calls with
ForkJoinPool.managedBlock, or - Replace the FJP entirely with a plain
ExecutorServicethat tolerates blocking.
This will eliminate the starvation risk and make behaviour more predictable.
...ain/java/com/google/devtools/build/lib/query2/query/output/StreamedProtoOutputFormatter.java
Show resolved
Hide resolved
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| throw new WrappedInterruptedException(e); | ||
| } catch (IOException e) { | ||
| throw new WrappedIOException(e); | ||
| } catch (ExecutionException e) { |
There was a problem hiding this comment.
Thread Safety Issue: The ExecutionException handling is inadequate, using a generic RuntimeException with a TODO comment. This can mask important exceptions during task execution, potentially leading to resource leaks or thread pool exhaustion.
Consider unwrapping and properly handling the cause:
catch (ExecutionException e) {
Throwable cause = e.getCause();
if (cause instanceof IOException) {
throw new WrappedIOException((IOException) cause);
} else if (cause instanceof InterruptedException) {
Thread.currentThread().interrupt(); // Preserve interrupt status
throw new WrappedInterruptedException((InterruptedException) cause);
} else if (cause instanceof RuntimeException) {
throw (RuntimeException) cause;
} else {
throw new RuntimeException("Error processing targets", cause);
}
}
| throws IOException, InterruptedException { | ||
| for (Target target : partialResult) { | ||
| toTargetProtoBuffer(target, labelPrinter).writeDelimitedTo(out); | ||
| ForkJoinTask<?> writeAllTargetsFuture; |
There was a problem hiding this comment.
Resource Management Issue: The ForkJoinPool is created with parallelism equal to all available processors without any safeguards against resource exhaustion. On many-core systems, this could lead to excessive thread creation and CPU contention.
Consider limiting the maximum parallelism and adding an exception handler:
// Limit the number of threads to avoid resource exhaustion
int parallelism = Math.min(Runtime.getRuntime().availableProcessors(), 4);
try (ForkJoinPool executor = new ForkJoinPool(
parallelism,
ForkJoinPool.defaultForkJoinWorkerThreadFactory,
(thread, throwable) -> {
// Log uncaught exceptions in worker threads
System.err.println("Worker thread exception: " + throwable);
},
// we use asyncMode to ensure the queue is processed FIFO
true)) {
| } | ||
| }); | ||
| try { | ||
| for (List<Target> targets : Iterables.partition(partialResult, TARGETS_PER_CHUNK)) { |
There was a problem hiding this comment.
Concurrency Issue: The blocking targetQueue.put() operation could lead to thread starvation if the queue becomes full. There's no timeout or circuit-breaking mechanism to handle slow or stuck tasks.
Consider using a non-blocking approach with timeouts:
// Use offer with timeout to prevent indefinite blocking
Future<List<byte[]>> task = executor.submit(() -> writeTargetsDelimitedToByteArrays(targets));
if (!targetQueue.offer(task, 5, TimeUnit.SECONDS)) {
// If queue is full for too long, process this batch directly to make progress
for (byte[] target : task.get(30, TimeUnit.SECONDS)) {
out.write(target);
}
}
…t/StreamedProtoOutputFormatter.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedProtoOutputFormatter.java (5)
53-61: Address resource management and parallelism concerns from previous reviewsThe ForkJoinPool creation has several issues that were previously identified but remain unaddressed:
- No upper bound on parallelism (could exhaust resources on many-core systems)
- No exception handler for uncaught worker thread exceptions
Apply this fix to address the resource management issues:
-try (ForkJoinPool executor = - new ForkJoinPool( - Runtime.getRuntime().availableProcessors(), - ForkJoinPool.defaultForkJoinWorkerThreadFactory, - null, - // we use asyncMode to ensure the queue is processed FIFO, which maximizes - // throughput - true)) { +int parallelism = Math.min(Runtime.getRuntime().availableProcessors(), 8); +try (ForkJoinPool executor = + new ForkJoinPool( + parallelism, + ForkJoinPool.defaultForkJoinWorkerThreadFactory, + (thread, throwable) -> { + System.err.println("Worker thread exception in query processing: " + throwable); + }, + // we use asyncMode to ensure the queue is processed FIFO, which maximizes + // throughput + true)) {
64-82: Critical threading architecture issue - consumer runs in same ForkJoinPoolThe consumer task is submitted to the same ForkJoinPool that processes the producer tasks, creating a high risk of deadlock and thread starvation as previously identified. The consumer performs blocking operations (
take(),get()) which can starve the work-stealing threads.Move the consumer to a dedicated thread outside the ForkJoinPool:
+ExecutorService consumerExecutor = Executors.newSingleThreadExecutor(); +try { var targetQueue = new LinkedBlockingQueue<Future<List<byte[]>>>(MAX_CHUNKS_IN_QUEUE); var stillAddingTargetsToQueue = new AtomicBoolean(true); - writeAllTargetsFuture = - executor.submit( + writeAllTargetsFuture = + consumerExecutor.submit( () -> { // consumer logic remains the same }); +} finally { + consumerExecutor.shutdown(); + if (!consumerExecutor.awaitTermination(60, TimeUnit.SECONDS)) { + consumerExecutor.shutdownNow(); + } +}
78-81: Incomplete exception handling in consumer taskThe TODO comment indicates incomplete exception propagation. This masks important failures and can lead to silent data loss or resource leaks.
Properly unwrap and propagate the ExecutionException cause:
-} catch (ExecutionException e) { - // TODO: figure out what might be in here and propagate - throw new RuntimeException(e); +} catch (ExecutionException e) { + Throwable cause = e.getCause(); + if (cause instanceof IOException) { + throw new WrappedIOException((IOException) cause); + } else if (cause instanceof InterruptedException) { + Thread.currentThread().interrupt(); + throw new WrappedInterruptedException((InterruptedException) cause); + } else if (cause instanceof RuntimeException) { + throw (RuntimeException) cause; + } else { + throw new RuntimeException("Error processing targets", cause); + }
84-89: Potential blocking issue with bounded queueThe
targetQueue.put()operation can block indefinitely if the consumer falls behind, as previously identified. This could lead to thread starvation.Use a timeout-based approach to prevent indefinite blocking:
-for (List<Target> targets : Iterables.partition(partialResult, TARGETS_PER_CHUNK)) { - targetQueue.put(executor.submit(() -> writeTargetsDelimitedToByteArrays(targets))); -} +for (List<Target> targets : Iterables.partition(partialResult, TARGETS_PER_CHUNK)) { + Future<List<byte[]>> task = executor.submit(() -> writeTargetsDelimitedToByteArrays(targets)); + if (!targetQueue.offer(task, 30, TimeUnit.SECONDS)) { + throw new RuntimeException("Queue timeout - consumer may be stuck"); + } +}
94-96: Duplicate incomplete exception handlingSame issue as in the consumer task - ExecutionException is not properly unwrapped and propagated.
Apply the same fix as for the consumer task:
-} catch (ExecutionException e) { - // TODO: propagate - throw new RuntimeException(e); +} catch (ExecutionException e) { + Throwable cause = e.getCause(); + if (cause instanceof IOException) { + throw (IOException) cause; + } else if (cause instanceof InterruptedException) { + Thread.currentThread().interrupt(); + throw (InterruptedException) cause; + } else if (cause instanceof RuntimeException) { + throw (RuntimeException) cause; + } else { + throw new RuntimeException("Error waiting for consumer completion", cause); + }
🧹 Nitpick comments (1)
src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedProtoOutputFormatter.java (1)
45-47: Consider making chunk size and queue capacity configurableThe hardcoded constants may not be optimal for all environments:
TARGETS_PER_CHUNK = 500might be too small/large depending on target complexityMAX_CHUNKS_IN_QUEUE = processors * 2could cause memory issues with large targetsConsider making these configurable via system properties or constructor parameters:
-private static final int MAX_CHUNKS_IN_QUEUE = Runtime.getRuntime().availableProcessors() * 2; -private static final int TARGETS_PER_CHUNK = 500; +private static final int MAX_CHUNKS_IN_QUEUE = Integer.getInteger("bazel.query.max_chunks", Runtime.getRuntime().availableProcessors() * 2); +private static final int TARGETS_PER_CHUNK = Integer.getInteger("bazel.query.targets_per_chunk", 500);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedProtoOutputFormatter.java(2 hunks)
🔇 Additional comments (6)
src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedProtoOutputFormatter.java (6)
16-21: LGTM - Import additions support the concurrent implementationThe added imports (
Iterables,Build,CodedOutputStream,List, concurrent utilities, andAtomicBoolean) are all necessary for the new multi-threaded implementation.Also applies to: 25-27
99-101: LGTM - Clean helper method implementationThe
writeTargetsDelimitedToByteArraysmethod correctly maps targets to delimited byte arrays using the fixedwriteDelimitedmethod.
103-109: LGTM - Proper exception wrapping for concurrent contextThe
toProtomethod correctly wrapsInterruptedExceptionin a runtime exception, which is necessary for use within the ForkJoinPool's lambda expressions.
113-138: Excellent fix for length-delimited proto serializationThe previous implementation had a critical bug where the length prefix was never actually written, making the output unparseable. The new implementation correctly:
- Writes the varint length prefix using a separate CodedOutputStream
- Writes the message bytes after the prefix
- Properly flushes both streams
This addresses the critical issue identified in previous reviews where
mergeDelimitedFromwould fail.
140-149: LGTM - Exception wrapper provides proper cause accessThe
WrappedIOExceptionclass correctly extends RuntimeException and overridesgetCause()to return the specific IOException type, addressing the concern from previous reviews.
151-160: LGTM - Consistent exception wrapper implementationThe
WrappedInterruptedExceptionclass follows the same pattern asWrappedIOException, providing proper access to the underlying InterruptedException.
|
/refacto-test |
1 similar comment
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Parallel Processing Implementation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| } | ||
| } catch (IOException e) { | ||
| throw new WrappedIOException(e); | ||
| } |
There was a problem hiding this comment.
Unclosed Resources Risk
The method has an extra closing brace that creates a syntax error. This will cause compilation failure and prevent the code from functioning properly.
| } | |
| } catch (IOException e) { | |
| throw new WrappedIOException(e); | |
| } | |
| } catch (IOException e) { | |
| throw new WrappedIOException(e); | |
| } | |
| } |
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness
| } catch (ExecutionException e) { | ||
| // TODO: propagate | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
Exception Handling Gap
The code catches ExecutionException but wraps it in a generic RuntimeException without preserving the original exception type. This loses important error context and makes debugging harder.
| } catch (ExecutionException e) { | |
| // TODO: propagate | |
| throw new RuntimeException(e); | |
| } | |
| } catch (ExecutionException e) { | |
| throw new WrappedExecutionException(e); | |
| } |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- SRE-Error-Propagation
Code Review: Parallel Processing Implementation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| try (ForkJoinPool executor = | ||
| new ForkJoinPool( | ||
| Runtime.getRuntime().availableProcessors(), | ||
| ForkJoinPool.defaultForkJoinWorkerThreadFactory, | ||
| null, | ||
| // we use asyncMode to ensure the queue is processed FIFO, which maximizes | ||
| // throughput | ||
| true)) { |
There was a problem hiding this comment.
Unclosed ForkJoinPool
The ForkJoinPool is created with try-with-resources but the Future's completion is awaited after the try block exits, risking premature pool shutdown before task completion.
ForkJoinPool executor = new ForkJoinPool(
Runtime.getRuntime().availableProcessors(),
ForkJoinPool.defaultForkJoinWorkerThreadFactory,
null,
// we use asyncMode to ensure the queue is processed FIFO, which maximizes
// throughput
true);
try {
var targetQueue = new LinkedBlockingQueue<Future<List<byte[]>>>(MAX_CHUNKS_IN_QUEUE);
var stillAddingTargetsToQueue = new AtomicBoolean(true);
// Rest of the code...
// After all processing is complete:
writeAllTargetsFuture.get();
} finally {
executor.shutdown();
}
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| writeAllTargetsFuture.get(); | ||
| } catch (ExecutionException e) { | ||
| // TODO: propagate | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
Uncaught Exceptions
ExecutionException is caught but wrapped in RuntimeException with a TODO comment. This loses specific error information and prevents proper error handling.
try {
writeAllTargetsFuture.get();
} catch (ExecutionException e) {
Throwable cause = e.getCause();
if (cause instanceof WrappedIOException) {
throw ((WrappedIOException) cause).getCause();
} else if (cause instanceof WrappedInterruptedException) {
throw ((WrappedInterruptedException) cause).getCause();
} else {
throw new IOException("Error processing targets", e);
}
}
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| } catch (ExecutionException e) { | ||
| // TODO: figure out what might be in here and propagate | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
Missing Error Propagation
ExecutionException caught with TODO comment and generic RuntimeException wrapping. Specific error information is lost, preventing proper error handling and recovery.
} catch (ExecutionException e) {
Throwable cause = e.getCause();
if (cause instanceof WrappedIOException) {
throw ((WrappedIOException) cause).getCause();
} else if (cause instanceof WrappedInterruptedException) {
throw ((WrappedInterruptedException) cause).getCause();
} else {
throw new IOException("Failed to process target chunk", e);
}
}
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| } | ||
| } catch (IOException e) { | ||
| throw new WrappedIOException(e); | ||
| } |
There was a problem hiding this comment.
Syntax Error
Syntax error with mismatched curly braces. The extra closing brace on line 135 breaks the method structure, causing compilation failure.
return output;
} catch (IOException e) {
throw new WrappedIOException(e);
}
Standards
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| try (ForkJoinPool executor = | ||
| new ForkJoinPool( | ||
| Runtime.getRuntime().availableProcessors(), | ||
| ForkJoinPool.defaultForkJoinWorkerThreadFactory, | ||
| null, | ||
| // we use asyncMode to ensure the queue is processed FIFO, which maximizes | ||
| // throughput | ||
| true)) { |
There was a problem hiding this comment.
Thread Pool Lifecycle
Creating a new ForkJoinPool for each processOutput call is expensive. Each pool creation allocates thread resources that must be initialized and later destroyed. Under high query volume, this pattern creates significant thread churn and initialization overhead, potentially degrading overall system performance.
// Use a shared thread pool to avoid creation/destruction overhead
private static final ForkJoinPool SHARED_EXECUTOR = new ForkJoinPool(
Runtime.getRuntime().availableProcessors(),
ForkJoinPool.defaultForkJoinWorkerThreadFactory,
null,
// we use asyncMode to ensure the queue is processed FIFO, which maximizes throughput
true);
@Override
public void processOutput(Iterable<Target> partialResult)
throws IOException, InterruptedException {
ForkJoinTask<?> writeAllTargetsFuture;
var targetQueue = new LinkedBlockingQueue<Future<List<byte[]>>>(MAX_CHUNKS_IN_QUEUE);
var stillAddingTargetsToQueue = new AtomicBoolean(true);
writeAllTargetsFuture = SHARED_EXECUTOR.submit(
() -> {
Standards
- ISO-IEC-25010-Performance-Resource-Utilization
- Netflix-Resource-Pooling
- Algorithm-Opt-Thread-Pool
| + targetProtoBuffer.writeTo(bodyOut); | ||
| + bodyOut.flush(); |
There was a problem hiding this comment.
Synchronization Missing
Multiple threads write to the same output stream without synchronization. Concurrent writes to the output stream can cause data corruption as bytes from different threads interleave unpredictably.
// Add synchronization when writing to the output stream
synchronized(out) {
targetProtoBuffer.writeTo(bodyOut);
bodyOut.flush();
}
Standards
- Algorithm-Correctness-Concurrency
- Logic-Verification-Thread-Safety
- Business-Rule-Data-Integrity
| + CodedOutputStream headerOut = CodedOutputStream.newInstance(output, 0, headerSize); | ||
| + headerOut.writeUInt32NoTag(serializedSize); | ||
| + headerOut.flush(); | ||
| + | ||
| + // 2. write the message bytes immediately after the prefix | ||
| + CodedOutputStream bodyOut = | ||
| + CodedOutputStream.newInstance(output, headerSize, serializedSize); | ||
| + targetProtoBuffer.writeTo(bodyOut); | ||
| + bodyOut.flush(); |
There was a problem hiding this comment.
Unclosed Resource Leak
CodedOutputStream instances are not properly closed. The streams implement Closeable interface and should be closed to prevent resource leaks, especially in high-throughput concurrent processing.
// Use try-with-resources to ensure streams are closed properly
try (CodedOutputStream headerOut = CodedOutputStream.newInstance(output, 0, headerSize)) {
headerOut.writeUInt32NoTag(serializedSize);
headerOut.flush();
}
// 2. write the message bytes immediately after the prefix
try (CodedOutputStream bodyOut = CodedOutputStream.newInstance(output, headerSize, serializedSize)) {
targetProtoBuffer.writeTo(bodyOut);
bodyOut.flush();
}
Standards
- Logic-Verification-Resource-Management
- Algorithm-Correctness-Resource-Handling
- Business-Rule-Resource-Cleanup
| } catch (ExecutionException e) { | ||
| // TODO: figure out what might be in here and propagate | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Exception Swallowing Risk
Generic exception handling wraps ExecutionException in RuntimeException without preserving cause details. Attackers could exploit swallowed exceptions to hide malicious activities. Proper exception handling needed for security visibility.
} catch (ExecutionException e) {
Throwable cause = e.getCause();
if (cause instanceof IOException) {
throw new WrappedIOException((IOException) cause);
} else if (cause instanceof InterruptedException) {
throw new WrappedInterruptedException((InterruptedException) cause);
} else {
throw new RuntimeException("Unexpected error during target processing", e);
}
Standards
- CWE-390
- CWE-755
| } catch (ExecutionException e) { | ||
| // TODO: propagate | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
Unhandled Interrupted Exception
ExecutionException handling doesn't restore interrupted status when InterruptedException is the cause. Thread interruption signals could be lost, preventing proper thread termination. Security-sensitive operations might continue after intended shutdown.
} catch (ExecutionException e) {
Throwable cause = e.getCause();
if (cause instanceof InterruptedException) {
Thread.currentThread().interrupt(); // Restore interrupted status
throw new WrappedInterruptedException((InterruptedException) cause);
} else if (cause instanceof IOException) {
throw new WrappedIOException((IOException) cause);
} else {
throw new RuntimeException("Error processing targets", e);
}
Standards
- CWE-391
- CWE-755
| final OutputStream out, final QueryOptions options, LabelPrinter labelPrinter) { | ||
| return new OutputFormatterCallback<Target>() { | ||
| private static final int MAX_CHUNKS_IN_QUEUE = Runtime.getRuntime().availableProcessors() * 2; | ||
| private static final int TARGETS_PER_CHUNK = 500; |
There was a problem hiding this comment.
Magic Number Usage
Magic number 500 lacks explanation for its selection. This hinders maintainability as future developers cannot understand the rationale for this specific batch size value.
// Balance between memory usage and parallelization efficiency
// Experimentally determined optimal value for typical target sizes
private static final int TARGETS_PER_CHUNK = 500;
Standards
- Clean-Code-No-Magic-Numbers
- Clean-Code-Comments
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Concurrency Implementation in StreamedProtoOutputFormatter👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| } | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
Syntax Error
Syntax error in the writeDelimited method. The code has an extra closing bracket after 'return output;' but before the catch block, which will prevent compilation and cause build failures. This breaks the try-catch structure.
return output;
Commitable Suggestion
| } | |
| } catch (IOException e) { | |
| return output; |
Standards
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- ISO-IEC-25010-Reliability-Maturity
| while (stillAddingTargetsToQueue.get() || !targetQueue.isEmpty()) { | ||
| Future<List<byte[]>> targets = targetQueue.take(); | ||
| for (byte[] target : targets.get()) { | ||
| out.write(target); | ||
| } | ||
| } |
There was a problem hiding this comment.
Deadlock Potential
The consumer thread calls targetQueue.take() which blocks until an element is available. If an exception occurs during queue population and stillAddingTargetsToQueue remains true while the queue is empty, the consumer thread will deadlock waiting indefinitely for items that will never arrive.
while (stillAddingTargetsToQueue.get() || !targetQueue.isEmpty()) {
Future<List<byte[]>> targets;
try {
targets = targetQueue.poll(100, TimeUnit.MILLISECONDS);
if (targets == null) {
continue;
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new WrappedInterruptedException(e);
}
for (byte[] target : targets.get()) {
out.write(target);
}
Commitable Suggestion
| while (stillAddingTargetsToQueue.get() || !targetQueue.isEmpty()) { | |
| Future<List<byte[]>> targets = targetQueue.take(); | |
| for (byte[] target : targets.get()) { | |
| out.write(target); | |
| } | |
| } | |
| while (stillAddingTargetsToQueue.get() || !targetQueue.isEmpty()) { | |
| Future<List<byte[]>> targets; | |
| try { | |
| targets = targetQueue.poll(100, TimeUnit.MILLISECONDS); | |
| if (targets == null) { | |
| continue; | |
| } | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| throw new WrappedInterruptedException(e); | |
| } | |
| for (byte[] target : targets.get()) { | |
| out.write(target); | |
| } |
Standards
- Algorithm-Correctness-Concurrency
- Logic-Verification-Deadlock-Prevention
| try (ForkJoinPool executor = | ||
| new ForkJoinPool( | ||
| Runtime.getRuntime().availableProcessors(), | ||
| ForkJoinPool.defaultForkJoinWorkerThreadFactory, | ||
| null, | ||
| // we use asyncMode to ensure the queue is processed FIFO, which maximizes | ||
| // throughput | ||
| true)) { |
There was a problem hiding this comment.
Resource Leak Risk
The ForkJoinTask created inside the try-with-resources block continues execution after the ForkJoinPool is closed. This creates a resource leak risk as tasks may attempt to use the shutdown executor, potentially causing RejectedExecutionException or task failures.
ForkJoinPool executor = new ForkJoinPool(
Runtime.getRuntime().availableProcessors(),
ForkJoinPool.defaultForkJoinWorkerThreadFactory,
null,
// we use asyncMode to ensure the queue is processed FIFO, which maximizes
// throughput
true);
try {
Commitable Suggestion
| try (ForkJoinPool executor = | |
| new ForkJoinPool( | |
| Runtime.getRuntime().availableProcessors(), | |
| ForkJoinPool.defaultForkJoinWorkerThreadFactory, | |
| null, | |
| // we use asyncMode to ensure the queue is processed FIFO, which maximizes | |
| // throughput | |
| true)) { | |
| ForkJoinPool executor = new ForkJoinPool( | |
| Runtime.getRuntime().availableProcessors(), | |
| ForkJoinPool.defaultForkJoinWorkerThreadFactory, | |
| null, | |
| // we use asyncMode to ensure the queue is processed FIFO, which maximizes | |
| // throughput | |
| true); | |
| try { |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Resource-Management
| } catch (ExecutionException e) { | ||
| // TODO: figure out what might be in here and propagate | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
Uncaught RuntimeException Propagation
Unresolved TODO with generic RuntimeException wrapping can mask actual failure causes. The code throws a generic RuntimeException without proper cause analysis, potentially hiding important error information and making debugging difficult in production.
} catch (ExecutionException e) {
Throwable cause = e.getCause();
if (cause instanceof IOException) {
throw new WrappedIOException((IOException) cause);
} else if (cause instanceof InterruptedException) {
throw new WrappedInterruptedException((InterruptedException) cause);
} else {
throw new RuntimeException("Error processing target data", e);
}
}
Commitable Suggestion
| } catch (ExecutionException e) { | |
| // TODO: figure out what might be in here and propagate | |
| throw new RuntimeException(e); | |
| } | |
| } catch (ExecutionException e) { | |
| Throwable cause = e.getCause(); | |
| if (cause instanceof IOException) { | |
| throw new WrappedIOException((IOException) cause); | |
| } else if (cause instanceof InterruptedException) { | |
| throw new WrappedInterruptedException((InterruptedException) cause); | |
| } else { | |
| throw new RuntimeException("Error processing target data", e); | |
| } | |
| } |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Error-Handling
| } catch (ExecutionException e) { | ||
| // TODO: propagate | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
Unhandled ExecutionException Propagation
Another unresolved TODO with generic RuntimeException wrapping. The code fails to properly analyze and handle the underlying cause of ExecutionException, which could contain important failure information needed for proper error handling and recovery.
} catch (ExecutionException e) {
Throwable cause = e.getCause();
if (cause instanceof WrappedIOException) {
throw ((WrappedIOException) cause).getCause();
} else if (cause instanceof WrappedInterruptedException) {
Thread.currentThread().interrupt();
throw ((WrappedInterruptedException) cause).getCause();
} else {
throw new IOException("Error processing query results", e);
}
} finally {
executor.shutdown();
}
Commitable Suggestion
| } catch (ExecutionException e) { | |
| // TODO: propagate | |
| throw new RuntimeException(e); | |
| } | |
| } catch (ExecutionException e) { | |
| Throwable cause = e.getCause(); | |
| if (cause instanceof WrappedIOException) { | |
| throw ((WrappedIOException) cause).getCause(); | |
| } else if (cause instanceof WrappedInterruptedException) { | |
| Thread.currentThread().interrupt(); | |
| throw ((WrappedInterruptedException) cause).getCause(); | |
| } else { | |
| throw new IOException("Error processing query results", e); | |
| } | |
| } finally { | |
| executor.shutdown(); | |
| } |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Error-Handling
| } catch (InterruptedException e) { | ||
| throw new WrappedInterruptedException(e); | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
Thread Interruption Handling
InterruptedException is caught but the thread's interrupted status is not restored. When wrapping InterruptedException, the current thread's interrupt status should be preserved with Thread.currentThread().interrupt() to ensure proper interruption propagation.
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new WrappedInterruptedException(e);
Commitable Suggestion
| } catch (InterruptedException e) { | |
| throw new WrappedInterruptedException(e); | |
| } catch (IOException e) { | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| throw new WrappedInterruptedException(e); | |
| } catch (IOException e) { |
Standards
- ISO-IEC-25010-Reliability-Maturity
- SRE-Error-Handling
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| try (ForkJoinPool executor = | ||
| new ForkJoinPool( | ||
| Runtime.getRuntime().availableProcessors(), | ||
| ForkJoinPool.defaultForkJoinWorkerThreadFactory, | ||
| null, | ||
| // we use asyncMode to ensure the queue is processed FIFO, which maximizes | ||
| // throughput | ||
| true)) { |
There was a problem hiding this comment.
Resource Leak Risk
ForkJoinPool is created with no exception handler (null parameter). Uncaught exceptions in worker threads may cause thread termination without proper cleanup, potentially reducing parallelism effectiveness under load and causing resource leaks.
Standards
- ISO-IEC-25010-Performance-Resource-Utilization
- Concurrency-Pattern-Exception-Handling
- Thread-Pool-Management
| + CodedOutputStream headerOut = CodedOutputStream.newInstance(output, 0, headerSize); | ||
| + headerOut.writeUInt32NoTag(serializedSize); | ||
| + headerOut.flush(); | ||
| + | ||
| + // 2. write the message bytes immediately after the prefix | ||
| + CodedOutputStream bodyOut = | ||
| + CodedOutputStream.newInstance(output, headerSize, serializedSize); | ||
| + targetProtoBuffer.writeTo(bodyOut); | ||
| + bodyOut.flush(); |
There was a problem hiding this comment.
Redundant CodedOutputStream Creation
Creating separate CodedOutputStream instances for header and body adds overhead. A single CodedOutputStream with appropriate positioning would be more efficient, reducing object creation and improving serialization performance under high throughput.
Standards
- ISO-IEC-25010-Performance-Resource-Utilization
- Optimization-Pattern-Object-Reuse
- Memory-Allocation-Efficiency
| try { | ||
| writeAllTargetsFuture.get(); | ||
| } catch (ExecutionException e) { | ||
| // TODO: propagate | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
Unhandled Interrupted Exception
The code catches ExecutionException but doesn't handle InterruptedException from Future.get(). Thread interruption signals are lost, potentially causing thread leaks and preventing proper application shutdown. This can lead to resource exhaustion in long-running applications.
Standards
- CWE-391
- OWASP-A06
- NIST-SSDF-PW.1
| // we use asyncMode to ensure the queue is processed FIFO, which maximizes | ||
| // throughput | ||
| true)) { | ||
| var targetQueue = new LinkedBlockingQueue<Future<List<byte[]>>>(MAX_CHUNKS_IN_QUEUE); |
There was a problem hiding this comment.
Resource Exhaustion Risk
The queue has a bounded size but no handling for queue full conditions. If producers outpace consumers, the put() operation will block indefinitely, potentially causing thread starvation or deadlock. Consider adding timeout mechanisms and graceful degradation when queue capacity is reached.
Standards
- CWE-400
- OWASP-A05
- NIST-SSDF-PW.8
| private static class WrappedIOException extends RuntimeException { | ||
| private WrappedIOException(IOException cause) { | ||
| super(cause); | ||
| } | ||
|
|
||
| @Override | ||
| public IOException getCause() { | ||
| return (IOException) super.getCause(); | ||
| } | ||
| } | ||
|
|
||
| private static class WrappedInterruptedException extends RuntimeException { | ||
| private WrappedInterruptedException(InterruptedException cause) { | ||
| super(cause); | ||
| } | ||
|
|
||
| @Override | ||
| public InterruptedException getCause() { | ||
| return (InterruptedException) super.getCause(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Wrapper Classes Duplication
Two nearly identical wrapper exception classes follow the same pattern with duplicated code structure. This violates DRY principle and increases maintenance burden. A generic wrapper with type parameter or unified exception handling strategy would be more maintainable.
Standards
- Clean-Code-DRY
- SOLID-DRY
- Maintainability-Quality-Duplication
| final OutputStream out, final QueryOptions options, LabelPrinter labelPrinter) { | ||
| return new OutputFormatterCallback<Target>() { | ||
| private static final int MAX_CHUNKS_IN_QUEUE = Runtime.getRuntime().availableProcessors() * 2; |
There was a problem hiding this comment.
Nested Anonymous Classes
Complex parallel processing logic is implemented within an anonymous inner class. This creates maintainability issues as the code is deeply nested and tightly coupled to its parent class. Extracting this to a named class would improve readability and testability.
Standards
- Clean-Code-Class-Organization
- Maintainability-Quality-Nesting
- Clean-Code-Functions
| @@ -34,13 +42,120 @@ public String getName() { | |||
| public OutputFormatterCallback<Target> createPostFactoStreamCallback( | |||
| final OutputStream out, final QueryOptions options, LabelPrinter labelPrinter) { | |||
| return new OutputFormatterCallback<Target>() { | |||
| private static final int MAX_CHUNKS_IN_QUEUE = Runtime.getRuntime().availableProcessors() * 2; | |||
There was a problem hiding this comment.
Magic Numbers Usage
The code uses a magic number (500) for chunk size without explanation of why this value was chosen. This reduces maintainability as future developers won't understand the reasoning behind this specific value or know when it should be adjusted.
Standards
- Clean-Code-Naming
- Maintainability-Quality-Constants
- Clean-Code-Comments
| } | ||
| } catch (IOException e) { | ||
| throw new WrappedIOException(e); | ||
| } |
There was a problem hiding this comment.
Broken Exception Chain
The writeDelimited method has mismatched braces causing a logical syntax error. The extra closing brace at line 135 will prevent compilation, and the exception handling block at lines 136-137 is improperly structured, breaking the intended error propagation flow.
Standards
- Algorithm-Correctness-Syntax
- Logic-Verification-Error-Handling
| private List<byte[]> writeTargetsDelimitedToByteArrays(List<Target> targets) { | ||
| return targets.stream().map(target -> writeDelimited(toProto(target))).toList(); |
There was a problem hiding this comment.
Memory Exhaustion Risk
The method processes all targets in memory without streaming or chunking large outputs. For large target lists, this could cause excessive memory consumption leading to OutOfMemoryError. Consider implementing true streaming with backpressure to prevent memory exhaustion.
Standards
- CWE-400
- OWASP-A05
- NIST-SSDF-PW.8
3 participants
This is a proposed fix for bazelbuild#24304
This speeds up a fully warm
bazel query ...by 54%, reducing wall time from 1m49s to 50sCurrent state:
This PR:
💁♂️ Note: when combined with bazelbuild#24298, total wall time is 37s, an overall reduction of 66%.
Summary by CodeRabbit