Re: [PR] [JSPWIKI-1178] Address potential deadlock with JDK 21 Virtual Threads. [jspwiki]
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 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
Re: [PR] [JSPWIKI-1178] Address potential deadlock with JDK 21 Virtual Threads. [jspwiki]
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
[PR] Replace 'size() == 0' with 'isEmpty()' [jspwiki]
arturobernalg opened a new pull request, #314: URL: https://github.com/apache/jspwiki/pull/314 (no comment) -- 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...@jspwiki.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [JSPWIKI-1178] Address potential deadlock with JDK 21 Virtual Threads. [jspwiki]
HI Arturo and Juan Pablo,
I don't disagree with anything either of you have written. I'm all in
favour of the ReentrantLock replacement via a Synchronizer pattern.
I'm only suggesting, as does both Larman/Guthrie and the quoted section
("There is no need to replace synchronized blocks and methods that are
used infrequently (e.g., only performed at startup)..."), that the
double-null-checked synchronized blocks used to protect singletons is
likely still a simpler and better practice, and there'd be little
benefit of a ReentrantLock in those cases. These are only called at
the moment when the singleton is created, i.e., once per session.
Cheers,
Murray
On 13/10/23 06:08, Arturo Bernal wrote:
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.[...]
...
Murray Altheim= = ===
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
