Added reflection emit member accessor#38233
Added reflection emit member accessor#38233steveharter merged 2 commits intodotnet:masterfrom YohDeadfall:json-member-access
Conversation
| namespace System.Text.Json.Serialization | ||
| { | ||
| internal static class MemberAccessor | ||
| internal abstract class MemberAccessor |
There was a problem hiding this comment.
Wouldn't this indirection via an abstract class cause some perf regression?
There was a problem hiding this comment.
No, it affects class info construction only. A constructed class info is cached in the associated JsonSerializerOptions and reused later. The class materializer is implemented the same way, so I'm going to merge them.
Speaking about performance, there should be an improvement because the inbox build uses reflection emit to generate stubs to unbox an object instance and to pass it to the actual getter or setter. This eliminates one delegate call per action.
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
| } | ||
| else | ||
| { | ||
| generator.Emit(OpCodes.Castclass, classType); |
There was a problem hiding this comment.
Originally was using emit for getters\setters, then switched to direct delegates (just as fast).
That emit approach was different - I passed in typeof(object) to the DynamicMethod ctor instead of objectType and then OpCodes.Ldarg_0 (object) + OpCodes.Ldarg_1 (property value) + Callvirt + Ret.
I wonder if that was faster than using Castclass?
There was a problem hiding this comment.
You know the answer, right? (: Castclass is just a safety check which I added because of discussion in #36506. Remove that operation?
There was a problem hiding this comment.
I think it's good although a micro benchmark would be useful.
There was a problem hiding this comment.
If safety isn't so important Castclass should be removed.
BenchmarkDotNet=v0.11.5, OS=Windows 10.0.18895
Intel Xeon CPU E5-2687W 0 3.10GHz, 2 CPU, 8 logical and 8 physical cores
.NET Core SDK=3.0.100-preview5-011568
[Host] : .NET Core 3.0.0-preview5-27626-15 (CoreCLR 4.6.27622.75, CoreFX 4.700.19.22408), 64bit RyuJIT
DefaultJob : .NET Core 3.0.0-preview5-27626-15 (CoreCLR 4.6.27622.75, CoreFX 4.700.19.22408), 64bit RyuJIT| Method | Mean | Error | StdDev | Op/s |
|---|---|---|---|---|
| WithCastCall | 9.751 ns | 0.3378 ns | 0.9854 ns | 102,552,670.5 |
| WithoutCastCall | 8.346 ns | 0.3414 ns | 1.0068 ns | 119,813,101.1 |
public class ValueHolder
{
public int Value { get; set; }
}
private static readonly Func<object, int> s_WithCastCall = CreatePropertyGetter<ValueHolder, int>(typeof(ValueHolder).GetProperty("Value"), true);
private static readonly Func<object, int> s_WithoutCastCall = CreatePropertyGetter<ValueHolder, int>(typeof(ValueHolder).GetProperty("Value"), false);
private object _holder = new ValueHolder { Value = 42 };
[Benchmark]
public int WithCastCall()
{
return s_WithCastCall(_holder);
}
[Benchmark]
public int WithoutCastCall()
{
return s_WithoutCastCall(_holder);
}
|
|
||
| generator.Emit(OpCodes.Ret); | ||
|
|
||
| return dynamicMethod.CreateDelegate(typeof(Action<,>).MakeGenericType(objectType, propertyInfo.PropertyType)); |
There was a problem hiding this comment.
For performance, it is possible to remove MakeGenericType if this method takes <TProperty> generic and binds to a delegate on JsonPropertyInfo<TProperty> (or elsewhere).
There was a problem hiding this comment.
It's an initialization only method, so the performance isn't so critical here. At the same time if TClass or TProperty is a value type, the JIT will produce a separate codegens of it which will be used only once and will occupy memory.
With the current code the generic stub method CreatePropertySetter<TClass, TProperty> is 16 bytes long and (if my memory serves me right, @AndyAyersMS knows it) therefore it will be inlined. The jitted code of non-generic CreatePropertySetter is shared in that case.
Correct me if I'm wrong.
There was a problem hiding this comment.
While I was writing about optimizations I completely forgot that the method is virtual and cannot be inlined. I think with static interface members (dotnet/csharplang#1711) it would be possible to inline generics.
There was a problem hiding this comment.
Virtual methods can be inlined, if the jit can determine that they're called.
New generic instantiations always take up some space, whether or not their methods ever get called or inlined; the runtime has to keep track of them somehow. Action is relatively pay-for-play though as instantiating it does not trigger instantiation of other types.
| } | ||
| else | ||
| { | ||
| generator.Emit(OpCodes.Castclass, classType); |
There was a problem hiding this comment.
Is Castclass better\faster than Ldarg_0?. Also see similar comment below for setter.
There was a problem hiding this comment.
Moreover the perf could be improved by removing castclass but at the cost of type safety. I've posted the requested benchmark results above.
|
I have a question about an empty line at the end of file. What style is preferred? In some files I see no empty lines, but in other they are. |
layomia
left a comment
layomia
left a comment
There was a problem hiding this comment.
@steveharter the immutable CreateRange delegate changes look okay to me.
Just make sure there's a linefeed\CR on the last line - no extra empty line is desired AFAIK. |
steveharter
left a comment
steveharter
left a comment
There was a problem hiding this comment.
Overall this looks really good and sets us up for fields. Thanks!
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
@steveharter Could you take the final review? |
|
The tests are failing -- IL causing InvalidProgramException? |
YohDeadfall
left a comment
YohDeadfall
left a comment
There was a problem hiding this comment.
I haven't changed materialization in this pull request. Just added reflection emit for property accessors.
|
|
||
| if (classType.IsValueType) | ||
| { | ||
| generator.Emit(OpCodes.Unbox_Any, classType); |
There was a problem hiding this comment.
Avoid boxing for perf?
There was a problem hiding this comment.
Note the previous code (before changed due to supporting struct) used a direct delegate which did not box on get\set.
There was a problem hiding this comment.
Yep, but to do that I need to multiply existing property info classes by two to support reference and value types. This will lead to unmaintable code which I'm trying to avoid. By the way ReadStackFrame stores a property value on the heap always, and only a json property info uses member accessors to write or to read some value. The property info is type safe. So I think that there is nothing to worry. The thing you are requesting can be addressed in the future, and at the same time we probably could optimize the stack frame type to store a value directly in it.
There was a problem hiding this comment.
ReadStackFrame only stores the "root" value that is going to be returned (JsonSerialier.Parse<int>("1")), not all property values on a POCO object for example.
I'm concerned that boxing on every stuct-based POCO property (int, DateTime, etc), based on previous benchmarks from initial prototyping.
|
|
||
| if (classType.IsValueType) | ||
| { | ||
| generator.Emit(OpCodes.Unbox_Any, classType); |
There was a problem hiding this comment.
Avoid boxing for perf?
There was a problem hiding this comment.
It doesn't hurt performance because before my changes there were casts in the C# code and all values stored as objects in a read stack.
There was a problem hiding this comment.
That is not true. The only object stored on a read stack is a root object being returned, not POCO properties.
How can I view the previous pipeline run results? It seems that it was more successful, but may be wrong. |
Usually they are available in the commit history, but I am not sure how long they remain there. |
|
The previous pipeline wasn't fully ran, so there was more green than red. I fixed the bug (used |
|
Now there is only one failed test and it's unrelated to this PR: |
|
@YohDeadfall I think we need to pause for a couple days until we can review this and ensure boxing is not affecting perf. Also, with the support for custom converters it is possible to write a converter for a struct, including a standard struct scenario where there is a constructor that takes the arguments and does not have setters for any properties. What are the viable scenarios where a large struct with several properties is desired over a class (and no converter is desired)? |
|
@steveharter My pull request introduces no boxing. It just adds code generation which does the same thing as two hops with delegates. The previous one didn't that too. The only thing that I changed is moved casting of an Improving performance via removing boxing is possible and I see to ways to do that:
internal sealed class ByReference<T>
{
public T Value;
}
Recommended this way as a temporary solution to the ASP.NET Core team, but as I know they chose another simpler way. Probably you should ask @rynowak or someone else working in that area. But as a customer I wouldn't like to write a custom converter too.
Not large, but small. For example, typified identifiers, coordinates and similar cases. No one would like to store them on the heap. |
|
@YohDeadfall changes here need to be vetted through benchmarks to ensure no regressions. This is a sensitive area where a lot of effort that went into it to make it performant. If you can provide before\after benchmark results on a POCO object with a few primitives (int, DateTime, etc) then we can compare and go from there. Thanks The benchmarks are at https://github.com/dotnet/performance/tree/master/src/benchmarks/micro/Serializers. I can run them for you. |
I will be able to run benchmarks on Friday, so it would be nice to see results earlier. |
|
@YohDeadfall, @steveharter - any updates on this PR? Looks like this change is waiting on perf numbers (particularly around boxing/allocations) |
|
I'm working on it, but getting errors mentioned in dotnet/performance#560 (comment). |
What version of the sdk are you using (can you share Can you try running the benchmarks following these instructions (pass in path to corerun): |
|
This is works for me:
|
.NET Core 3 Preview 6
Haven't tried yet. Found the issue in the last moment,
Thank you! Will take a try. |
BenchmarkDotNet=v0.11.3.1003-nightly, OS=Windows 10.0.18895
Intel Xeon CPU E5-2687W 0 3.10GHz, 2 CPU, 8 logical and 8 physical cores
.NET Core SDK=3.0.100-preview7-012598
[Host] : .NET Core 3.0.0-preview7-27824-03 (CoreCLR 4.700.19.32302, CoreFX 4.700.19.32001), 64bit RyuJIT
Job-DAJFDT : .NET Core ? (CoreCLR 4.700.19.32003, CoreFX 4.700.19.32201), 64bit RyuJIT
Runtime=Core Toolchain=CoreRun IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
No Faster results for the provided threshold = 5% and noise filter = 0.3ns. ReadJsonMyEventsListerViewModelBefore
After
LoginViewModelBefore
After
LocationBefore
After
IndexViewModelBefore
After
BinaryDataBefore
After
WriteJsonMyEventsListerViewModelBefore
After
LoginViewModelBefore
After
LocationBefore
After
IndexViewModelBefore
After
BinaryDataBefore
After
|
|
An anomaly in Location and IndexViewModel - one considerable faster before and one considerably slower before. Otherwise the performance is within the margin of error. |
|
Test failure in Linux x64_Debug is unrelated (System.Net.HttpListener.Tests). |
Messed up results. Fixed now. |
|
One thing to note here, the existing benchmarks don't have any user-defined structs. So, any allocation increase due to boxing won't show up in the benchmarks. We may want to consider adding a struct based test in the benchmarks and verify the allocations aren't increasing before/after. |
| return factory(propertyGetter); | ||
| } | ||
| else | ||
| if (createRangeMethod == null) |
There was a problem hiding this comment.
@YohDeadfall - we are annotating System.Text.Json with nullability and it looks like this branch is not covered by any tests . When can createRangeMethod be null? And if this is a valid branch, we should add a test. Using Debug.Fail doesn't work in release.
There was a problem hiding this comment.
It's not mine code, but @layomia's (: The comment says that this shouldn't happen. Probably it would be better to throw exception directly from FindImmutableCreateRangeMethod.
There was a problem hiding this comment.
It's not mine code
Yes, just realized that when looking at the commit/history more closely (this PR moved the code). Sorry about the ping :)
This is the PR I am interested in: #37710
There was a problem hiding this comment.
No worries, feel free to ping me for any question (:
By the way, is it the right time to move the field support to the new repo? Have you finished optimizations?
Commit migrated from dotnet/corefx@25374d3
6 participants
This is prerequisite for #36505 and opened as a separate PR because it can be viewed as an optimization of existing code. The feature will come after merging this one.
/cc @ahsonkhan @steveharter