Update the conventions for discovering and invoking metadata update handlers #17460
Conversation
…andlers * Introduce ClearCache and UpdateApplication action names * Invoke actions in topologically sorted order.
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
| // This would ensure that there is a well-defined order and that caches and updates more lower in the application stack | ||
| // are up to date before ones higher in the stack are recomputed. | ||
| var assemblies = GetAssembliesWithMetadataUpdateHandlerAttributes(); | ||
| var sortedAssemblies = TopologicalSort(CollectionsMarshal.AsSpan(assemblies)); |
There was a problem hiding this comment.
There is no point in using CollectionsMarshal.AsSpan here. Change it to use a regular enumerator instead.
The potentially dangerous APIs like ``CollectionsMarshal.AsSpan` are meant to be only used for core that needs to be micro-optimized to the last cycle. It is not the case here. You are calling GetCustomAttributes inside the loop that is like 1000x times more expensive than the foreach.
|
|
||
| static void Visit(ReadOnlySpan<Assembly> assemblies, Assembly assembly, List<Assembly> sortedAssemblies, HashSet<string> visited) | ||
| { | ||
| var assemblyIdentifier = assembly.FullName ?? assembly.ToString(); |
There was a problem hiding this comment.
| var assemblyIdentifier = assembly.FullName ?? assembly.ToString(); | |
| var assemblyIdentifier = assembly.FullName!; |
Runtime loaded assemblies always have non-empty full names. This is just nullability annotations not being helpful.
|
|
||
| foreach (var item in deltas) |
There was a problem hiding this comment.
I had it updated to use a local and I was worried I would get complaints about capturing delegates. I reverted it - the original version had less code 👍🏽
There was a problem hiding this comment.
This is updated.
Also sent dotnet/runtime#52409 to update the runtime to use the new names
|
|
||
| foreach (var item in deltas) |
There was a problem hiding this comment.
I had it updated to use a local and I was worried I would get complaints about capturing delegates. I reverted it - the original version had less code 👍🏽
Contributes to dotnet/runtime#51545