(perf:) Miscellaneous performance improvements - #723
Merged
Conversation
get_tpar() fell back to suppressWarnings(par(o)) for any name absent from .tpar. Most tpar parameters are tinyplot's own (grid.bg, palette, lty.xaxs, x/yaxr, ...), which base par() does not recognise, so it warned on each one and we paid to raise and immediately suppress that warning. A miss cost ~50us against ~13us for a real lookup, and a single plot takes that path around a dozen times. Gate the par() call behind the set of names base R actually knows, so unknown names fall straight through to the next candidate or the default. Same result, no warning round trip. The name set is cached in .tinyplot_env on first use rather than at load time: par() needs an open device and would otherwise open one as a side effect during .onLoad, writing a stray Rplots.pdf. The set does not vary by device (identical across pdf, png, svglite and postscript), so one per-session cache is safe. Benchmarked against v0.7.0 across 28 canonical plot calls: development HEAD was ~6% slower than the release, and is now ~9% faster. 300 plots in succession drop from 2130ms to 1777ms. Full suite passes, 729/729, with NOT_CRAN=true so snapshots ran. Claude-Session: https://claude.ai/code/session_01AjZRHA9ZvJkDPMFdDXUcUs
The assert_* helpers composed their error message with sprintf() before running the check, so every passing assertion paid for a string it then threw away. Passing is the overwhelmingly common case: assert_tpar() alone runs about 30 assertions on every plot. Move each sprintf() inside the failure branch. This also defers the `name = as.character(substitute(x))` default, which is only referenced when composing the message, so callers that do not pass `name` explicitly no longer pay for substitute() on the happy path. Messages are unchanged. All 22 error paths across the touched helpers were compared before and after, including the substitute()-derived names, and are byte-identical. assert_tpar() drops from ~99us to ~77us (-22%) and assert_numeric() from ~3.6us to ~2.9us (-19%). End to end this is a modest ~0.6% on plot time, since the helpers are a small share of it; the 300-plot succession goes from 1777ms to 1737ms. Full suite passes, 729/729, with NOT_CRAN=true so snapshots ran. Claude-Session: https://claude.ai/code/session_01AjZRHA9ZvJkDPMFdDXUcUs
env2env() assigned key by key. A single plot moves a few hundred keys across roughly 20 calls, and the per-key assign() dominates: swapping the loop for mget() + list2env() is about 4x faster on the copy itself. Behaviour is preserved. ifnotfound = list(NULL) keeps writing NULL for a key absent from the source (the loop did this via `[[`, which returns NULL rather than erroring), so the key is still created in the target. mget()'s inherits = FALSE default matches `[[` on an environment, so values in enclosing environments are still not picked up. list2env() returns the target environment visibly, so return invisible(NULL) to match what the for loop returned; no call site uses the value. Larger effect than the previous two commits: 9.7% off plot time, and every one of the 28 benchmarks improves. Cumulative for the branch, development HEAD was ~6% slower than v0.7.0 and is now ~19% faster (0.814x); against dev HEAD the branch is 0.765x. The 300-plot succession drops from 2130ms to 1605ms. Full suite passes, 729/729, with NOT_CRAN=true so snapshots ran. Claude-Session: https://claude.ai/code/session_01AjZRHA9ZvJkDPMFdDXUcUs
There was a problem hiding this comment.
🟢 Approval recommended
The optimizations preserve behavior; the remaining documentation cleanup is non-blocking.
Pull request overview
Optimizes recurring plot setup overhead while preserving existing behavior.
Changes:
- Batches environment copies with
mget()andlist2env(). - Caches valid base graphical parameter names.
- Defers assertion-message construction until failure.
File summaries
| File | Description |
|---|---|
R/utils.R |
Optimizes environment copying. |
R/tpar.R |
Avoids invalid par() lookups. |
R/environment.R |
Adds the parameter-name cache. |
R/assertions.R |
Lazily constructs error messages. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
par(no.readonly = FALSE) already returns the read-only parameters (cin, cra, csi, cxy, din, page); it is no.readonly = TRUE that excludes them. Appending them was a no-op that left six duplicates in the allowlist, and the comment asserted the opposite of the API's actual behaviour. The resulting set is unchanged (72 names, same unique membership), so behaviour is identical; this only removes the duplicates and corrects the comment. Spotted in review by Copilot on #723. 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.
Did some performance snooping with Claude's help. The headline result is a ~20% improvement in (per) plot time by removing fixed per-call overhead.
Main improvement is via
get_tpar(). Previously, it fell back tosuppressWarnings(par(o))for every name absent from.tpar. Since mosttparparams are tinyplot native (grid.bg,,x/yaxr, ...), this was creating a surprising amount of uncessary overhead. Gating thepar()call behind the set of names that base R actually knows removes it.Two smaller ones:
env2env()now copies viamget()+list2env()instead of oneassign()per key (~9%).assert_*()helpers build their error message inside the failure branch instead of before the check (~0.6%).