web: multithread and optimize save_image rendering - #11239
Conversation
- Parallelize tile rendering and compositing in renderImageBuffer using utl::ThreadPool. - Parallelize nearest-neighbor image resampling with precomputed coordinate maps. - Implement 64-byte block copy, solid span filling, and transparency check (anyNonZero). - Use fast fixed-point arithmetic for Porter-Duff alpha compositing. - Add row-level transparency early-exit in Lanczos-2 decimation. - Thread count is propagated from WebServer to TileGenerator, respecting ord::set_thread_count. SaveImageTest.LabelsFollowTheVisibilityFlag performance (4096x3589 px) vs baseline: | Configuration | Time (ms) | Speedup vs Baseline | |---------------|-----------|---------------------| | Baseline | 3445 | 1.00x | | 1 thread | 323 | 10.67x | | 4 threads | 116 | 29.70x | | 16 threads | 58 | 59.40x | Signed-off-by: Drew Lewis <cannada@google.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces multi-threaded tile rendering and various pixel-level optimizations to the TileGenerator and WebServer classes, including fast-path transparency checks, optimized alpha-compositing, and parallelized resampling. The review feedback provides valuable recommendations to improve the robustness and portability of these changes. Specifically, it suggests ensuring stable Z-ordering when layers are skipped, replacing compiler-specific inline attributes with standard C++ inline, handling arbitrary span sizes in fillSpan, marking buffers dirty early to handle early returns safely, and guarding against potential floating-point overflow during downsampling when the accumulated alpha is extremely small.
| static thread_local bool super_buffer_dirty = false; | ||
| if (super_buffer.size() != static_cast<size_t>(super_buffer_size)) { | ||
| super_buffer.assign(super_buffer_size, 0); | ||
| super_buffer_dirty = false; | ||
| } else if (super_buffer_dirty) { | ||
| std::memset(super_buffer.data(), 0, super_buffer_size); | ||
| super_buffer_dirty = false; | ||
| } |
There was a problem hiding this comment.
Setting super_buffer_dirty = true at the very end of renderTileBuffer is risky because any early return or exception thrown during the rendering loop could bypass this assignment, leaving the thread-local super_buffer dirty without marking it as such. Setting super_buffer_dirty = true immediately after clearing/initializing the buffer at the start of the function guarantees that the buffer is always marked dirty for the next run, regardless of how the current run exits.
static thread_local bool super_buffer_dirty = false;
if (super_buffer.size() != static_cast<size_t>(super_buffer_size)) {
super_buffer.assign(super_buffer_size, 0);
} else if (super_buffer_dirty) {
std::memset(super_buffer.data(), 0, super_buffer_size);
}
super_buffer_dirty = true;
This is the image for nangate45 aes to show another diff. |
Address review comments from PR The-OpenROAD-Project#11239: - Ensure deterministic Z-ordering in saveImageLayerOrder across visibility filters by advancing layer_z for all tech layers. - Replace compiler-specific __attribute__((always_inline)) with standard C++ inline. - Handle non-multiple-of-4 byte spans defensively in fillSpan. - Set super_buffer_dirty at buffer initialization for exception/early-return safety. - Guard against floating-point overflow in lanczos2Downsample when alpha is near zero (< 0.001f). Signed-off-by: Drew Lewis <cannada@google.com>
Simplify thread-local super_buffer management in renderTileBuffer by unconditionally zeroing the buffer with std::memset, eliminating the redundant super_buffer_dirty state flag. Signed-off-by: Drew Lewis <cannada@google.com>
AES on ASAP7 for fun after fixing the Gemini requests. |
- Use ThreadPoolFuture::get() rather than wait() when joining the tile render and resample workers: wait() never rethrows, so an exception in a worker silently produced a partial image with a clean exit. - Index the tile-span buffers with size_t. kMaxDim clamps the final image, but the tile span can be ~2x larger, and the int products overflowed at the 16k cap. - Zero Lanczos output pixels whose alpha rounds to 0 (ai < 0.5) instead of emitting (255,255,255,0), which defeated the transparent-buffer early exits downstream. - Drop dead code in compositePixel (out_a can never be 0; the 16.16 lerp cannot leave [d, s]) and the unused <bit> include; make fillSpan assert whole pixels instead of writing a partial one. - Add tests: saveImageLayerOrder honors visible_layers for tech layers, hidden tech layers are not drawn, and the rendered image is identical across thread counts. Signed-off-by: Drew Lewis <cannada@google.com>
No behavior change. - compositeTile(): one implementation of the sparse tile-onto-output composite, replacing the duplicated layer and label loops. The per-pixel copy shortcut is dropped: compositePixel already starts with the same test. - parallelRanges(): one implementation of the contiguous-range fan-out (with exception propagation) used by both the tile render and the resample passes. The pool is no longer created for a single tile. - WebServer::ensureGenerator() creates the TileGenerator and pushes the thread count once, replacing four creation sites and the per-call setThreadCount() pushes. - Naming/comments: hw_threads -> num_threads (it is the configured count), the Lanczos lambda is unpremult (not blend), the anyNonZero comment no longer claims a SIMD path, and the static_assert on Color's layout sits with the memcpy helpers that rely on it. Signed-off-by: Drew Lewis <cannada@google.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b18a97b821
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
From Claude: 5.6x on 16 threads is worth calling out in review. Two likely causes: parallelRanges splits tiles into static contiguous ranges, so with sparse designs a thread that draws the dense core does all the work while the border-tile threads finish instantly — the early exits made the imbalance worse, not better. And what's left is largely memory-bandwidth-bound (~59 MB output buffer) plus the serial crop/encode tail. A dynamic or interleaved tile assignment would likely recover a good chunk of that. |
|
Nice. Some of this should carry over the the tile rendering from the client (the single-threaded optimizations). |
If a worker task in parallelRanges threw an exception, the previous loop exited immediately on the first future get() failure. During stack unwinding, the local stack lambda (e.g. render_tiles or resample_rows) was destroyed before the thread pool. As the pool destructor drained the task queue, remaining worker tasks could invoke the captured reference to the destroyed lambda, causing undefined behavior. Collect the first exception while continuing to call get() on every future so all workers finish executing before rethrowing the exception. Signed-off-by: Drew Lewis <cannada@google.com>
Was this a request to work on that or just a comment. TBH if we want to go faster for image generation I think threading the PNG writer (you can apparently parallelize the deflate part) is the next biggest win on typical images, but I think we've hit the point of diminishing returns, or at least my willingness to do more optimizations to this code. My main reason for this PR was making an image at the end of our flow was taking 10-15s. For small modules (<10k cells) this was taking more time than the gpl or the resizer calls. |
|
Just a comment in case you were looking for more improvement. |
2 participants
Summary
SaveImageTest.LabelsFollowTheVisibilityFlag performance (4096x3589 px) vs baseline:
Type of Change
Performance improvement
Impact
Makes it run faster
Verification
./etc/Build.sh).I also attached two diffs I generated from the test images.