Re: synchronized and Loom

2022-06-25 Thread Stefan Bodewig
On 2022-06-06, Jaikiran Pai wrote:

> My personal opinion is that we continue with our release plans
> (whenever we do it) instead of waiting for completing these
> changes. The virtual threads feature is a preview feature in JDK 19,
> so I think as long as our tests pass and Ant is usable in Java 19 with
> --enable-preview, I think that's a good enough for an Ant release.

Not having looked into Loom too much I thought we had to remove all uses
of synchronized - largely by seeing some change made else-place. You
seem to have a much better grasp on what Loom means to a code-base than
I have, I fully trust your judegment.

Thanks

Stefan

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



Re: synchronized and Loom

2022-06-06 Thread Jaikiran Pai

Hello Stefan,

My personal opinion is that we continue with our release plans (whenever 
we do it) instead of waiting for completing these changes. The virtual 
threads feature is a preview feature in JDK 19, so I think as long as 
our tests pass and Ant is usable in Java 19 with --enable-preview, I 
think that's a good enough for an Ant release.


As for doing the actual changes, I think we will have to review these 
call paths in core and individual tasks. Specifically, I think we need 
to understand when/where a virtual thread might get used during a build 
in a regular use case. The other thing I am hoping with our Ant release 
is that we get actual users to start using the version with their own 
builds/projects and tell us if/how the current release doesn't play well 
(in terms of performance etc...) when it comes to virtual threads.


Given that this all boils down to "pinning" of the thread and since the 
JEP 425 states:


"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."


I think we should take our time to review these usages on a per use 
basis (I realize 500 odd is too big to do on a per use basis, but I 
think once we do the first few necessary changes, there should be more 
clarity on which ones to focus on next).


-Jaikiran

On 06/06/22 4:19 pm, Stefan Bodewig wrote:

Hi

right now the Loom prototype doesn't play well with synchronized and the
recommended approach is to use ReentrantLock instead. A quick grep over
Ant's codebase turns up more than 500 hits for synchronized - many of
which in method declarations. So updating it will be a bigger task, in
particular if we wanted to take the time to think through the reasons
for synchronization and whether splitting read/write locks may be
useful.

Do you feel we should do this before the next release or should we defer
it? Another option might be to mechanically replace synchronized with
reentrantlock now and do the "read/write lock separation would be good"
assessment later.

Stefan

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



synchronized and Loom

2022-06-06 Thread Stefan Bodewig
Hi

right now the Loom prototype doesn't play well with synchronized and the
recommended approach is to use ReentrantLock instead. A quick grep over
Ant's codebase turns up more than 500 hits for synchronized - many of
which in method declarations. So updating it will be a bigger task, in
particular if we wanted to take the time to think through the reasons
for synchronization and whether splitting read/write locks may be
useful.

Do you feel we should do this before the next release or should we defer
it? Another option might be to mechanically replace synchronized with
reentrantlock now and do the "read/write lock separation would be good"
assessment later.

Stefan

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org