Skip to content
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

Reduce lock contention while generating new proxy types #484

Merged
merged 1 commit into from
Apr 2, 2020

Conversation

tangdf
Copy link
Contributor

@tangdf tangdf commented Feb 24, 2020

No description provided.

@tangdf
Copy link
Contributor Author

tangdf commented Apr 2, 2020

Review the code please.

Copy link
Member

@jonorossi jonorossi left a comment

Choose a reason for hiding this comment

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

Was this change driven by any performance profiling, or just reading of the code? I'd be interested to know what performance impact this change will have, and which usage of SynchronizedDictionary has a factory delegate that causes the impact.

I don't foresee any problems merging this change since only one thread can have an upgradeable read lock.

Tip for the future, please add some information about what and why you are proposing a change, means that maintainers can more quickly review your proposed change, and therefore increasing the likelihood it'll get merged quickly.

Thanks

@jonorossi jonorossi added this to the vNext milestone Apr 2, 2020
@jonorossi jonorossi merged commit 0efb1ef into castleproject:master Apr 2, 2020
@tangdf
Copy link
Contributor Author

tangdf commented Apr 2, 2020

   public class UserAppService
   {
       public virtual int Add(UserDto userDto)
       {
           //add to database;
           return 0;
       }
   }

   public class RoleAppService
   {
       public virtual int Add(RoleDto roleDto)
       {
           //add to database;
           return 0;
       }
   }

   public static class ServiceFactory
   {
       private static readonly IProxyBuilder s_proxyBuilder = new DefaultProxyBuilder(new ModuleScope(false, true));

       private static readonly ProxyGenerationOptions s_Options = new ProxyGenerationOptions(new PipelineServiceProxyHook());

       public static TService CreateService<TService>()
       {
           IInterceptor interceptor = new PipelineInterceptor();

           ProxyGenerator generator = new ProxyGenerator(s_proxyBuilder);

           return (TService) generator.CreateClassProxy(typeof(TService), s_Options, interceptor);
       }
   }
            //demo code

            //thread 1
            UserAppService userAppService = ServiceFactory.CreateService<UserAppService>();

            userAppService.Add(new UserDto());

            //thread 2
            RoleAppService roleAppService = ServiceFactory.CreateService<RoleAppService>();

            roleAppService.Add(new RoleDto());

When the proxy class of type UserAppService is generated, but when generating the proxy class of type RoleAppService , it prevents the creation of an instance of UserAppService.

@jonorossi
Copy link
Member

When the proxy class of type UserAppService is generated, but when generating the proxy class of type RoleAppService , it prevents the creation of an instance of UserAppService.

Ah yes, thanks. I'll update the changelog.

@jonorossi jonorossi changed the title Shorten lock time Reduce lock contention while generating new proxy types Apr 2, 2020
@stakx
Copy link
Member

stakx commented Apr 2, 2020

First off, apologies for having kept silent about this PR. The reason being that when I first saw this proposed change, it didn't feel like one that was entirely safe to make. It might be a good idea to look beyond SynchronizedDictionary and verify that locking in DynamicProxy will still work as expected. At surface level however, this LGTM. 👍

@jonorossi
Copy link
Member

@stakx no problem, I've been MIA lately anyway.

I wondered that too when I first looked at it. Since this doesn't change the external semantics of SynchronizedDictionary, I then looked at BaseProxyGenerator.ObtainProxyType and it accesses the naming scope and calls GenerateType delegates all inside the same lock, now an upgradeable read lock rather than the write lock but still synchronised, I then confirmed that only generators use naming scopes since they aren't thread safe (i.e. can't read while writing), yes they are the only thing using it. I don't foresee any problems.

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.

None yet

3 participants