-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move ActivationFactory into WinRT.Runtime.dll #1390
Move ActivationFactory into WinRT.Runtime.dll #1390
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments 🙂
Just a couple nits but I do love this PR, and the fact |
b2783ce
to
9be15b9
Compare
760d15f
to
f9b5cd3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done another review pass on the updated diff 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SHIP IT!
|
||
public static bool TryLoad(string fileName, out DllModule module) | ||
{ | ||
lock (_cache) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is now more likely that we see parallel calls here due to different threads activating different types. Given these are already cached too, it shouldn't have too much of an impact, but will need to monitor perf traces to see if this shows up and if we need to improve the locking here. But for now, this is fine.
This enables additional optimizations that couldn't be done in #1375