> On March 9, 2017, 6:27 p.m., Kirk Lund wrote: > > Here's the change I recommend for improving performance. The problem is > > this line... > > > > private static SecurityService defaultInstance = new > > IntegratedSecurityService(); > > > > It always installs a IntegratedSecurityService. Branch here and install > > IntegratedSecurityService iif we have security enabled; else we install > > NullSecurityService which is a no-op that does nothing but return false, > > etc. > > Jinmei Liao wrote: > but we need the object to determine whether it is enabled or not. With it > being null, how can we determine if we have security enabled or not? > > Kirk Lund wrote: > It's not null, the name is NullSecurityService because it follows the > Null Object Pattern. Imagine if you call it NoOpSecurityService or > NoSecurityService. > > Kirk Lund wrote: > And it requires further refactoring. The static "getSecurityService" > would need to move to a different class. > > SecurityService securityService = SecurityService.getSecurityService(); > > If Security is enabled then the above returns an instance of > IntegratedSecurityService. > > If Security is disabled then the above returns an instance of > NullSecurityService. All of the methods return false, null or if return type > is void then the method is simply empty. JIT will eventually remove the calls > to methods that are empty so runtime gets even faster. > > Jinmei Liao wrote: > Oh, I see what you mean. As it requires more refactoring (pulling the > code to initSecurity, and check for isIntegratedSecurity out of the > interface), can we do this for now, and leave the ticket open for this > further improvement? > > Kirk Lund wrote: > Definitely. We'll probably want to do this iteratively. We should look > into writing a new JMH benchmark to help with this process.
+1 for JMH testing. We need to drive performance changes from data and JMH is great for assessing performance through micro-benchmarks. Concretely, I'd like to see this called out in the commit comment, e.g. "Implement null object pattern for security to optimize performance. Results in an improvement of up to XXX% when security is disabled." - Anthony ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57466/#review168485 ----------------------------------------------------------- On March 9, 2017, 3:52 p.m., Jinmei Liao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57466/ > ----------------------------------------------------------- > > (Updated March 9, 2017, 3:52 p.m.) > > > Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, and Kirk > Lund. > > > Repository: geode > > > Description > ------- > > * clean up interface, reduce function call > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java > 8366930a3bf6a244d077c745fc810d77c4699c38 > > geode-core/src/main/java/org/apache/geode/internal/security/SecurityService.java > 14784c391212095413c0d577cfc65de7247080b5 > > geode-core/src/main/java/org/apache/geode/management/internal/security/AccessControlMBean.java > 6514a33e52611994ddc16a58414146ebaad75c65 > geode-core/src/main/java/org/apache/geode/security/ResourcePermission.java > 45da464419779773c9116d824fcf11774bafbd79 > > > Diff: https://reviews.apache.org/r/57466/diff/1/ > > > Testing > ------- > > precheckin successful > > > Thanks, > > Jinmei Liao > >
