Return of RAR metadata caching#2222
Conversation
There was a problem hiding this comment.
I'd name it assemblyMetadataCache. Though if it's going to get rewritten anyway, maybe not important :)
Shouldn't GetTargetFrameworkAttribute get cached as well? Included in the metadata data class. Refers to: src/Tasks/AssemblyDependency/AssemblyInformation.cs:222 in d541b3f. [](commit_id = d541b3fde95571060de73d4ef91576029c729490, deletion_comment = False) |
Yes, but it doesn't seem to be called anywhere. So I didn't bother. |
jeffkl
left a comment
jeffkl
left a comment
There was a problem hiding this comment.
Open an item to circle back to the AssemblyInformation class to get rid of AssemblyMetadata.
Are there any other usages of the AssemblyInformation constructor? If yes, it might be worth beefing up AssemblyMetadata with the new usages. Refers to: src/Tasks/AssemblyDependency/AssemblyInformation.cs:65 in d541b3f. [](commit_id = d541b3fde95571060de73d4ef91576029c729490, deletion_comment = False) |
|
No, no other uses of the AssemblyInformation constructor, so I think this is reasonably complete. |
rainersigwald
force-pushed
the
features/rar-cache-assemblyinfo
branch
2 times, most recently
from
9db14d7 to
20cb6af
Compare
|
@dotnet-bot test Ubuntu16.04 Build for CoreCLR please |
Improved implementation of 55f846e: doesn't hold handles to the assemblies after reading their metadata. Long term, more functionality could be moved into AssemblyMetadata and out of AssemblyInformation, and `GetAssemblyMetadata()` could be modified to return an `AssemblyMetadata` instead of its three out parameters. But that's a risky change to make at this point in the cycle.
|
I did some benchmarks with and without this change on top of current vs15.3 (520d1aa). Testbed is cli
That's about a 3% gain, which is nothing to sneeze at but doesn't meet the bar for 15.3 at the moment, so I'll retarget this PR to master. |
rainersigwald
force-pushed
the
features/rar-cache-assemblyinfo
branch
from
20cb6af to
649cbbc
Compare
|
@dotnet-bot test Windows_NT Build for CoreCLR please |
1 similar comment
|
@dotnet-bot test Windows_NT Build for CoreCLR please |
|
@dotnet-bot test this please |
4 participants
My previous attempt at this in #2192 had a bad side effect: it kept handles within
AssemblyInformationobjects live in the cache past the point of RAR returning, because nothing disposed them. They should have been cleaned up eventually, if finalizers ran after GC, but that wasn't sufficient to keep an internal test that did a rebuild in VS working--it failed copying from obj to bin because a the file in bin was held in use.This is a cleaner approach: wrap the extraction process as it was done before, and cache an object consisting only of the results.
That "object consisting only of the results" seems like it should just be
AssemblyInformationinstead of a new type. I introduced the new type to minimize churn and maximize understandability of this change in the short term.Relates to #2015.