(perf): measure legend once - #724
Merged
Merged
Conversation
Every legend was measured twice. draw_legend() measures once during setup so par(oma) can be sized before the plot is drawn, and tinylegend() measured again inside recordGraphics() because the device may have been resized. On the initial draw nothing has changed between the two, so the second pass re-derived a result it already had, at roughly the cost of drawing the legend itself. measure_fake_legend() now records the device it measured on, and tinylegend() re-measures only when dev.size() no longer matches. Resize correctness is preserved by construction rather than by trying to detect a replay: a resized device fails the comparison and measures again. Multi-legend is unaffected, since each draw_legend() call builds its own legend_env and so carries its own stamp. Measuring passes per plot drop from 2 to 1 (6 to 4 for multi-legend). Plots that draw a legend are ~5% faster overall and ~8% for the lighter ones (points_small_by 5.58ms -> 5.13ms); plots without a legend are untouched. Full suite passes, 729/729, with NOT_CRAN=true. Rendering was compared against main across 8 configurations replayed through a sequence of device sizes (shrink, grow, back to the original, then the same size again), covering outer and inner legends, facets, gradient, multi-legend, ljust = "center" and a theme: byte-identical in all 40 comparisons. Claude-Session: https://claude.ai/code/session_01AjZRHA9ZvJkDPMFdDXUcUs
The device-size guard added in the previous commit calls dev.size() without declaring it, which R CMD check flags as an undefined global. Add it to the existing grDevices importFrom tag in legend.R and regenerate NAMESPACE. R CMD check is clean again (Status: OK). Claude-Session: https://claude.ai/code/session_01AjZRHA9ZvJkDPMFdDXUcUs
There was a problem hiding this comment.
🟡 Changes recommended
The cache can reuse invalid measurements when replayed onto a different same-sized graphics device.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Optimizes legend rendering by caching measurements until the graphics device size changes.
Changes:
- Cache legend dimensions across initial rendering.
- Track the device size used for measurement.
- Update imports and release notes.
File summaries
| File | Description |
|---|---|
R/legend.R |
Adds legend measurement caching. |
NAMESPACE |
Imports dev.size. |
NEWS.md |
References the related performance PR. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
dev.size() alone does not identify a device. Replaying a recorded plot onto a different backend that happens to have the same dimensions (text metrics differ by backend) would reuse the source device's measurement, leaving outer margins and gradient swatches mis-sized. Include dev.cur(), which is a named integer and so carries the backend as well as the device number. Demonstrated by recording on pdf and replaying onto svglite at a matching 8x8: main's replay matches a native draw on the destination, the previous key's did not. With the device in the key it matches again. Note this is a narrow hole rather than a common one: the everyday export path, dev.copy2pdf(), re-measures under the old key anyway. It needs an explicitly same-sized replay onto another backend. The guard costs one dev.cur() call and does not move the benchmarks, so it is cheap insurance. Also fixes the changelog typo flagged in review. Spotted in review by Copilot on #724. Claude-Session: https://claude.ai/code/session_01AjZRHA9ZvJkDPMFdDXUcUs
This file contains hidden or 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
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.
Some more Claude-assisted profiling golf. In short; avoiding remeasuring the legend unless the device resizes. Ekes out another ~5-8%.