Hi Murray
I'd like to clarify the main goal of my PR, which is to transition from
traditional synchronized blocks to the more modern ReentrantLocks. The
Synchronizer class is essentially a way to standardize this transition
across the codebase.
I agree with Juan Pablo's point about the benefits of switching to
ReentrantLock to make the most of virtual threads, especially with JDK 21
and beyond. The use of ReentrantLock can indeed offer more flexibility and
control, which can be advantageous in high-concurrency scenarios.
As for the performance aspect, while I haven't seen the generated bytecode
either, I believe the performance difference between ReentrantLock and
synchronized is often negligible for most applications. However,
ReentrantLock may offer better scalability, which could be beneficial as we
look to future-proof the code.
Murray, your point about the double-null check for singletons is
well-taken. The synchronized keyword has its merits, especially in simpler
use-cases where the overhead of creating a lock object and handling
exceptions might not be justified.
In summary, I don't have a strong opinion on one over the other for all
scenarios, but I do think that ReentrantLock offers advantages that align
well with the direction Java is taking with virtual threads, also I believe
that ReentrantLocks could offer us more flexibility and scalability as Java
continues to evolve.
Best regards,
Arturo
On Thu, Oct 12, 2023 at 5:09 PM Juan Pablo Santos Rodríguez <
juanpablo.san...@gmail.com> wrote:
> Hi Murray,
>
> to add some context to the PR, from what I understand, the important
> thing about the Synchronizer class, is more on the switch to use
> ReentrantLock instead of synchronized blocks, than the code
> readability/simplification. The Synchronizer class just captures an
> idiom throughout the code, but what's important is the change being
> carried, using ReentrantLocks instead of synchronized.
>
> As for the why of above, for the time now, we are not going to use
> virtual threads directly, but we can benefit from it indirectly when
> running under JDK21, request dispatching and I/O are two examples
> where we should see performance gains. And to that we should move away
> from synchronized sections (not from *every* synchronized section, a
> bit on that below). JEP 444 [#1] introduced Virtual Threads so is a
> good starting point to get an overall feeling of how they work and
> what they strive to deliver. Quoting from it:
>
> > [...] There are two scenarios in which a virtual thread cannot be
> unmounted during blocking operations because it is pinned to its carrier:
> >
> > When it executes code inside a synchronized block or method, or
> > When it executes a native method or a foreign function.
> >
> > Pinning does not make an application incorrect, but it might hinder its
> scalability. If a virtual thread performs a blocking operation such as I/O
> > or BlockingQueue.take() while it is pinned, then its carrier and the
> underlying OS thread are blocked for the duration of the operation. Frequent
> > pinning for long durations can harm the scalability of an application by
> capturing carriers.
> > The scheduler does not compensate for pinning by expanding its
> parallelism. Instead, avoid frequent and long-lived pinning by revising
> synchronized
> > blocks or methods that run frequently and guard potentially long I/O
> operations to use java.util.concurrent.locks.ReentrantLock instead. There
> is no
> > need to replace synchronized blocks and methods that are used
> infrequently (e.g., only performed at startup) or that guard in-memory
> operations.
> > As always, strive to keep locking policies simple and clear.
> > [...]
> > In a future release we may be able to remove the first limitation above,
> namely pinning inside synchronized.[...]
>
> (the carrier is the platform [normal / real / underlying] thread)
>
> So, generally, switching to ReentrantLock should be the way to go to
> benefit from virtual threads. The case you highlight, as per the jep,
> could be left as a it is, but I don't see much difference on
> performance there either. Most probably, the synchronized keyword is
> being translated under the hoods to the code generated by the
> synchronized class; I haven't seen the generated bytecode, so I may be
> completely wrong here, it's a gut feeling. I don't have an strong
> opinion whether to use one over the other (I'll defer Arturo to that),
> but since it seems that it could fit the idiom, and was done in the
> PR, which carries some significant work, I've lgtm'ed that when
> reviewing - but I'm more than happy to be corrected if it isn't a good
> solution.
>
> Regarding the increase in memory usage, as per [#2], a fully working
> WikiEngine weights around 5.5MB. The attached PR adds, say about, 40
> to 60 ReentrantLock, haven't counted them, which are either static or
> part of a WikiEngine manager, that is, those locks should not increase
> over time. I have no idea about the impact on memory that they will
> have, but hopefully we can run that test afterwards and observe how
> much that costs to the Engine and iterate over that. I expect that the
> memory increase should be tolerable, specially if it allows to benefit
> from virtual threads, but again, will be very grateful for ayone
> correcting me. And, on the particular case you mention, I have no
> strong opinion on which one should be used, I think both of them
> should be fine, but happy to be corrected if needed, I just wanted to
> put some context on the PR.
>
> As a side note, IIRC, the technique you describe for creating
> singletons is the same that enums use under the covers to create their
> values, so a very clean way to create singletons as of today is
> through enums, f.ex.:
>
> public enum IdGenerator {
> INSTANCE;
> public static getInstance() {
> return INSTANCE;
> }
> [...]
> }
>
> should be roughly the same as the example noted on the previous email.
>
>
> best regards,
> juan pablo
>
> [#1] https://openjdk.org/jeps/444
> [#2] https://lists.apache.org/thread/cqppw3bmgmthr86718yrhtz93kw3d2h1
>
>
> On Thu, Oct 12, 2023 at 12:38 AM Murray Altheim <murra...@altheim.com>
> wrote:
> >
> > Hi,
> >
> > My use of the synchronized block in WikiEventManager was based on a
> > section "Use Double-Checked Locking to Reduce Synchronization" of Craig
> > Larman and Rhett Guthrie's book "Java 2: Performance and Idiom Guide"*
> > about the use of a synchronized block on singleton classes protected by
> > a pair of null checks.
> >
> > I have been successfully using this approach in my own software designs
> > since the 90s, quoting from page 100:
> >
> > "Consider the example of a 'Singleton' method implementing the
> > Singleton pattern [GHJV 1995]. This is likely to be a high-use method,
> > but the critical section is in the rarely entered lazy initialization
> > block. It is not desirable to synchronize the method, when only a very
> > rarely entered block contains the critical section for which the
> > synchronization was required. Synchronizing the entire method adds
> > unnecessary overhead for all method invocations, and reduces
> > concurrency."
> >
> > What I take from this is somewhat of a cautionary advisory not to use
> > this new lock-based approach everywhere in JSPWiki, that a simple double-
> > null check was a means to avoid a synchronized block's overhead. Use of
> > locks certainly does have its place -- particularly where the use of the
> > lock spans methods -- but the creation of a lock object, the necessity
> for
> > an additional utility method call, the potential for performance hits,
> and
> > the additional possibility of exceptions and their handling when locks
> > cannot be obtained, all this additional complexity might suggest that at
> > least for this particular use in lazily creating singletons the
> > synchronized keyword probably makes the most sense.
> >
> > A typical existing usage of the double null check for singletons:
> >
> > /**
> > * Returns the single instance of this class.
> > * <p>
> > * If the singleton has already been generated it is returned.
> > * </p>
> > */
> > public static IdGenerator getInstance()
> > {
> > if ( instance == null ) {
> > synchronized (IdGenerator.class) { // see larman/guthrie
> p.100
> > if ( instance == null ) {
> > IdGenerator idg = new IdGenerator(IdType.BASE64);
> > return idg;
> > }
> > }
> > }
> > return instance;
> > }
> >
> > While use of the proposed Synchronizer method might appear to be a
> > simplification,
> >
> > public static IdGenerator getInstance()
> > {
> > if ( instance == null ) {
> > final ReentrantLock lock = new ReentrantLock();
> > Synchronizer.synchronize(lock, () -> {
> > IdGenerator idg = new IdGenerator(IdType.BASE64);
> > return idg;
> > }
> > lock.unlock();
> > }
> > return instance;
> > }
> >
> > Under the covers the replacement for this would be akin to:
> >
> > public static IdGenerator getInstance()
> > {
> > if ( instance == null ) {
> > final ReentrantLock lock = new ReentrantLock();
> > try {
> > if ( lock.tryLock(20, TimeUnit.MILLISECONDS) ) {
> > IdGenerator idg = new IdGenerator(IdType.BASE64);
> > return idg;
> > } else {
> > throw new RuntimeException("unable to obtain
> lock.");
> > }
> > } catch ( InterruptedException e ) {
> > throw new RuntimeException("the current thread was
> interrupted: " + e.getMessage());
> > } finally {
> > lock.unlock();
> > }
> > }
> > return instance;
> > }
> >
> > Though I'm entirely willing to entertain discussion to the contrary as
> I'm
> > possibly not understanding the proposal or its implementation. I'm also
> > sure that a book published in 1999 for Java 2 may have been superceded by
> > more current ideas about how to handle high performance concurrent
> > applications, and indeed, if anyone knows of a good book on the subject
> > that's up to date with JDK 21 I'd be interested in knowing about it.
> >
> > Murray
> >
> > *
> https://www.abebooks.com/9780130142603/Java-2-Performance-Idiom-Guide-0130142603/plp
> >
> > On 11/10/23 22:52, juanpablo-santos (via GitHub) wrote:
> > >
> > > juanpablo-santos commented on code in PR #307:
> > > URL: https://github.com/apache/jspwiki/pull/307#discussion_r1354460996
> > [...]
> > > ##########
> > >
> jspwiki-event/src/main/java/org/apache/wiki/event/WikiEventManager.java:
> > > ##########
> > > @@ -154,11 +168,13 @@ private WikiEventManager() {
> > > * @return A shared instance of the WikiEventManager
> > > */
> > > public static WikiEventManager getInstance() {
> > > - if( c_instance == null ) {
> > > - synchronized( WikiEventManager.class ) {
> > > - return new WikiEventManager();
> > > - // start up any post-instantiation services here
> > > - }
> > > + if (c_instance == null) {
> > > + Synchronizer.synchronize(lock, () -> {
> > >
> >
> >
> ...........................................................................
> > Murray Altheim <murray18 at altheim dot com> = =
> ===
> > http://www.altheim.com/murray/ ===
> ===
> > = =
> ===
> > In the evening
> > The rice leaves in the garden
> > Rustle in the autumn wind
> > That blows through my reed hut.
> > -- Minamoto no Tsunenobu
> >
>
