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
>
