Skip to content

Conversation

@jlaanstra
Copy link
Collaborator

This enables additional optimizations that couldn't be done in #1375

@jlaanstra jlaanstra changed the title Move ActivationFacory into WinRT.Runtime.dll Move ActivationFactory into WinRT.Runtime.dll Nov 18, 2023
Copy link
Member

@Sergio0694 Sergio0694 left a 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 🙂

@Sergio0694
Copy link
Member

Just a couple nits but I do love this PR, and the fact ActivationFactory.Get(string) is now public! 🎉

@jlaanstra jlaanstra force-pushed the user/jlaans/factory-opt branch from b2783ce to 9be15b9 Compare January 24, 2024 20:17
@jlaanstra jlaanstra force-pushed the user/jlaans/factory-opt branch from 760d15f to f9b5cd3 Compare January 24, 2024 20:31
Copy link
Member

@Sergio0694 Sergio0694 left a 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 🙂

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SHIP IT! :shipit:


public static bool TryLoad(string fileName, out DllModule module)
{
lock (_cache)
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants