Hello Yash, Thank you for your work on this so far. It's great to see people focusing on refactoring, which I think should probably be the top priority for all of us.
I will review the JIRAs some more over the coming days, but I have a concern that some of the patches are very large. We had many discussions in the past about focused refactoring vs. general trends. Focused refactoring means you go after a specific piece of code like a class or group of related classes / artifacts and fixing them. General trends, on the other hand, means that you identify a certain pattern and then making a sweeping change across the entire code base. General trends refactorings can be very dangerous, because you are running after a "trend" not isolating a specific piece of code and fixing it. So my recommendation, especially for the bigger patches that you have, is to redesign / refactor so that it is a topic-based, not trend-based. We need to make these commits in isolated bite-sized chunks that focus on a specific area instead of a specific trend (make public to private, add try-with-resources, or whatever else) Cheers, Taher On Thu, Apr 19, 2018 at 3:24 PM, Yash Sharma <yash.sha...@hotwaxsystems.com> wrote: > Hi Devs, > > Here is the detailed information about the things I am working on for > performance optimization in our OFBiz code. > > *1.) Downsize Accessibility Scope* > I've tried to downsize accessibility scope of classes, interfaces, abstract > class, declared member variables, enumerations, methods, and constructors > to as minimum as possible as per OFBIz current implementation, still there > is a lot of scope for improvement but it would require changes at the > granular level. I've used this > <https://docs.oracle.com/javase/tutorial/java/javaOO/accesscontrol.html> as > my reference point. example: > > - public void noteKeyRemoval(UtilCache<K, V> cache, K key, V oldValue); > + void noteKeyRemoval(UtilCache<K, V> cache, K key, V oldValue); > > Limiting the scope of the method from public modifier to package level. > > *2.) Using Lambda Expressions* > Then tried to use lambda expressions on simple functional work to leverage > implicit type of coding an example: > > - Map<String, String> initParameters = new LinkedHashMap<>(); > - for (Element e : initParamList) { > - initParameters.put(e.getAttribute("name"), > e.getAttribute("value")); > - } > + Map<String, String> initParameters = > initParamList.stream().collect(Collectors.toMap(e -> > e.getAttribute("name"), e -> e.getAttribute("value"), (a, b) -> b, > LinkedHashMap::new)); > > > Some of the key benefits of using lambdas will introduce Functional > style over Imperative style > <https://stackoverflow.com/questions/2078978/functional-programming-vs-object-oriented-programming>, > we can use method referencing > <https://docs.oracle.com/javase/tutorial/java/javaOO/methodreferences.html>, > usage of aggregate operations, and it will help developers to write > memory efficient code. > > > *3.) Using Type Inference* > Java uses type inference so to make code lightweight I've updated > code constructs as shown in the example for more on this refer this article > <https://docs.oracle.com/javase/tutorial/java/generics/genTypeInference.htmlv> > . > > - Map<String, ? extends Object> systemProps = > UtilGenerics.<String, Object> checkMap(System.getProperties()); > + Map<String, ?> systemProps = > UtilGenerics.checkMap(System.getProperties()); > > > *4.) Use of single quote for character* > There is a significant usage of <"Single Character"> in the codebase for > example: > > - throw new GenericConfigException("Error opening file at > location [" + fileUrl.toExternalForm() + "]", e); > + throw new GenericConfigException("Error opening file at > location [" + fileUrl.toExternalForm() + ']', e); > > > "]" is comparativlelly slower then ']' Java internally uses Flyweight > Design pattern to create String literals so for every call it will not > create a new Object and used an existing one this will improve > performace to some extend an study can be seen on this > <https://stackoverflow.com/questions/24859500/concatenate-char-literal-x-vs-single-char-string-literal-x> > page. > > > *5.) Updated Variable Declaration* > > Lastly some of the variable declaration is updated this doesn't create > a huge difference but helps JVM at the from implicit conversion. > > - private long cumulativeEvents = 0; > + private long cumulativeEvents = 0L; > > > Based on above findings, I have done some code improvement and > provided following patches. *And need community help for reviewing > these changes.* > Kindly provide any improvents or suggestion you have in mind :) > > > 1. [OFBIZ-10344] Refactoring Variable Scope for > org.apache.ofbiz.base package > <https://issues.apache.org/jira/browse/OFBIZ-10344> > 2. [OFBIZ-10345] Refactoring Variable Scope for > org.apache.ofbiz.catalina.container > <https://issues.apache.org/jira/browse/OFBIZ-10345> > 3. [OFBIZ-10346] Refactoring Variable Scope for > org.apache.ofbiz.common > <https://issues.apache.org/jira/browse/OFBIZ-10346> > 4. [OFBIZ-10347] Refactoring Variable Scope for > org.apache.ofbiz.datafile > <https://issues.apache.org/jira/browse/OFBIZ-10347> > 5. [OFBIZ-10348] Refactoring Variable Scope for > org.apache.ofbiz.entity > <https://issues.apache.org/jira/browse/OFBIZ-10348> > > > P.S. Apart from this I am also working on performance matrix and will share > it soon. > > > Thanks & Regards, > > -- > *Pradhan Yash Sharma* > > > On Tue, Apr 17, 2018 at 11:28 AM, Yash Sharma <yash.sha...@hotwaxsystems.com >> wrote: > >> Thank you for the feedback I've created a Jira ticket OFBIZ-10343 >> <https://issues.apache.org/jira/browse/OFBIZ-10343> and I will add >> patches for the same for your review. >> >> Thanks & Regards, >> -- >> *Pradhan Yash Sharma* >> >> On Fri, Apr 13, 2018 at 5:50 PM, Taher Alkhateeb < >> slidingfilame...@gmail.com> wrote: >> >>> Hello Pradhan, >>> >>> Refactoring is exactly what we need and is a welcomed activity. I >>> think we should, however, try to avoid "big ideas" across the entire >>> code base. The subject of your message is the reason why I say that. >>> >>> So, if you want to start refactoring, I suggest to start with one >>> piece of code, study it careful, issue a JIRA, and provide a patch. >>> This should be focused similar to your notes on UtilCache. >>> >>> On Fri, Apr 13, 2018 at 12:14 PM, Pradhan Yash Sharma >>> <pradhanyashsharm...@gmail.com> wrote: >>> > Hello, >>> > >>> > While I was working on UtilCache.java file came across some >>> improvements, >>> > they are as follows: >>> > >>> > 1) Method and Variable access modifiers can be narrowed down to private >>> > access modifier. >>> > >>> > 2) Then AtomicLong can be given value 0L instead of 0. >>> > >>> > 3) Some Variables is used in both synchronized and unsynchronized >>> blocks, >>> > so they can be declared final. eg, >>> > >>> > >>> > >>> > *protected AtomicLong hitCount = new AtomicLong(0); >>> private >>> > final AtomicLong hitCount = new AtomicLong(0L);* >>> > One variable was able to get most of my attention is >>> > >>> > * protected ConcurrentMap<Object, CacheLine<V>> >>> memoryTable >>> > = null;* >>> > >>> > This is used in synchronized and unsynchronized blocks, this Object can >>> be >>> > converted into ThreadLocal or AtomicReference but it would require >>> changes >>> > in the current implementation as well. >>> > >>> > Lastly, there is extensive use of for loops for iteration we can use >>> Java 8 >>> > Streams, Collector, and other functions to leverage implicit looping >>> > mechanism. >>> > >>> > >>> > -- >>> > Pradhan Yash Sharma >>> >> >>
