Re: RFR: 8266846: Add java.time.InstantSource

[Ralph Goers]

On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne  
wrote:

> 8266846: Add java.time.InstantSource

Can you possibly find a way that this can be used in a garbage free manner?  
Providing a MutableInstant interface that allows the Instant object to be 
provided and have its values set by a currentInstant(MutableInstant instant) 
method would solve the problem nicely for Log4j - or any other app that is 
trying to avoid allocating new objects.

-

PR: https://git.openjdk.java.net/jdk/pull/4016

Re: RFR: 8266846: Add java.time.InstantSource

[Istvan Neuwirth]

On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne  
wrote:

> 8266846: Add java.time.InstantSource

src/java.base/share/classes/java/time/InstantSource.java line 93:

> 91:  * @since 17
> 92:  */
> 93: public interface InstantSource {

Should not we add `@FunctionalInterface`? I can easily imagine this interface 
being used in tests where we can define the `InstantSource` with lambdas.

-

PR: https://git.openjdk.java.net/jdk/pull/4016

Re: JEP draft: Scope Locals

[Peter Levart]

Hi,

So if scopeLocal.get() is called in a thread outside any .run() scope in 
that thread, it will throw exception and scopeLocal.isBound() will 
return false. Unless it was called on an inheritableScopeLocal which 
inherited binding from parent thread.


What if I wanted to create and start a thread that would be "pristine" - 
not have any ScopeLocal value bound? Is this intentionally not allowed 
in order to make sure that inheritable ScopeLocal(s) can't be cleared by 
code that has no access to ScopeLocal instance(s)?


Another question: Suppose there are two inheritable ScopeLocal variables 
with bound values in scope (A and B) when I take a snapshot:


var snapshot = ScopeLocal.snapshot();

now I pass that snapshot to a thread which does the following:

ScopeLocal
    .where(C, "value for C")
    .run(() -> {
        System.out.printf("A:%s B:%s C:%s\n", A.isBound(), B.isBound(), 
C.isBound());

        ScopeLocal.runWithSnapshot​(() -> {
            System.out.printf("A:%s B:%s C:%s\n", A.isBound(), 
B.isBound(), C.isBound());

        }, snapshot);
        System.out.printf("A:%s B:%s C:%s\n", A.isBound(), B.isBound(), 
C.isBound());

    });

What would this code print?

...in other words, does runWithSnapshot replace the whole set of bound 
values or does it merge it with existing set?



Peter

On 5/12/21 4:42 PM, Andrew Haley wrote:

There's been considerable discussion about scope locals on the loom-dev list,
and it's now time to open this to a wider audience. This subject is important
because. although scope locals were motivated by the the needs of Loom, they
have many potential applications outside that project.

The draft JEP is at

https://bugs.openjdk.java.net/browse/JDK-8263012

I've already received some very helpful suggestions for enhancements to
the API, and it'll take me a while to work through them all. In particular,
Paul Sandoz has suggested that I unify the classes Snapshot and Carrier,
and it will take me some time to understand the consequences of that.

In the meantime, please have a look at the JEP and comment here.


For reference, earlier discussions are at

https://mail.openjdk.java.net/pipermail/loom-dev/2021-March/002268.html
https://mail.openjdk.java.net/pipermail/loom-dev/2021-April/002287.html
https://mail.openjdk.java.net/pipermail/loom-dev/2021-May/002427.html

Re: JEP draft: Scope Locals

[Remi Forax]

Apart the performance, threadLocal has currently two issues that are addressed 
by scope locals.

ThreadLocal let the programmer defines the lifecycle, while it seems great on 
paper, it's a huge mess in reality.
Because not everybody wraps the calls to set()/remove() in a try/finally, so 
the values inside thread locals are not reclaimed by the GC.
It was less an issue before pooling because all the values are reclaimed when 
the thread die, but with pooling, threads do not die.
(I think this is the point made by Brian about ThreadLocal not interacting 
smoothly with ThreadLocal)

InheritableThreadLocal copies everything each time a new thread is created, 
again this lead to memory leaks because
if an unexpected threads is created, sadly some libraries do that, all values 
are now referenced by this new thread that may never die.

If you want more
https://www.google.com/search?hl=en=ThreadLocal%20memory%20leak

I think the JEP should be more explicit about the shortcoming of ThreadLocal 
and how the design of scope local fix both issues.

Rémi

- Mail original -
> De: "Brian Goetz" 
> À: "Andrew Haley" , "core-libs-dev" 
> , "loom-dev"
> 
> Envoyé: Mercredi 12 Mai 2021 20:57:33
> Objet: Re: JEP draft: Scope Locals

> Scope locals have come together nicely.
> 
> I have some vague thoughts on the presentation of the JEP draft.  There
> are (at least) three intertwined things in the motivation:
> 
>  - ThreadLocal (and ITL especially) were always compromises, and with
> the advent of Loom, have become untenable -- but the need for implicit
> parameters has not gone away
>  - Scoped locals, because of their effective finality and dynamic
> scoping, offer a programming model that is a better fit for virtual
> threads, but, even in the absence of virtual threads, are an enabler for
> structured concurrency
>  - The programming model constraints enable a better-performing
> implementation
> 
> In reading the draft, these separate motivations seem somewhat tangled.
> All the words make sense, but a reader has a hard time coming away with
> a clear picture of "so, why did we do this exactly, besides that its
> cool and faster?"
> 
> A possible way to untangle this is:
> 
>  - the underlying use cases: various forms of implicit context
> (transaction context, implicit parameters, leaving breadcrumbs for your
> future self.)
>  - the existing solution: thread locals.  ThreadLocals are effectively
> mutable per-thread globals.  The unconstrained mutability makes them
> hard to optimize.  ThreadLocals interact poorly with pooled threads.
>  - Here comes Loom!  We no longer need to pool threads.  So, why are
> TLs not good enough?
>  - The more constrained programming model of SLs enables two big new
> benefits:
>    - structured concurrency, which is applicable to virtual and
> non-virtual threads alike
>    - better optimization of inheritance and get operations
> 
> 
> 
> 
> 
> 
> 
> On 5/12/2021 10:42 AM, Andrew Haley wrote:
>> There's been considerable discussion about scope locals on the loom-dev list,
>> and it's now time to open this to a wider audience. This subject is important
>> because. although scope locals were motivated by the the needs of Loom, they
>> have many potential applications outside that project.
>>
>> The draft JEP is at
>>
>> https://bugs.openjdk.java.net/browse/JDK-8263012
>>
>> I've already received some very helpful suggestions for enhancements to
>> the API, and it'll take me a while to work through them all. In particular,
>> Paul Sandoz has suggested that I unify the classes Snapshot and Carrier,
>> and it will take me some time to understand the consequences of that.
>>
>> In the meantime, please have a look at the JEP and comment here.
>>
>>
>> For reference, earlier discussions are at
>>
>> https://mail.openjdk.java.net/pipermail/loom-dev/2021-March/002268.html
>> https://mail.openjdk.java.net/pipermail/loom-dev/2021-April/002287.html
>> https://mail.openjdk.java.net/pipermail/loom-dev/2021-May/002427.html

Re: JEP draft: Scope Locals

[Andrew Haley]

On 5/15/21 10:10 AM, Alex Otenko wrote:
> ScopeLocal.where(...).run(...).finally(...) would be nice.

Is that different from try ... run ... finally ?

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: JEP draft: Scope Locals

[Andrew Haley]

On 5/14/21 8:19 PM, Mike Rettig wrote:
> I don't see a way to capture the lifecycle of the scoped variable. I think
> for framework designers it's going to be important to know the life cycle
> of the scoped variable especially with snapshotting and inheritance.  The
> lack of a defined life cycle for thread locals is one of the many reasons
> why it should be avoided.

This requirement sounds similar, but I think it's really a different idea.
Scope locals are only binding data to names and don't attempt to handle
lifetime issues. Apart from anything else, that would require a more
heavyweight mechanism than scope locals, which need to be very lightweight
for some applications.

Over at Loom we've been talking about lifetimes and Ron has some ideas, but
scope locals are not likely to be that thing.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: 8266846: Add java.time.InstantSource

[Michael Hixson]

On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne  
wrote:

> 8266846: Add java.time.InstantSource

src/java.base/share/classes/java/time/InstantSource.java line 68:

> 66:  * @implSpec
> 67:  * This interface must be implemented with care to ensure other classes 
> operate correctly.
> 68:  * All implementations that can be instantiated must be final, immutable 
> and thread-safe.

(I'm not an openjdk reviewer)

I'm wondering if we can revisit this "immutable" requirement as it is being 
shuffled from `Clock` to `InstantSource`.

This precludes an implementation that might be useful for testing, where the 
time provided by a single `InstantSource` instance can be adjusted manually in 
the test code.  To me that is a notable downside (worth breaking the contract 
for), and it's not so clear what the upside is.  "Immutable" seems like an odd 
way to describe the system clock anyway.

Do you know if the people who already use their own `InstantSource` / 
`TimeSource` / `Supplier` have a mutable version that they use in 
tests?  I would be surprised if they're all satisfied with 
`InstantSource.fixed`, but perhaps my intuition is wrong here.

-

PR: https://git.openjdk.java.net/jdk/pull/4016

Re: RFR: 8266846: Add java.time.InstantSource

[Stephen Colebourne]

On Fri, 14 May 2021 07:19:03 GMT, Anthony Vanelverdinghe 
 wrote:

>> 8266846: Add java.time.InstantSource
>
> src/java.base/share/classes/java/time/Clock.java line 513:
> 
>> 511:  * {@link System#currentTimeMillis()} or equivalent.
>> 512:  */
>> 513: static final class SystemInstantSource implements InstantSource, 
>> Serializable {
> 
> Is it possible to declare this as an enum? As doing so would simplify its 
> implementation.

My feeling is that in this case it is better to keep control of the kind of 
class being used.

-

PR: https://git.openjdk.java.net/jdk/pull/4016

Re: RFR: 8266846: Add java.time.InstantSource

[Naoto Sato]

On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne  
wrote:

> 8266846: Add java.time.InstantSource

src/java.base/share/classes/java/time/Clock.java line 2:

> 1: /*
> 2:  * Copyright (c) 2012, 2019, 2021, Oracle and/or its affiliates. All 
> rights reserved.

`2019, ` should be removed.

src/java.base/share/classes/java/time/Clock.java line 115:

> 113:  * Where an application only requires the current instant {@code 
> InstantSource} should
> 114:  * be preferred to this class. For example, his might be the case where 
> the time-zone
> 115:  * is provided by a separate user localization subsystem.

`@see InstantSource` would be preferred here.

src/java.base/share/classes/java/time/InstantSource.java line 32:

> 30: import java.time.Clock.SystemClock;
> 31: import java.time.Clock.SystemInstantSource;
> 32: import java.time.Clock.TickClock;

Some of them are not used.

src/java.base/share/classes/java/time/InstantSource.java line 112:

> 110:  * @return a source that uses the best available system clock, not 
> null
> 111:  */
> 112: public static InstantSource system() {

`public` is redundant here, same for other methods.

-

PR: https://git.openjdk.java.net/jdk/pull/4016

RFR: 8266846: Add java.time.InstantSource

[Stephen Colebourne]

8266846: Add java.time.InstantSource

-

Commit messages:
 - 8266846: Add java.time.InstantSource
 - 8266846: Add java.time.InstantSource

Changes: https://git.openjdk.java.net/jdk/pull/4016/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4016=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8266846
  Stats: 615 lines in 4 files changed: 519 ins; 83 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4016.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016

PR: https://git.openjdk.java.net/jdk/pull/4016

Re: RFR: 8266846: Add java.time.InstantSource

[Anthony Vanelverdinghe]

On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne  
wrote:

> 8266846: Add java.time.InstantSource

A nice addition to the JDK, thanks for taking this on.

src/java.base/share/classes/java/time/Clock.java line 114:

> 112:  * provides access to the current instant, and does not provide access 
> to the time-zone.
> 113:  * Where an application only requires the current instant {@code 
> InstantSource} should
> 114:  * be preferred to this class. For example, his might be the case where 
> the time-zone

his -> this

src/java.base/share/classes/java/time/Clock.java line 513:

> 511:  * {@link System#currentTimeMillis()} or equivalent.
> 512:  */
> 513: static final class SystemInstantSource implements InstantSource, 
> Serializable {

Is it possible to declare this as an enum? As doing so would simplify its 
implementation.

src/java.base/share/classes/java/time/InstantSource.java line 59:

> 57:  *  }
> 58:  * 
> 59:  * This approach allows an alternate source, such as {@link 
> #fixed(Instant) fixed}

alternate -> alternative? To me (being a non-native speaker) the latter reads 
more naturally

src/java.base/share/classes/java/time/InstantSource.java line 62:

> 60:  * or {@link #offset(InstantSource, Duration) offset} to be used during 
> testing.
> 61:  * 
> 62:  * The {@code system} factory method provide a source based on the best 
> available

provide -> provides

src/java.base/share/classes/java/time/InstantSource.java line 63:

> 61:  * 
> 62:  * The {@code system} factory method provide a source based on the best 
> available
> 63:  * system clock This may use {@link System#currentTimeMillis()}, or a 
> higher

missing dot after "clock"

src/java.base/share/classes/java/time/InstantSource.java line 175:

> 173: /**
> 174:  * Obtains a source that returns instants from the specified source 
> with the
> 175:  * specified duration added

missing dot to end the sentence

-

PR: https://git.openjdk.java.net/jdk/pull/4016

Re: JEP draft: Scope Locals

[Andrew Haley]

On 5/15/21 12:28 AM, Nathan Reynolds wrote:
> What about cleanup of the scoped objects?  For example, how do I ensure
> close() or some cleanup method is called at the right time on the scoped
> object?  Do I need to use a Weak/Soft/PhantomReference and a ReferenceQueue
> to track the object?  Do I use Cleaner?  Does the object need to implement
> finalize()... hopefully not?

All a scope local does is bind a value to a name, in a scope. This isn't
about C++-style destructors, which have their merits but are a different
subject.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671