Take back the width optimisation, which does nothing - #202
Merged
Conversation
#200 made the identity line's `width` conditional, on the reasoning that a definite width is what the layout stage pays for and most display names cannot overflow anyway. It said plainly that the saving was argued and not measured, because the harness of the time varied 2.4x run to run and could not resolve it. #201 fixed the harness. The answer is that there is no saving. Measured against the fixed corpus, best-of-three rounds, same power state: with the conditional (as shipped) 1388 1495 1526 1594 every leaf bounded (as before) 1426 with this revert 1370 1374 The pre-#200 behaviour sits inside the range of the version that was meant to beat it. Whatever #138 costs, it is not the width on these two leaves. So it goes. It was a branch, a constant, two counters and four tests whose only justification was a number that turns out to be zero, and code kept for a reason that has been disproven is worse than code that was never written: the next person reads the comment and believes it. What the tests were protecting is intact and always was. The width is unconditional again, which is exactly what #138 established was needed for a stranger's display name to ellipsize rather than run through the side of the window, and the sweep that renders every screen at the window floor with each field filled to its buffer still covers it. Two things worth keeping from the exercise, and they are in #201 and here rather than in code: #138 IS the step, now that it can be measured properly. On the fixed corpus at matched node counts: #137 908us, #138 1433us and 1324us, today about 1450us. I had retracted that attribution in #200 for being based on a single reading, which was the right call on the evidence then; the evidence exists now and it says the original claim was right. And roughly 500us of layout arrived in #138 whose cause is still unknown. It is not the identity widths, and probes of the two column widths showed nothing either, though those were taken on the old harness and are worth redoing. That is now a findable question rather than an unanswerable one.
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
1 participant
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.
#200 made the identity line's
widthconditional, reasoning that a definite width is what the layout stage pays for and that most display names cannot overflow anyway. It said plainly that the saving was argued rather than measured, because the harness of the time varied 2.4x run to run and could not resolve it.#201 fixed the harness. The answer is that there is no saving.
Measured
Fixed corpus, best-of-three rounds, same power state, matched node counts:
The pre-#200 behaviour sits inside the range of the version meant to beat it. Whatever #138 costs, it is not the width on these two leaves.
So it goes
A branch, a constant, two counters and four tests, whose only justification was a number that turns out to be zero. Code kept for a reason that has been disproven is worse than code never written, because the next person reads the comment and believes it.
What those tests protected is intact and always was: the width is unconditional again, which is what #138 established is needed for a stranger's display name to ellipsize instead of running through the side of the window. The sweep that renders every screen at the window floor with each field filled to its buffer capacity still covers that.
Two things worth keeping
#138 is the step, confirmed. On the fixed corpus at matched node counts: #137 908 us, #138 1433 us and 1324 us, today about 1450 us. I retracted that attribution in #200 for resting on a single reading, which was right on the evidence then. The evidence exists now and the original claim holds.
About 500 us of layout arrived in #138 and its cause is still unknown. Not the identity widths. Probes of the two column widths showed nothing either, but those were taken on the old harness and deserve redoing. That is now a findable question rather than an unanswerable one, which is the whole return on #201.