Repository navigation
Optimize Span operations, especially the hot Span::to - #163971
joshtriplett wants to merge 8 commits into
Conversation
Currently there are span representations if you have a context and no
parent, or a parent and no context. But if you have both a context *and*
a parent, the span is always interned. In incremental builds this is
something like 20-40% of all spans.
This commit uses a new compression scheme that gives small perf wins for
non-incremental and bigger perf wins for incremental. The following table
briefly summarizes the formats.
```
-----------------------------------------------------------------
old new
-----------------------------------------------------------------
InlineCtxt lo32,len15,ctxt16,parent0 lo32,len15,ctxt16,parent0
InlineParent lo32,len15,ctxt0,parent16 lo32,len14,ctxt0,parent15
InlinePair n/a lo24,len7,ctxt15,parent16
PartialInt. ctxt16 everything else
FullInt. everything else n/a
```
Things to note.
- The old scheme had a 32-bit field and two 16-bit fields. The new scheme has
one 64-bit field and uses `repr(packed(4))`. (Keeping the alignment at
4 is important to keep AST node sizes low.)
- The new scheme distinguishes the four formats with a single variable-length
prefix in the high bits. This is simpler than the old combination of tag bits
and special values.
- The new `SpanField` type is used to describe the various field widths
for each format, and provide operations on them.
- The new `InlineParent` is very slightly worse than the old one, with one less
bit for each of len and parent. This doesn't affect many spans.
- The new `InlinePair` format handles most cases where both a context and a
parent are present. It has to compromise some on `lo` and `len`, but still
gets a lot of them. This is the big performance win.
- The partially/fully interned cases are merged, because the Interned format
can now always hold a full context inline, and the new `Interned` is
equivalent to the old PartiallyInterned format. (This requires
limiting ctxts to 29 bits, but we'll hit other problems, such as OOM,
long before that limit is reached.) This also means we don't need to
store the ctxt field in the interner, and we use the new
`SpanDataNoCtxt` type for that.
…t help `prepare_to_combine`, as its last fallback, checked `a.ctxt == b.span().source_callsite().ctxt()`. Immediately prior to that, it has a fast-path of checking `a.ctxt.is_root()`. `source_callsite()` recurses until it finds a root context. So, if it returns at all (rather than panicking), it'll return a root context. This means `a.ctxt == b.span().source_callsite().ctxt()` cannot be true unless `a.ctxt.is_root()` was already true. So, the call to source_callsite will do a pile of work but not change the outcome. tt-muncher hits this case, and recurses deeply into it. Remove this case, and let `a.ctxt.is_root()` be the only check here. This saves more than 2% instructions on tt-muncher incr-full and more on incr-unchanged.
Valid spans have `lo <= hi`, so if we know we got the values from an already valid span, we don't need to recheck them. This eliminates a check and conditional move in `to`, which is a hot path.
Rather than passing in full decoded SpanData to the `try_metavars` helper, just pass in contexts, and get back `Option`s for optionally replacing the corresponding span. This allows the caller to avoid materializing the decoded SpanData on the stack.
If `try_metavars` returns `(None, None)`, we definitely won't have matching contexts and we need the error case. Otherwise, we definitely have matching contexts (since that led `try_metavars` to return one or both `Some` values), so we don't need to recheck that.
`Span::to` is a hot path. `InlineCtxt` is by far the most common case. Checking for that case is fast (a prefix check on a span). And in that case, we know we don't have parents, we can do a quick comparison of the contexts, and we know the context will fit back in an inline span. As a result, this fast path manages to entirely skip setting up a stack frame or saving registers.
|
Benchmarking this with @nnethercote's PR as the base: @bors try parent=d2adbd4acd6d3b565c7a003a9fc9439c67d42cf6 @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Correct hash this time: @bors try parent=03aba7c9e40d02c0476b996cdc81cbe1bd24e247 @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Optimize Span operations, especially the hot `Span::to`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (045d996): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.4%, secondary -0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.2%, secondary -0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 492.261s -> 488.235s (-0.82%) |
|
r? nnethercote |
|
@rustbot ready |
This stack of changes incrementally optimizes
Spanoperations, and in particularSpan::to, until at the endSpan::tohas a hot path that runs entirely via registers with no stack frame.I recommend reviewing commit by commit, ignoring the first commit.
This is based on @nnethercote's PR overhauling span compression, because some of it depends on that, either directly or just because all of my benchmarking happened atop that.
In
Span::prepare_to_combine, skip callingsource_callsite()as it can't help.prepare_to_combine, as its last fallback, checkeda.ctxt == b.span().source_callsite().ctxt(). Immediately prior to that, it has a fast-path of checkinga.ctxt.is_root().source_callsite()recurses until it finds a root context. So, if it returns at all (rather than panicking), it'll return a root context. This meansa.ctxt == b.span().source_callsite().ctxt()cannot be true unlessa.ctxt.is_root()was already true. So, the call to source_callsite will do a pile of work but not change the outcome.tt-muncher hits this case, and recurses deeply into it.
Remove this case, and let
a.ctxt.is_root()be the only check here.Add a
Span::new_orderedconstructor that skips thelo > hicheck.Valid spans have
lo <= hi, so if we know we got the values from an already valid span, we don't need to recheck them. This eliminates a check and conditional move into, which is a hot path.Rework
Span::try_metavarsandSpan::prepare_to_combineto reduce spilling.Rather than passing in full decoded SpanData to the
try_metavarshelper, just pass in contexts, and get backOptions for optionally replacing the corresponding span. This allows the caller to avoid materializing the decoded SpanData on the stack; the spill showed up in profiles.As a minor rework, reorder
Span::prepare_to_combineto avoid redundant conditions.After the previous change, if
try_metavarsreturns(None, None), we definitely won't have matching contexts and we need the error case. Otherwise, we definitely have matching contexts (since that ledtry_metavarsto return one or bothSomevalues), so we don't need to recheck that.Span::to: Add a fast path where both spans are inlineInlineCtxtis by far the most common case. Checking for that case is fast (a prefix check on a span). And in that case, we know we don't have parents, we can do a quick comparison of the contexts, and we know the context will fit back in an inline span.As a result, this fast path manages to entirely skip setting up a stack frame or saving registers.