Inline All The Things #8946
Open
Inline All The Things #8946
+176
−151
Conversation
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
Changes it so that the renderer retains its own MemoryPool for PageList pages so that new pages rarely need to be allocated when cloning the screen. Also switches to using an arena allocator in `updateFrame` to avoid having to deinit the cloned screen since instead we can just throw out the memory.
Supported by benchmarks (vtebench on Apple M3 Max)
Supported by benchmarks (vtebench on Apple M3 Max)
A whole bunch of inline annotations, some of these were tracked down with Instruments.app, others are guesses / just seemed right because they were trivial wrapper functions. Regardless, these changes are ultimately supported by improved vtebench results on my machine (Apple M3 Max).
|
Oh, test failures. I totally neglected to test before PRing. I'll look in to that tomorrow. Edit: They're very confusing errors actually-- is it some sort of miscompile? It's seemingly caused by |
Very weird failures, not 100% sure of the cause; regardless, this fixes them.
qwerasd205
force-pushed
the
inline-all-the-things
branch
from
7f6737b to
a06d32b
Compare
|
I can run the vtebench marks at home when I have my x86 machine. |
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.
I used the new CPU counter mode in Instruments.app to track down functions that had instruction delivery bottlenecks (indicating i-cache misses) and picked a bunch of trivial functions to mark as inline (plus a couple that are only used once or twice and which benefit from inlining).
The size of
macos-arm64/libghostty-fat.abuilt withzig build -Doptimize=ReleaseFast -Dxcframework-target=nativegoes from145,538,856bytes onmainto145,595,952on this branch, a negligible increase.These changes resulted in some pretty sizable improvements in vtebench results on my machine (Apple M3 Max):
With this, the only vtebench test we're slower than Alacritty in (on my machine, at 130x51 window size) is
dense_cells(which, IMO, is so artificial that optimizing for it might actually negatively impact real world performance).I also did a pretty simple improvement to how we copy the screen in the renderer, gave it its own page pool for less memory churn. Further optimization in that area should be explored since in some scenarios it seems like as much as 35% of the time on the
io-readerthread is spent waiting for the lock.Note
Before this is merged, someone really ought to test this on an x86 processor to see how the performance compares there, since this is tuning for my processor specifically, and I know that M chips have pretty big i-cache compared to some x86 processors which could impact the performance characteristics of these changes.