Avoid caching indentation data that changes over time#61304
Avoid caching indentation data that changes over time#61304sharwell merged 6 commits intodotnet:mainfrom
Conversation
| (baseIndentationData, totalAdjustment) = (indentationData, 0); | ||
| } | ||
|
|
||
| return indentationData.Indentation + indentationDelta; |
There was a problem hiding this comment.
📝 indentationData.Indentation could be a computed property against values that change during formatting. The result of this computation is no longer directly cached.
| var baseIndentation = _tokenStream.GetCurrentColumn(operation.Option.IsOn(IndentBlockOption.RelativeToFirstTokenOnBaseTokenLine) ? | ||
| _tokenStream.FirstTokenOfBaseTokenLine(operation.BaseToken) : |
There was a problem hiding this comment.
📝 baseIndentation is computed from properties that can change during formatting. This value is no longer cached.
| } | ||
|
|
||
| public IndentationData BaseIndentationData { get; } | ||
| public int Adjustment { get; } |
There was a problem hiding this comment.
i wouldn't mind some docs on this. the relation of base-indentation, adjustment, and text span aren't super clear.
There was a problem hiding this comment.
I'm not sure what the text span is for, but I can add docs to explain the other two. 😄
| { | ||
| private readonly Lazy<int> _indentationGetter; | ||
| public LazyIndentationData(TextSpan textSpan, Lazy<int> indentationGetter) | ||
| private readonly Lazy<int> _lazyIndentationDelta; |
There was a problem hiding this comment.
so the idea is htat we wait until we actually need this, then comput it, then cache it at that point?
There was a problem hiding this comment.
That was the idea even before this PR. The problem stemmed from the fact that too much of the computed result was captured in that lazily-computed value. The part that changes is now a separate lambda which is called every time instead of cached.
| protected abstract IndentationData WithTextSpanCore(TextSpan span); | ||
| } | ||
|
|
||
| private class RootIndentationData : SimpleIndentationData |
| if (existingIndentationDelta != UninitializedIndentationDelta) | ||
| return existingIndentationDelta; | ||
|
|
||
| return indentationDelta; |
There was a problem hiding this comment.
confused by this code. very very confused by it. needs comments majorly. if this is just an int, it's unclear to me why we need volatile at all.
if this is to somehow allow multiple callers to initialize, but let first win, why not let any win?
if this is to allow multiple callers to initialize, and they'll get different results... then 😨
There was a problem hiding this comment.
This is the standard optimistic lazy initialization pattern, but I've extracted a helper method so it's reusable.
There was a problem hiding this comment.
i guess my point is: for integer values, where all callers will produce the same value, it seems liek with normal .net memory model this can be trivial. just:
There was a problem hiding this comment.
var result = _lazyIndentationDelta;
if (result == UninitializedIndentationDelta
result = _lazyIndentationDelta = _indentationDeltaGetter(_formattingContext, Operation);
return result;There was a problem hiding this comment.
... where all callers will produce the same value ...
The code does not assume or require this. It's not desirable for this to be violated, but if it is we have the most reasonable available outcome.
|
Overall, def needs more doc-comments and explanation/example comments in code. And there is one piece of code that i really do not like in the current state :) |
| return existingValue; | ||
|
|
||
| return InterlockedStore(ref target, uninitializedValue, valueFactory(state)); | ||
| } |
Suggested review strategy: commit-by-commit
Fixes #60437