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? >>>>>>>>>> >>>>>>>>>> >>>> >
