Optimize and simplify delegate layout#99200
Conversation
ghost
added
the
|
I'm not sure what's up with the failures here, tests that are failing on the CI seem to pass on my machine. |
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
|
/azp run runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Could you please collect some perf numbers to give us an idea about the improvements and regressions in the affected areas? We may want to do some optimizations to mitigate the regressions. |
The existing code tries to compare MethodInfos as a cheap fast path. Most delegates do not have cached MethodInfo, so this fast path is hit rarely - but it is very cheap, so it is still worth it. This cheap fast path is not cheap anymore with this change. It may be best to delete the fast path that is trying to compare the MethodInfos and potentially optimize |
I think the thing that'd need benchmarking here are equality checks and maybe the impact of collectible delegates being stored in the CWT on the GC, the rest of things shouldn't be performance sensitive enough to matter I think? I'm not fully sure what'd be the proper way for benchmarking the latter.
I am going to benchmark the impact of the equality change tomorrow, I feel like if the impact won't be big, potential optimizations to it can be done later. |
Write a small program that loads an assembly as collectible and calls a method in it. The method in collectible assembly can create delegates in a loop. (If you would like to do it under benchmarknet, it works too - but it is probably more work.) |
| Debug.Assert(invocationList[0] is MulticastDelegate); | ||
| Debug.Assert((uint)invocationList.Length >= (nuint)_extraData); | ||
|
|
||
| ref MulticastDelegate first = ref Unsafe.As<object, MulticastDelegate>(ref MemoryMarshal.GetArrayDataReference(invocationList)); |
There was a problem hiding this comment.
I would use Delegate instead of MulticastDelegate where possible. There is nothing interesting on MulticastDelegate now. I expect that we will delete MulticastDelegate.CoreCLR.cs in a follow up and move whatever is left to Delegate.cs.
There was a problem hiding this comment.
Only issue with that is that it'll make calls to Equals and such virtual. As such, I think waiting for the wrapper struct is better here.
| Debug.Assert(invocationList[0] is MulticastDelegate); | ||
| Debug.Assert((uint)invocationList.Length >= (nuint)_extraData); | ||
|
|
||
| ref MulticastDelegate first = ref Unsafe.As<object, MulticastDelegate>(ref MemoryMarshal.GetArrayDataReference(invocationList)); |
There was a problem hiding this comment.
Can we make this more type safe by construction by using Wrapper[] or Delegate[] for the invocation array and avoid these unsafe conversions? Wrapper[] is what's used by NativeAOT. Delegate[] would works too, but it has some extra overhead during initialization due to array co-variance.
There was a problem hiding this comment.
I wanted to keep that for a followup that moves everything to Delegate.cs
Would the struct variant be preferred then or Delegate?
There was a problem hiding this comment.
I think the struct variant is better: It is aligned with NAOT and it avoids the array co-variance overhead (without resorting to unsafe code).
|
I have no idea why the cDAC tests are still failing here. |
|
/azp run runtime-coreclr outerloop, runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@jkotas Can we keep the rest of refactors for followups here? |
There was a problem hiding this comment.
@jkotas Can we keep the rest of refactors for followups here?
I am not happy about refactors in this PR that are introducing unsafe code that can be done with safe code instead. I would be fine with no refactors; or refactors using safe code.
| } | ||
|
|
||
| DelegateType delegateType = DelegateType.Unknown; | ||
| if (!isMulticast && target.ReadNInt(address + /* Delegate::ExtraData offset */) != -1) |
There was a problem hiding this comment.
We should document the -1 as a contract constant at the top of the file.
See Contract Constants section in other datacontract doc files like Loader.md for reference:
### Contract Constants:
| Name | Type | Purpose | Value |
| --- | --- | --- | --- |
| `ASSEMBLY_NOTIFYFLAGS_PROFILER_NOTIFIED` | uint | Flag in Assembly NotifyFlags indicating the Assembly will notify profilers. | `0x1` |
There was a problem hiding this comment.
Does it look good now?
Do you mean the unsafe code in If you prefer though, I can remove EDIT: |
|
The code in main does not have the unsafe array casts or bounds check removals via unsafe code. The refactoring that introduces the extra unsafe code is a performance optimization that is not strictly related to the layout changes. The unsafe array casts can be eliminated by using the struct wrapper like in NAOT, and I do not mind an extra bound check for Equals - I expect that it will barely register in Equals performance. |
|
@jkotas Is the last commit what you intended? |
First attempt at making delegate GC fields immutable in CoreCLR so that they can be allocated on the NonGC heap.
I've checked it with a simple app and corerun locally with a delegate from an unloadable ALC and it seemed to not crash, assert nor unload the ALC from under the delegate, however I couldn't actually find any runtime tests that would verify delegates from unloadable ALCs work so the CI coverage might be missing.
One small point of concern is that this might make delegate equality checks slower since they rely on checking the methods in the last "slow path" check, which is however always hit for different delegates AFAIR.
Contributes to #85014.
cc @jkotas