+1 We are attempting the same on the non-Java clients too.
-Jake On Mon, Apr 3, 2017 at 1:30 PM Kirk Lund <kl...@apache.org> wrote: > My primary goal is to defer instantiating the Command instances until after > the Cache has initialized Security. I then have several options for > handling Security within the Commands that has much less performance impact > than it currently has. A major reconfiguration or lifecycle point, such as > closing and then recreating the Cache should be the only source of garbage > caused by throwing away the old instances and creating new ones. > > Removal of singletons primarily involves moving the instantiation code and > the lifecycle of the class (that's a singleton) further up the layers of > Geode code so it becomes driven by Cache initialization rather than by > class loading. Conceptually, you would still only have one instance of > SecurityService for the Cache, but the code that's responsible for that > "singularity" are the collaborators that own and interact with the > instance. This allows us to replace the instance very easily between unit > tests or between lifecycles of the Cache (which is where a user would > reconfigure things) or even make future changes to support multiple > instances for some component types that are currently singletons. > > What I'm planning is just a first pass at reducing our dependency on > singletons specifically to allow me to improve performance involving how > client Commands interact with Security (when security is enabled or > disabled). > > On Mon, Apr 3, 2017 at 11:02 AM, Bruce Schuchardt <bschucha...@pivotal.io> > wrote: > > > I'm not against refactoring the command classes. They originated from > the > > refactoring of a very large method that attempted to handle all client > > operations and, consequently, are currently stateless. You can't really > > hurt anything by creating multiple instances of them but please avoid > > creating unnecessary garbage. > > > > > > > > Le 4/3/2017 à 10:38 AM, Galen M O'Sullivan a écrit : > > > >> +1 to getting rid of singletons and non-constant use of static. Also to > >> code where the ownership semantics are clear. > >> > >> On Mon, Apr 3, 2017 at 10:30 AM, Jacob Barrett <jbarr...@pivotal.io> > >> wrote: > >> > >> +1 refactor > >>> On Mon, Apr 3, 2017 at 9:35 AM Michael William Dodge < > mdo...@pivotal.io> > >>> wrote: > >>> > >>> +1 to modular and questioning non-constant use of static > >>>> > >>>> On 3 Apr, 2017, at 09:27, Anthony Baker <aba...@pivotal.io> wrote: > >>>>> > >>>>> Using singletons leads to very monolithic systems that are hard to > test > >>>>> > >>>> and hard to change. Instead we should prefer modular services like > Udo > >>>> proposed. > >>>> > >>>>> I would go further and say that we should question any non-constant > use > >>>>> > >>>> of “static”. > >>>> > >>>>> Anthony > >>>>> > >>>>> On Apr 3, 2017, at 9:01 AM, Udo Kohlmeyer <ukohlme...@pivotal.io> > >>>>>> > >>>>> wrote: > >>>> > >>>>> Correct, that would be the definition. > >>>>>> > >>>>>> Yet, we find that our use of singletons within Geode is limiting to > >>>>>> > >>>>> say > >>> > >>>> that least. With the idea of wanting to be able to create/run multiple > >>>> cache instance within the same JVM (especially for testing) a > singleton > >>>> will be problematic. > >>>> > >>>>> In addition to that, the alternative is not that hard to construct > and > >>>>>> > >>>>> in many cases easier to manage. > >>>> > >>>>> --Udo > >>>>>> > >>>>>> > >>>>>> On 4/3/17 08:57, Jinmei Liao wrote: > >>>>>> > >>>>>>> Isn't "that instance is reused each invocation" my understanding > of a > >>>>>>> "singleton"? > >>>>>>> > >>>>>>> On Mon, Apr 3, 2017 at 11:49 AM, Udo Kohlmeyer < > >>>>>>> > >>>>>> ukohlme...@pivotal.io> > >>> > >>>> wrote: > >>>>>>> > >>>>>>> -1 For using singletons > >>>>>>>> > >>>>>>>> Using a Factory pattern you can avoid having to create singletons > in > >>>>>>>> addition to caching created commands to avoid the recreation of > the > >>>>>>>> instance. > >>>>>>>> > >>>>>>>> The SSLConfigurationFactory is a simple example where you create > an > >>>>>>>> instance when required. Once an instance is created, that instance > >>>>>>>> > >>>>>>> is > >>> > >>>> reused each invocation. > >>>>>>>> > >>>>>>>> --Udo > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> On 4/3/17 08:30, Jinmei Liao wrote: > >>>>>>>> > >>>>>>>> I think the client commands needs to be singleton instances even > >>>>>>>>> > >>>>>>>> after you > >>>> > >>>>> change the sequence of initialization. We don't want to have each > >>>>>>>>> > >>>>>>>> client > >>>> > >>>>> operation ends up creating a new command instance, right? That > >>>>>>>>> > >>>>>>>> would > >>> > >>>> be a > >>>> > >>>>> more performance drag. > >>>>>>>>> > >>>>>>>>> On Thu, Mar 30, 2017 at 2:14 PM, Kirk Lund <kl...@apache.org> > >>>>>>>>> > >>>>>>>> wrote: > >>> > >>>> PS: I'll be writing and using JMH benchmarks to drive these > >>>>>>>>> > >>>>>>>> changes. > >>> > >>>> I'll > >>>> > >>>>> also create new unit tests for each of these classes that don't > >>>>>>>>>> > >>>>>>>>> currently > >>>> > >>>>> have unit tests. > >>>>>>>>>> > >>>>>>>>>> On Thu, Mar 30, 2017 at 10:58 AM, Kirk Lund <kl...@apache.org> > >>>>>>>>>> > >>>>>>>>> wrote: > >>>> > >>>>> The client Commands now check with SecurityService even when > >>>>>>>>>> > >>>>>>>>> security is > >>>> > >>>>> not configured. This has introduced a negative performance > >>>>>>>>>>> > >>>>>>>>>> impact. > >>> > >>>> The best way to fix something like this is to tell the Command > >>>>>>>>>>> > >>>>>>>>>> instance > >>>> > >>>>> when it's being constructed that is does or does not need to > >>>>>>>>>>> > >>>>>>>>>> perform > >>>> > >>>>> security checks. > >>>>>>>>>>> > >>>>>>>>>>> Unfortunately, Commands are all implemented as singletons which > >>>>>>>>>>> > >>>>>>>>>> are very > >>>> > >>>>> eagerly instantiated during class loading of CommandInitializer > >>>>>>>>>>> > >>>>>>>>>> (who > >>>> > >>>>> thought that was a good idea?!). > >>>>>>>>>>> > >>>>>>>>>>> In order to fix this performance problem, I would need to get > rid > >>>>>>>>>>> > >>>>>>>>>> of > >>>> > >>>>> these > >>>>>>>>>> > >>>>>>>>>> problematic static initializer blocks that so eagerly construct > >>>>>>>>>>> > >>>>>>>>>> the > >>> > >>>> Commands so that I can put off constructing them until AFTER the > >>>>>>>>>>> > >>>>>>>>>> Cache > >>>> > >>>>> is > >>>>>>>>>>> initializing and specifically AFTER the Cache has determined if > >>>>>>>>>>> > >>>>>>>>>> it > >>> > >>>> is > >>>> > >>>>> using > >>>>>>>>>> > >>>>>>>>>> security or not. > >>>>>>>>>>> > >>>>>>>>>>> This means I'm going to have to do some refactoring of > >>>>>>>>>>> > >>>>>>>>>>> CommandInitializer, > >>>>>>>>>> > >>>>>>>>>> the Command classes, ServerConnection, AcceptorImpl, etc. > >>>>>>>>>>> > >>>>>>>>>>> Any other approach is going to have such minimal impact on > >>>>>>>>>>> > >>>>>>>>>> performance > >>>> > >>>>> that I'm not even interested in doing less than this. > >>>>>>>>>>> > >>>>>>>>>>> From a very high level, I would change the code so that the > >>>>>>>>>>> Cache > >>>>>>>>>>> > >>>>>>>>>> owns > >>>> > >>>>> the > >>>>>>>>>> > >>>>>>>>>> Server which owns the Command instances. In this way, the > >>>>>>>>>>> > >>>>>>>>>> configuring of > >>>> > >>>>> use of security can trickle down from Cache to each Command. I > >>>>>>>>>>> > >>>>>>>>>> would > >>>> > >>>>> primarily be altering static singletons, static initializers and > >>>>>>>>>>> > >>>>>>>>>> adding > >>>> > >>>>> constructors. > >>>>>>>>>>> > >>>>>>>>>>> Does anyone have a problem with me changing the above classes > and > >>>>>>>>>>> especially getting rid of the static initializers and singleton > >>>>>>>>>>> > >>>>>>>>>>> instances? > >>>>>>>>>> > >>>>>>>>>> > >>>> > > >
