Allow end users to observe JSX token's value #331
Conversation
adams85
force-pushed
the
fix/jsxtoken-value
branch
4 times, most recently
from
7bf06d3
to
f9aa1ce
Compare
src/Esprima/Token.cs
Outdated
| public object? Value | ||
| { | ||
| TokenType.Template => ((TemplateHolder) _value!).Cooked, | ||
| TokenType.RegularExpression => ((RegexHolder) _value!).Value, | ||
| _ => _value | ||
| }; | ||
| // When this accessor gets inlined, it has a significant negative impact on max. nesting depth (on .NET 6), | ||
| // which is kind of weird... | ||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| get => _value is ValueHolder holder ? holder.Value : _value; | ||
| } |
There was a problem hiding this comment.
@lahma Any idea on this?
I had to add MethodImplOptions.NoInlining to pass the new max. nesting depth test. I assume that the runtime started to inline the accessor automatically because it got shorter. I suspected that the local variable messed things up but changing it to something like _value is ValueHolder ? ((ValueHolder) _value).Value : _value; made no difference.
How come an inlined method uses more stack space than a non-inlined one?
There was a problem hiding this comment.
Well at least this is now more complex for the runtime. Earlier was quick jump for regular case (no value holder) without type checks. Now this always incurs more expensive type check.
There was a problem hiding this comment.
Unfortunately, this doesn't answer the question: I tested the earlier version with forced inlining (AggressiveInlining applied to the getter of Value). The result is the same: inlining Value reduces max. nesting depth to around 430. It just hasn't turned out before because it seems that the runtime doesn't tend to inline the earlier version without explicitly telling it to do so, as opposed to the new one.
Well at least this is now more complex for the runtime. Earlier was quick jump for regular case (no value holder) without type checks. Now this always incurs more expensive type check.
This, of course, is true but according to the above it may only affect execution time. I'll do some benchmarks later to ensure that we don't have a regression concerning that. We can revert to switch if that proves to be faster.
There was a problem hiding this comment.
While reading Pro .NET Memory Management yesterday, I noticed a small mention at some point suggesting that AggressiveInlining may actually disable a bunch of optimizations. It made me think of this. Any chance that forcing inlining disables layout optimizations of the struct, or similar?
There was a problem hiding this comment.
I noticed a small mention at some point suggesting that AggressiveInlining may actually disable a bunch of optimizations
Basically, inlining should enable a lot of optimizations which aren't possible otherwise, not prevent them. Especially, on .NET 6: see this excellent article by Stephen Toub on the topic.
I suppose the author of Pro .NET Memory Management is talking about this:
There’s also the impact on the JIT compiler itself, as the JIT has limits on things like the size of a method before it’ll give up on optimizing further; inline too much code, and you can exceed said limits.
But this goes for the method into which we inline other methods, not for the methods we inline.
Any chance that forcing inlining disables layout optimizations of the struct, or similar?
I doubt that inlining would affect the memory layout of structs. After some experimenting, I came to the conclusion that the problem lies around downcasting and accessing the Value property of the downcast holder object. After moving that part into a separate, non-inlined method, inlining started to work without extra stack consumption (see my later comment). I haven't had the patience to unearth the exact reasons but that's for sure we're pushing the limits of the runtime's capabilities here.
adams85
force-pushed
the
fix/jsxtoken-value
branch
from
f9aa1ce
to
d82442b
Compare
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| get | ||
| { | ||
| // NOTE: This condition must not be inverted, otherwise the runtime (.NET 6) fail to inline the accessor correctly. | ||
| return Type is not (TokenType.RegularExpression or TokenType.Template or TokenType.Extension) | ||
| ? _value | ||
| : GetValueFromHolder(in this); | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| static object? GetValueFromHolder(in Token token) => ((ValueHolder) token._value!).Value; | ||
| } |
There was a problem hiding this comment.
Finally I've managed to find a combination which the runtime can inline without penalty.
If you change anything in this code (even if just invert the condition), inlining goes south and the acceptance test starts to fail. Mind-boggling...
However, this looks to work reliably and according to the benchmarks, we even have a little bit of improvement in execution time:
Esprima.Benchmark.FileParsingBenchmark
| Diff | Method | FileName | Mean | Error | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|
| Old | ParseProgram | angular-1.2.5 | 17.628 ms | 2.5634 ms | 718.7500 | 343.7500 | - | 4,315 KB |
| New | 17.475 ms (-1%) | 1.1170 ms | 718.7500 (0%) | 312.5000 (-9%) | - | 4,315 KB (0%) | ||
| Old | ParseProgram | backbone-1.1.0 | 2.408 ms | 0.2234 ms | 136.7188 | 39.0625 | - | 676 KB |
| New | 2.338 ms (-3%) | 0.2539 ms | 140.6250 (+3%) | 35.1563 (-10%) | - | 676 KB (0%) | ||
| Old | ParseProgram | jquery-1.9.1 | 14.161 ms | 0.6347 ms | 625.0000 | 296.8750 | - | 3,764 KB |
| New | 13.805 ms (-3%) | 1.0131 ms | 609.3750 (-3%) | 296.8750 (0%) | - | 3,764 KB (0%) | ||
| Old | ParseProgram | jquery.mobile-1.4.2 | 22.922 ms | 0.3971 ms | 1031.2500 | 500.0000 | 31.2500 | 5,798 KB |
| New | 22.039 ms (-4%) | 2.4387 ms | 1031.2500 (0%) | 500.0000 (0%) | 31.2500 (0%) | 5,798 KB (0%) | ||
| Old | ParseProgram | mootools-1.4.5 | 11.000 ms | 0.1603 ms | 500.0000 | 250.0000 | - | 3,091 KB |
| New | 10.776 ms (-2%) | 0.4187 ms | 500.0000 (0%) | 250.0000 (0%) | - | 3,091 KB (0%) | ||
| Old | ParseProgram | underscore-1.5.2 | 2.022 ms | 0.0332 ms | 117.1875 | 42.9688 | - | 571 KB |
| New | 1.930 ms (-5%) | 0.0355 ms | 113.2813 (-3%) | 42.9688 (0%) | - | 571 KB (0%) | ||
| Old | ParseProgram | yui-3.12.0 | 10.352 ms | 1.6380 ms | 468.7500 | 234.3750 | - | 2,856 KB |
| New | 9.828 ms (-5%) | 1.0396 ms | 468.7500 (0%) | 234.3750 (0%) | - | 2,856 KB (0%) |
A minor correction to #328