scottyuancoc commented on PR #10: URL: https://github.com/apache/sling-org-apache-sling-scripting-javascript/pull/10#issuecomment-3142725008
Hello @rombert and @joerghoh, Thank you for sharing the references and your insights. After reviewing the materials you provided, along with additional sources including: - [LCK10-J: Use a correct form of the double-checked locking idiom](https://wiki.sei.cmu.edu/confluence/display/java/LCK10-J.+Use+a+correct+form+of+the+double-checked+locking+idiom) - [Effective Java: Use Lazy Initialization Judiciously](https://dev.to/kylec32/effective-java-use-lazy-initialization-judiciously-23h8) - The original lazy initialization implementation from Effective Java™, 2nd Edition by Joshua Bloch - The concurrent LazyInitializer implementation from Apache Commons Lang 3 Based on these, I tested the following Java implementation using the volatile keyword to ensure thread-safe lazy initialization: `// Use a temporary variable to reduce the number of reads of the volatile field.` `NativeJavaObject tempNjo = njo;` `if (tempNjo == null) {` ` synchronized (this) {` ` tempNjo = njo;` ` if (tempNjo == null) {` ` njo = tempNjo = new NativeJavaObject(start, wrapped, getStaticType());` ` }` ` }` `}` `return tempNjo.get(name, start);` This implementation uses the double-checked locking idiom to ensure thread safety. However, it triggers [SonarQube rule java:S3077](https://sonarcloud.io/organizations/apache/rules?open=java%3AS3077&rule_key=java%3AS3077). If we choose to adopt this pattern, we may need to suppress the warning or document a rationale for its use. As an alternative—as suggested by @joerghoh—we can use an AtomicReference<NativeJavaObject> with a special NULL_OBJECT placeholder. This intermediate state helps avoid repeated instantiation of NativeJavaObject in each compareAndSet(...) operation. To compare performance, I ran mvn test with JDK 21 across ten iterations for each of the four approaches. The average execution times were: 1. AtomicReference with NULL_OBJECT placeholder: 9.2823 seconds 2. Synchronized method approach: 9.3049 seconds (~0.243% slower than #1) 3. AtomicReference without NULL_OBJECT: 9.3143 seconds (~0.345% slower than #1) 4. Volatile double-checked locking: 9.4039 seconds (~1.310% slower than #1) These results were somewhat surprising, as the volatile double-check idiom is generally expected to offer better performance. However, repeated tests produced consistent trends. While this is not a definitive benchmark, it serves as a useful approximation. Based on these findings, the current PR adopts the AtomicReference with NULL_OBJECT placeholder approach for its balance of clarity, safety, and performance—while avoiding issues flagged by static analysis tools. From a practical standpoint, in my use case, this module is part of a Sling-based solution that powers high-profile websites developed by JavaScript teams. Reliability is a critical success factor in such environments, and improving the robustness of this component can have a meaningful impact. I hope this analysis is helpful to the change review process and supports our decision-making. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
