web: cut tile server CPU per frame roughly in half - #11347
Conversation
A viewport frame asks the tile server for one tile per layer per grid square -- 1280 of them for a 32-layer design at 1920x1080 -- and PNG encoding, not rasterization, is where that time goes: 1060 ms of a 1729 ms single-threaded frame on coralnpu/asap7 after global routing. Encode a tile from a palette when it holds 256 colours or fewer. Almost every tile does: a layer with nothing in view is one transparent colour, and a layer with something is its own colour at a few alpha levels. lodepng then filters and deflates one byte per pixel instead of four. Richer images fall through to RGBA, having paid only for a scan that stops at the 257th colour. Share one encoding for the fully transparent tile. Its bytes depend only on its size, and 23 of the 32 layers had nothing in view. Resolve each font size's kerning pairs once instead of per lookup. stbtt_GetCodepointKernAdvance searches the cmap twice and walks the GPOS table, and textWidth() asks for a pair between every adjacent pair of characters, so measuring a label cost about as much as drawing it. Round both edges of a heat-map bin the same way. Bins tile the plane, but toPixels() rounds outward, so the pixels on a shared edge went to both neighbours and were composited twice -- a lattice of darker seams over every map. It also mixed pairs of ramp entries into thousands of distinct colours per tile, which kept heat maps off the palette path above: RUDY tiles now hold 48 colours where they held 772, and their PNGs are 4.1 kB where they were 22.4 kB. Server CPU for one viewport frame drops from 1762 ms to 892 ms. Layer tiles are pixel-identical before and after. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Most of the tiles in a viewport are blank -- 1092 of 1280 on coralnpu at a fit-to-design zoom, 725 of 960 on bp_multi -- because a ring of tile positions covers only the bounds margin, the via layers are below the sub-resolution cull, and some tech layers hold no geometry at all. They were served as a fully transparent PNG. That is ~100 bytes on the wire but it still decodes to (256*dpr)^2 * 4 bytes on the client, and decoded image bytes are exactly what tile-merge.js is rationing: it merges panes to stay under a 350 MB budget because Chrome starts discarding decoded images somewhere around 458 MB and the discarded regions paint white. At dpr 1 the blank tiles alone were 286 MB of that budget on coralnpu and 190 MB on bp_multi, held for images with nothing in them. Add payload type 3 to the wire protocol: no body, meaning the renderer drew nothing. The layer, overlay and heat-map handlers return it in place of a transparent PNG, and the tile cache stores those entries empty so a blank tile costs an LRU slot rather than the bytes of an image nobody decodes. Detection is exact rather than heuristic -- a blank tile is encoded to one shared buffer per tile size, so equality with it means the renderer drew nothing. On the client a type-3 payload resolves to null. The merged tile path already treated a null payload as "draw nothing", so it needed no change; the <img> path now clears the element and completes the tile itself, because Leaflet keeps a tile hidden until done() is called and an <img> with no src never fires the load event that normally calls it. Clearing matters on a refresh too: a tile that had content before an edit removed it must drop the decode it is still holding. An unrecognised payload type also resolves rather than leaving the tile pending forever, so a newer server cannot hang an older client's map. The handler tests that asserted a tile always comes back as a PNG were asserting it of layers that are in fact blank in the fixture; they now either point at a layer that draws or assert the empty response, which in the flywire cases is the more direct evidence that nothing was drawn. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
A viewport is one request per layer per grid square, so tile responses go out in bursts of hundreds, and most of them are small -- a blank tile now carries no payload at all. Nagle holds a small segment until the previous one is acknowledged, so the first reply of a burst waits on the client's delayed ACK and the rest of the burst queues behind it. The stall is per burst rather than per response, which is why it hid: it cost the same ~40 ms whether the burst was 40 requests or 200. Measured over loopback on coralnpu, a 32-layer viewport at 1920x1080: cold frame, wall 1799 ms -> 858 ms frame served from the tile cache 1274 ms -> 19.9 ms The cold figure is now equal to the 862 ms of CPU the same frame costs, so what is left is work rather than waiting. Not covered by a test: the session tests drive the handlers directly and there is no socket-level rig to assert a socket option through. A failure to set it is logged under the websocket debug key rather than refusing the connection, since the session still works, just with the stall. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
There was a problem hiding this comment.
Code Review
This pull request introduces significant performance optimizations for tile rendering and transmission. It adds support for a new empty tile response type (type 3, or kEmpty) to avoid sending, decoding, and caching fully transparent PNGs on the client. It also implements indexed PNG encoding for tiles with few colors, caches blank PNGs, and optimizes font kerning lookups in the glyph cache. Additionally, Nagle's algorithm is disabled on the WebSocket socket to reduce latency during bursty tile transmissions. The review comments suggest minor improvements: reserving capacity for the palette vector to avoid reallocations, and logging a warning when an unrecognized payload type is received.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 146a0fd2e7
ℹ️ 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".
Review feedback on The-OpenROAD-Project#11347. Reserve the palette up front. It is bounded by kMaxPaletteColors and it grows inside the per-pixel loop, so this trades a handful of reallocations for one. Report an unrecognised payload type rather than treating it as empty in silence, which hid a protocol mismatch behind a viewport that merely looked blank. Once per type rather than once per message: the cause is a version skew, so every tile arrives the same way and a warning each would bury the first one. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Review feedback on The-OpenROAD-Project#11347. Only payload type 3 carries "blank tile", but this manager is shared with the JSON endpoints, and resolving null for an unrecognised type handed that null to them as if it were data -- `tech` and `bounds` dereference the reply directly, so a protocol mismatch surfaced as a null dereference or as silently missing data rather than as an error. Reject instead. The tile callers already catch a dropped request: the merged path paints the tile blank and the <img> path leaves it for Leaflet to evict, which is what they do for a cancelled request too. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Review feedback on The-OpenROAD-Project#11347. The heat map has its own tile layer in main.js, which the empty response skipped past: both its request paths hand the payload straight to URL.createObjectURL(), so a null threw and the tile only recovered through the catch that substitutes BLANK_TILE. Take the null explicitly instead, in both paths -- and on a refresh that also has to replace whatever image the tile was holding, since a bin can empty out under an edit. BLANK_TILE rather than a cleared src: it is the 1x1 the layer already uses when no heat map is selected, it decodes to nothing, and its load event completes the tile. generateHeatMapTile() returned no bytes at all for coordinates off the tile grid, which predates the empty response and was already wrong: the handler puts that behind a PNG frame, leaving the client a zero-length body it can only fail to decode. Return the blank encoding, as the other tile entry points do, and the handler turns it into an empty response from there. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
|
Both fixed in
Added |
Clamp a heat-map bin's pixel edges before the cast. A bin is converted whole rather than clipped to the tile first -- that is what keeps the lattice identical in every tile the bin crosses -- so its edge in tile pixels grows with zoom without bound and eventually passes what an int holds. Casting that is undefined, and what it did was wrap to a negative span and drop the bin: the map went blank exactly where it was most magnified. toPxX() already clamps for this reason; do the same here. Report the lodepng error text again. encodeImagePng() collapsed every failure to an empty vector, which left the three callers able to say only that an encode had failed -- including WEB-23, the one an operator sees. It now hands back the code behind an empty result. Filter the static report's blank images through isBlankTilePng() instead of a byte threshold. The threshold was derived for a 256 px transparent tile, which is exactly 102 bytes, and the timing-path overlays are 512 px, where a blank one is 125: a path whose shapes all fall outside the die area rendered to a blank image the report then carried. A path with no shapes at all already rendered to no bytes and was unaffected. The exact test does not have to be re-derived when the encoder changes. Bound a label's font height. It reaches GlyphCache, which rasterizes all 95 printable ASCII glyphs at that height and holds them for the life of the process, and it is the one font height a caller sets directly -- every other is a constant scaled by the quantized device pixel ratio. Applied in addLabel/updateLabel rather than at either entry point, so the Tcl command and the websocket request are bounded by the same line and labelsJson() reports back the size that will be drawn. Also: assert Color is the four bytes the indexed path packs a pixel into and unpacks it from; move markEmptyIfBlank and heatMapBinSpan into their files' anonymous namespaces; release the object URL on the heat map layer's "no active map" refresh path, which walks every tile on screen; and document overlay_tile and the dpr/tile_px sizing fields shared by the three tile endpoints, none of which had an entry in server-api.md. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
tile_merge_test failed under `bazel test //src/web/...`, which runs 46 test binaries at once: "issues the requests concurrently, not one after another" issued three 40 ms requests and asserted the whole render finished inside 100 ms, and on a machine that busy the three timers took 105 ms. Timing the render only inferred concurrency anyway. Assert the property itself instead -- every request is in flight before any of them answers -- by holding each one open on a promise the test resolves. With all three held, a serialised implementation could not have issued the second, and nothing about the machine's load can change that. The five ordering tests around it were staged the same way, with delays whose only job was to fix an arrival order: two arrivals 5 ms apart can land either way round under the same load. They now open the gates in the order they mean. The waits become one setImmediate, which fires after the microtask queue has drained and so settles the request -> decode -> paint chain however deep it is. Also drop zeroRGBA, which has had no caller since save_image was multithreaded and warns as unused. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Opening a design drew a grey grid over the layout, one cell per tile. An empty tile (payload type 3) cleared its element's src attribute to say "hold no image". An <img> with no src but a CSS width and height is not nothing to the browser, though: Chrome paints the empty replaced element, and since most tiles in a viewport are empty -- which is the whole reason the empty response exists -- what it painted was the tile grid. Point those tiles at a 1x1 transparent GIF instead. That is a 4-byte decode, so the response still saves what it is meant to: no tile-sized transparent PNG decoded, no tile-sized bitmap held against the budget in tile-merge.js. It also restores the ordinary completion path -- the load event calls done() -- so the _orDone plumbing added to work around a tile that never loads goes away with it. The blank tile main.js already had was not transparent. A 1x1 GIF only is if it carries a Graphic Control Extension saying so, and the widely pasted one has no such block: its single opaque pixel stretches over the tile, so substituting it turned the layout white rather than grey. It reached the layout through the empty heat-map tiles handled two commits ago. The constant moves to tile-request.js, transparent, shared by both layers, with a test that decodes the data URI and checks for the extension rather than trusting the string to look right. Found by opening gcd in the viewer and hiding the src-less tiles, which took the grid with them. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Three changes to the web viewer's tile path, plus a heat-map rendering fix
that fell out of investigating them.
Measured on
coralnpu(asap7, post-GRT, 174k instances) at a fit-to-designzoom: a 32-layer viewport is 1280 tile requests.
For reference the Qt GUI renders the same frame in 362 ms, so the gap
narrows from ~4.9x to ~2.4x. Confirmed on
bp_multi(nangate45,post-detailed-route, 278k instances) as well: 1587 -> 1020 ms CPU, where
the gap was already smaller because Qt's cost tracks shape count while the
tile server's tracks layers x tiles.
1.
dcdf069711— encode cost, kerning, heat-map bin seamsPNG encoding, not rasterization, was where the frame went: 1060 ms of it.
Tiles are nearly always encodable from a palette — a layer with nothing in
view is one transparent colour, and a layer with something is its own
colour at a few alpha levels — so lodepng now gets one byte per pixel
instead of four, and the fully transparent tile is encoded once and shared.
Kerning is resolved once per font size instead of per lookup;
stbtt_GetCodepointKernAdvancesearches the cmap twice and walks the GPOStable, and
textWidth()asks for a pair between every adjacent pair ofcharacters.
Separately, heat-map bins were rounding both edges outward, so the pixels
on a shared edge went to both neighbouring bins and were composited twice.
That drew a lattice of darker seams over every map, and mixing pairs of
ramp entries pushed a tile's colour count into the thousands where the ramp
holds 256. RUDY tiles now hold 48 distinct colours where they held 772, and
their PNGs are 4.1 kB where they were 22.4 kB.
Layer tiles are pixel-identical before and after: 280/280 on coralnpu,
960/960 on bp_multi.
2.
9ae2c083da— blank tiles as an empty responseMost tiles in a viewport are blank: 1092 of 1280 here, 725 of 960 on
bp_multi. A ring of tile positions covers only the bounds margin, the via
layers sit below the sub-resolution cull, and some tech layers hold no
geometry at all.
They were served as a transparent PNG — ~100 bytes on the wire, but still a
(256*dpr)^2 * 4byte decode on the client. Decoded image bytes are whattile-merge.jsis rationing: it merges panes to stay under a 350 MB budgetbecause Chrome starts discarding decoded images around 458 MB and the
discarded regions paint white. At dpr 1 the blank tiles alone were 286 MB
of that budget on coralnpu, held for images with nothing in them.
Adds payload type 3 to the wire protocol (no body). The merged tile path
already treated a null payload as "draw nothing" and needed no change; the
<img>path clears the element and completes the tile itself, sinceLeaflet keeps a tile hidden until
done()and an<img>with nosrcnever fires the load event that would call it.
3.
146a0fd2e7— disable Nagle on the tile socketTile responses go out in bursts of hundreds and most are small. Nagle held
the first small segment of a burst until the client's delayed ACK arrived
and the rest queued behind it — a ~40 ms stall per burst regardless of its
size, which is why it hid. Cold frame wall time 1799 -> 858 ms, which now
equals the CPU the same frame costs.
Not included
Two further optimisations were implemented, measured, and dropped because
they were slower: a single-RGB fast path for the palette scan (867 -> 890
ms) and a flattened, branch-free Lanczos tap table (857 -> 920 ms, though
verified bit-exact over 960 tiles). The
pa == 0skip in the resampler isdoing more work than vectorising the loop would recover, on sparse tiles.
Compositing
save_imageinto one supersampled buffer per tile is not soundas it stands:
drawFilledRect's solid path overwrites rather than blends,which is safe only because each layer gets its own scratch buffer. Doing it
properly needs a super-resolution accumulator each layer blends into.