perf: skip the margin width scan when there are no vertical margins - #722
Open
ZviBaratz wants to merge 1 commit into
Open
perf: skip the margin width scan when there are no vertical margins#722ZviBaratz wants to merge 1 commit into
ZviBaratz wants to merge 1 commit into
Conversation
applyMargins called getLines unconditionally, so every non-inline Render paid a
full grapheme-cluster width pass over the whole block to compute a `spaces`
string that is only used when a top or bottom margin is set. Most styles set
neither, so most Render calls did the walk and threw the result away.
getLines is guarded on the margins that consume it now. Behaviour is unchanged:
`width` has no other reader in the function, and getLines is pure.
Measured with the repo's own BenchmarkStyleRender, unmodified (go1.26,
linux/amd64, -benchtime 2s):
case before after
simple-1-line 1683 ns/op 1469 ns/op
simple-5-lines 6814 ns/op 5360 ns/op
simple-5-lines-inline 1334 ns/op 1321 ns/op
simple-10-lines-5-height-40-width 19989 ns/op 13461 ns/op
simple-10-lines-width-maxwidth 27164 ns/op 21544 ns/op
simple-10-lines-width-maxwidth-borders 47630 ns/op 39199 ns/op
...-borders-padding-margins 76691 ns/op 75509 ns/op
Each case that sets no vertical margin loses exactly one allocation and 21-33%
of its time. The last row is the control: it does set margins, and neither its
time nor its allocation count (411 both sides) moves. The inline row is
untouched, as it was already excluded.
TestVerticalMargins covers both sides of the new guard. Top/bottom margin
rendering had no direct test before this — the existing margin tests are
getters and setters — so the guard would otherwise have been unpinned.
Fixes charmbracelet#721.
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.
Fixes #721.
applyMarginscallsgetLinesunconditionally, so every non-inlineRenderpays a full grapheme-cluster width pass over the whole block to build aspacesstring that is only read when a top or bottom margin is set. Most styles set neither.widthhas no other reader in the function andgetLinesis pure, so guarding it on the margins that consume it is behaviour-preserving.Measured with this repo's own
BenchmarkStyleRender, unmodifiedgo1.26, linux/amd64, Intel Core Ultra 7 258V,
-benchtime 2s:simple-1-linesimple-5-linessimple-5-lines-inlinesimple-10-lines-5-height-40-widthsimple-10-lines-width-maxwidthsimple-10-lines-width-maxwidth-borders…-borders-padding-marginsEvery case that sets no vertical margin loses exactly one allocation and 21–33% of its time.
Two of those rows are controls rather than results:
…-borders-padding-marginsdoes set margins. Its time is unchanged within run-to-run noise and its allocation count is identical on both sides (411), which is what says the guard did not skip work that was needed. Confirmed across three runs.simple-5-lines-inlineis unchanged, as expected —inlinealready excluded this block.Why there is a test
Top/bottom margin rendering had no direct coverage — the existing margin tests are getters and setters (
GetMarginTop,UnsetMarginBottom, …), and the only render-level margin test is intable/. So the new guard would have shipped unpinned.TestVerticalMarginscovers both sides of it. I mutation-tested it: dropping either half of the condition, or making it never fire, all fail it.It also records one thing that surprised me, in case it saves someone a wrong "optimization" later: by the time
applyMarginsruns, horizontal alignment has already padded every line out to the widest, so reading the width off the first line passes the test too. That equality is incidental to the ordering, not a property of the block, so the test says so rather than implying it pins the walk.One note on the fixture: the test uses
NewStyle().Align(Left)rather than a bareNewStyle(), because with no properties at allRenderreturns at thes.props == 0early exit and never reaches alignment or margins — a bare style would have made the no-margin case assert nothing about this path.Context
Found while profiling a Bubble Tea app whose frame build turned out to be ~70%
ansi.StringWidth. In a CPU profile of one full frame,applyMarginswas 11.2% of samples — 13.5% of the frame build — and 94% of that was this one call. That app sets exactly one margin in its entire tree.go test ./...passes (lipgloss,list,table,tree). Thegofmt/go vetfindings intable/table_test.goare pre-existing onmasterand untouched here.