Fine by me thanks Stephen On 5 May 2016 at 10:57, Bhanu Gopularam <bhanu.prakash.gopularam@oracle.com> wrote:

Please see the updated webrev: http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.01/

Thanks, Bhanu

-----Original Message----- From: Stephen Colebourne [mailto:scolebourne@joda.org] Sent: Thursday, May 05, 2016 3:18 PM To: core-libs-dev Subject: Re: RFR 8066291: ZoneIdPrinterParser can be optimized

In ZoneRulesProvider.getAvailableZoneIdsSize() there is no need for the trailing paragraph <p> tag in the Javadoc. Otherwise, fine by me. thanks Stephen

On 5 May 2016 at 10:10, Bhanu Gopularam <bhanu.prakash.gopularam@oracle.com> wrote:

...

Hi all,

Please review fix following issue

Bug Id : https://bugs.openjdk.java.net/browse/JDK-8066291

Solution: provided new method to get size of available zone ids

webrev : http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.00

Thanks,

Bhanu

I fail to see why adding a new read-only method alongside the existing method adds any more value to the API than adding a new size method. At least with the size method the API is still sensible - a mutable and immutable method alongside each other shouts out that a mistake was made. A size method is more subtle about the mistake (plenty of APIs have a size method alongside a collection method). Finally, a read-only method involves object creation, thus has worse performance than adding a size method. The only other alternative is perhaps to use SharedSecrets? I don't know what possibilities there are there. If not SharedSecrets, then no matter what, we are adding "a trivial method to the public API used only for an optimization". Stephen On 5 May 2016 at 15:23, Roger Riggs <Roger.Riggs@oracle.com> wrote:

Hi Bhanu,

Adding a trivial method to the public API used only for an optimization is not a good fix for this issue.

A better fix was suggested to add a non-copying read-only version of

ZoneRulesProvider.getAvailableZoneIds()

Please revise the fix to instead implement and use:

/** * Gets a readonly set of available zone IDs. * <p> * These IDs are the string form of a {@link ZoneId}. * * @return a unmodifiable copy of the set of zone IDs, not null */ public static Set<String> getReadOnlyAvailableZoneIds() { return Collections.unmodifiableSet(ZONES.keySet()); }

Roger

On 5/5/2016 5:10 AM, Bhanu Gopularam wrote:

Hi all,

Please review fix following issue

Bug Id : https://bugs.openjdk.java.net/browse/JDK-8066291

Solution: provided new method to get size of available zone ids

webrev : http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.00

Thanks,

Bhanu

On reflection, both your and my solution have a race. the size method, is a clear check-then-act the read-only method uses Collections.unmodifiableSet() which only decorates the underlying set, thus is still check-thern-act (the current implementation does not have a race condition, as the data is copied, thus the size will match the data) There is no pleasant way to solve this that I can see. This is my best attempt: 1) Add a new field private static final CopyOnWriteArrayList<String> ZONE_IDS; 2) At the end of registerProvider0() add all of the new IDs to the list (must be outside the loop as otherwise CopyOnWriteArrayList will be slow) 3) Add a new method getAvailableZoneIdList() that returns the list wrapped in Collections.unmodifiableList() 4) In the calling code, query the size, and then use subList(0, size) to lock the size from race conditions. A variation would be 1) Add a new field private static volatile Set<String> ZONE_IDS; 2) Synchronize/lock registerProvider0() to ensure only one thread is in there at a time. 3) At the end of registerProvider0() add all of the new IDs to the set, storing Collections.unmodifiableSet(combinedSet) 4) Add a new method getAvailableZoneIdSet() that returns the unmodifiable set 5) Change the calling code to use the new method, but no other changes Stephen On 5 May 2016 at 16:53, Roger Riggs <Roger.Riggs@oracle.com> wrote:

Hi Stephen,

The aspect of the current implementation that is problematic is the copying of the set, its not just single object creation but an entry for every ZoneID.

Adding a size method exposing some internal state increases the possibility that when it is used independently it will be out of sync. Not a big issue, but one to avoid when designing an API.

Exposing a mutable Hashset was probably a mistake but one that cannot be corrected now. The optics aren't concerning to me.

SharedSecrets are much messier and not appropriate.

Adding a method to provide what was originally needed is a cleaner solution.

Roger

On 5/5/2016 11:27 AM, Stephen Colebourne wrote:

...

I fail to see why adding a new read-only method alongside the existing method adds any more value to the API than adding a new size method. At least with the size method the API is still sensible - a mutable and immutable method alongside each other shouts out that a mistake was made. A size method is more subtle about the mistake (plenty of APIs have a size method alongside a collection method).

Finally, a read-only method involves object creation, thus has worse performance than adding a size method.

The only other alternative is perhaps to use SharedSecrets? I don't know what possibilities there are there. If not SharedSecrets, then no matter what, we are adding "a trivial method to the public API used only for an optimization".

Stephen

On 5 May 2016 at 15:23, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi Bhanu,

Adding a trivial method to the public API used only for an optimization is not a good fix for this issue.

A better fix was suggested to add a non-copying read-only version of

ZoneRulesProvider.getAvailableZoneIds()

Please revise the fix to instead implement and use:

/** * Gets a readonly set of available zone IDs. * <p> * These IDs are the string form of a {@link ZoneId}. * * @return a unmodifiable copy of the set of zone IDs, not null */ public static Set<String> getReadOnlyAvailableZoneIds() { return Collections.unmodifiableSet(ZONES.keySet()); }

Roger

On 5/5/2016 5:10 AM, Bhanu Gopularam wrote:

Hi all,

Please review fix following issue

Bug Id : https://bugs.openjdk.java.net/browse/JDK-8066291

Solution: provided new method to get size of available zone ids

webrev : http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.00

Thanks,

Bhanu

The set of zones can only increase, it cannot decrease as there is no removal mechanism. As such, the size of the set is a proxy for the number you describe. One other point. The method that most users will call to get the set of ZoneIds is ZoneId.getAvailableZoneIds(). That method delegates to the one on ZoneRulesProvider. As such, we can change the method on ZoneRulesProvider to return an immutable set while still keeping the method commonly used by users returning a mutable set. The incompatibility impact caused by this would be vanishingly small. To me, this is by far the best way to address this problem, as it avoids a new method. Thus, I propose: 1) Add a new field private static volatile Set<String> ZONE_IDS; 2) Synchronize/lock registerProvider0() to ensure only one thread is in there at a time. 3) At the end of registerProvider0() add all of the existing and new IDs to a new HashSet wrapped in Collections.unmodifiableSet(combinedSet) and change ZONE_IDS to point at the new set. 4) Change ZoneRulesProvider.getAvailableZoneIds() to return ZONE_IDS. Change the spec to indicate the result is unmodifiable. 5) Change ZoneId.getAvailableZoneIds() to return new HashSet(ZoneRulesProvider.getAvailableZoneIds()) Code changes: ZoneId: public static Set<String> getAvailableZoneIds() { return new HashSet(ZoneRulesProvider.getAvailableZoneIds()); } ZoneRulesProvider: private static volatile Set<String> ZONE_IDS; @return the unmodifiable set of zone IDs, not null public static Set<String> getAvailableZoneIds() { return ZONE_IDS; } private static synchronized void registerProvider0(ZoneRulesProvider provider) { for (String zoneId : provider.provideZoneIds()) { Objects.requireNonNull(zoneId, "zoneId"); ZoneRulesProvider old = ZONES.putIfAbsent(zoneId, provider); if (old != null) { throw new ZoneRulesException( "Unable to register zone as one already registered with that ID: " + zoneId + ", currently loading from provider: " + provider); } } Set<String> combinedSet = new HashSet(ZONES.keySet()); ZONE_IDS = Collections.unmodifiableSet(combinedSet); } Stephen On 5 May 2016 at 19:48, Roger Riggs <Roger.Riggs@oracle.com> wrote:

Hi,

Using the current number of ZoneIDs to avoid the recompilation of the cache is a bit weak anyway, though it seems unlikely that a ZoneID would be added and one deleted without being noticed.

An alternative would be a API that returned a number that would change every time the set of ZoneIds changed. That would be more suitable both as a new API and as something to trigger updates to the cache.

I'd rather see a localized implementation with a simple model.

Roger

On 5/5/2016 1:43 PM, Stephen Colebourne wrote:

...

On reflection, both your and my solution have a race.

the size method, is a clear check-then-act

the read-only method uses Collections.unmodifiableSet() which only decorates the underlying set, thus is still check-thern-act

(the current implementation does not have a race condition, as the data is copied, thus the size will match the data)

There is no pleasant way to solve this that I can see. This is my best attempt:

1) Add a new field

private static final CopyOnWriteArrayList<String> ZONE_IDS;

2) At the end of registerProvider0() add all of the new IDs to the list (must be outside the loop as otherwise CopyOnWriteArrayList will be slow)

3) Add a new method getAvailableZoneIdList() that returns the list wrapped in Collections.unmodifiableList()

4) In the calling code, query the size, and then use subList(0, size) to lock the size from race conditions.

A variation would be

1) Add a new field

private static volatile Set<String> ZONE_IDS;

2) Synchronize/lock registerProvider0() to ensure only one thread is in there at a time.

3) At the end of registerProvider0() add all of the new IDs to the set, storing Collections.unmodifiableSet(combinedSet)

4) Add a new method getAvailableZoneIdSet() that returns the unmodifiable set

5) Change the calling code to use the new method, but no other changes

Stephen

On 5 May 2016 at 16:53, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi Stephen,

The aspect of the current implementation that is problematic is the copying of the set, its not just single object creation but an entry for every ZoneID.

Adding a size method exposing some internal state increases the possibility that when it is used independently it will be out of sync. Not a big issue, but one to avoid when designing an API.

Exposing a mutable Hashset was probably a mistake but one that cannot be corrected now. The optics aren't concerning to me.

SharedSecrets are much messier and not appropriate.

Adding a method to provide what was originally needed is a cleaner solution.

Roger

On 5/5/2016 11:27 AM, Stephen Colebourne wrote:

...

I fail to see why adding a new read-only method alongside the existing method adds any more value to the API than adding a new size method. At least with the size method the API is still sensible - a mutable and immutable method alongside each other shouts out that a mistake was made. A size method is more subtle about the mistake (plenty of APIs have a size method alongside a collection method).

Finally, a read-only method involves object creation, thus has worse performance than adding a size method.

The only other alternative is perhaps to use SharedSecrets? I don't know what possibilities there are there. If not SharedSecrets, then no matter what, we are adding "a trivial method to the public API used only for an optimization".

Stephen

On 5 May 2016 at 15:23, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi Bhanu,

Adding a trivial method to the public API used only for an optimization is not a good fix for this issue.

A better fix was suggested to add a non-copying read-only version of

ZoneRulesProvider.getAvailableZoneIds()

Please revise the fix to instead implement and use:

/** * Gets a readonly set of available zone IDs. * <p> * These IDs are the string form of a {@link ZoneId}. * * @return a unmodifiable copy of the set of zone IDs, not null */ public static Set<String> getReadOnlyAvailableZoneIds() { return Collections.unmodifiableSet(ZONES.keySet()); }

Roger

On 5/5/2016 5:10 AM, Bhanu Gopularam wrote:

Hi all,

Please review fix following issue

Bug Id : https://bugs.openjdk.java.net/browse/JDK-8066291

Solution: provided new method to get size of available zone ids

webrev : http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.00

Thanks,

Bhanu

ConcurrentHashMap is (probably!) cheaper than it used to be. For read-mostly operations there won't be a lock. But yes, even better for read-mostly use is to keep an immutable map and CAS-loop whenever you need to update. The VarHandle work and the immutable collections work should make that better in the future. Perhaps adding something in j.u.c. is worthwhile, but until then it's not too hard to write your own CAS-loop to update the map (but wait for VarHandles to be fully baked). On Fri, May 6, 2016 at 10:15 AM, Roger Riggs <Roger.Riggs@oracle.com> wrote:

Hi Stephen,

It still seems pretty elaborate and duplicates the set; but it would be a bigger change to restructure the internal ZONES map to have an immutable map in the implementation and synchronize its update when a new Provider is added (very infrequently/never). ConcurrentHashMap is a pretty expensive datatype when concurrency is low/not existent.

So, your proposal is a reasonable course of action.

If there are any objections to the behavior change, we can fall back to a new method.

Roger

On 5/6/2016 6:23 AM, Stephen Colebourne wrote:

...

The set of zones can only increase, it cannot decrease as there is no removal mechanism. As such, the size of the set is a proxy for the number you describe.

One other point. The method that most users will call to get the set of ZoneIds is ZoneId.getAvailableZoneIds(). That method delegates to the one on ZoneRulesProvider. As such, we can change the method on ZoneRulesProvider to return an immutable set while still keeping the method commonly used by users returning a mutable set. The incompatibility impact caused by this would be vanishingly small. To me, this is by far the best way to address this problem, as it avoids a new method.

Thus, I propose:

1) Add a new field private static volatile Set<String> ZONE_IDS;

2) Synchronize/lock registerProvider0() to ensure only one thread is in there at a time.

3) At the end of registerProvider0() add all of the existing and new IDs to a new HashSet wrapped in Collections.unmodifiableSet(combinedSet) and change ZONE_IDS to point at the new set.

4) Change ZoneRulesProvider.getAvailableZoneIds() to return ZONE_IDS. Change the spec to indicate the result is unmodifiable.

5) Change ZoneId.getAvailableZoneIds() to return new HashSet(ZoneRulesProvider.getAvailableZoneIds())

Code changes:

ZoneId:

public static Set<String> getAvailableZoneIds() { return new HashSet(ZoneRulesProvider.getAvailableZoneIds()); }

ZoneRulesProvider:

private static volatile Set<String> ZONE_IDS;

@return the unmodifiable set of zone IDs, not null public static Set<String> getAvailableZoneIds() { return ZONE_IDS; }

private static synchronized void registerProvider0(ZoneRulesProvider provider) { for (String zoneId : provider.provideZoneIds()) { Objects.requireNonNull(zoneId, "zoneId"); ZoneRulesProvider old = ZONES.putIfAbsent(zoneId, provider); if (old != null) { throw new ZoneRulesException( "Unable to register zone as one already registered with that ID: " + zoneId + ", currently loading from provider: " + provider); } } Set<String> combinedSet = new HashSet(ZONES.keySet()); ZONE_IDS = Collections.unmodifiableSet(combinedSet); }

Stephen

On 5 May 2016 at 19:48, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi,

Using the current number of ZoneIDs to avoid the recompilation of the cache is a bit weak anyway, though it seems unlikely that a ZoneID would be added and one deleted without being noticed.

An alternative would be a API that returned a number that would change every time the set of ZoneIds changed. That would be more suitable both as a new API and as something to trigger updates to the cache.

I'd rather see a localized implementation with a simple model.

Roger

On 5/5/2016 1:43 PM, Stephen Colebourne wrote:

...

On reflection, both your and my solution have a race.

the size method, is a clear check-then-act

the read-only method uses Collections.unmodifiableSet() which only decorates the underlying set, thus is still check-thern-act

(the current implementation does not have a race condition, as the data is copied, thus the size will match the data)

There is no pleasant way to solve this that I can see. This is my best attempt:

1) Add a new field

private static final CopyOnWriteArrayList<String> ZONE_IDS;

2) At the end of registerProvider0() add all of the new IDs to the list (must be outside the loop as otherwise CopyOnWriteArrayList will be slow)

3) Add a new method getAvailableZoneIdList() that returns the list wrapped in Collections.unmodifiableList()

4) In the calling code, query the size, and then use subList(0, size) to lock the size from race conditions.

A variation would be

1) Add a new field

private static volatile Set<String> ZONE_IDS;

2) Synchronize/lock registerProvider0() to ensure only one thread is in there at a time.

3) At the end of registerProvider0() add all of the new IDs to the set, storing Collections.unmodifiableSet(combinedSet)

4) Add a new method getAvailableZoneIdSet() that returns the unmodifiable set

5) Change the calling code to use the new method, but no other changes

Stephen

On 5 May 2016 at 16:53, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi Stephen,

The aspect of the current implementation that is problematic is the copying of the set, its not just single object creation but an entry for every ZoneID.

Adding a size method exposing some internal state increases the possibility that when it is used independently it will be out of sync. Not a big issue, but one to avoid when designing an API.

Exposing a mutable Hashset was probably a mistake but one that cannot be corrected now. The optics aren't concerning to me.

SharedSecrets are much messier and not appropriate.

Adding a method to provide what was originally needed is a cleaner solution.

Roger

On 5/5/2016 11:27 AM, Stephen Colebourne wrote:

...

I fail to see why adding a new read-only method alongside the existing method adds any more value to the API than adding a new size method. At least with the size method the API is still sensible - a mutable and immutable method alongside each other shouts out that a mistake was made. A size method is more subtle about the mistake (plenty of APIs have a size method alongside a collection method).

Finally, a read-only method involves object creation, thus has worse performance than adding a size method.

The only other alternative is perhaps to use SharedSecrets? I don't know what possibilities there are there. If not SharedSecrets, then no matter what, we are adding "a trivial method to the public API used only for an optimization".

Stephen

On 5 May 2016 at 15:23, Roger Riggs <Roger.Riggs@oracle.com> wrote: > > Hi Bhanu, > > Adding a trivial method to the public API used only for an > optimization > is > not a good fix for this issue. > > A better fix was suggested to add a non-copying read-only version of > > ZoneRulesProvider.getAvailableZoneIds() > > Please revise the fix to instead implement and use: > > /** > * Gets a readonly set of available zone IDs. > * <p> > * These IDs are the string form of a {@link ZoneId}. > * > * @return a unmodifiable copy of the set of zone IDs, not > null > */ > public static Set<String> getReadOnlyAvailableZoneIds() { > return Collections.unmodifiableSet(ZONES.keySet()); > } > > Roger > > > > On 5/5/2016 5:10 AM, Bhanu Gopularam wrote: > > Hi all, > > > > Please review fix following issue > > > > Bug Id : https://bugs.openjdk.java.net/browse/JDK-8066291 > > > > Solution: provided new method to get size of available zone ids > > > > webrev : > http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.00 > > > > Thanks, > > Bhanu > >

One point - the Javadoc for the method on ZoneRulesProvider @return a modifiable copy of the set of zone IDs, not null needs to change to @return the unmodifiable set of zone IDs, not null I'd welcome a second review on the volatile / thread-safety aspects, although I think it is pretty simple and safe. Stephen On 9 May 2016 at 11:58, Bhanu Gopularam <bhanu.prakash.gopularam@oracle.com> wrote:

Hi all,

Here is the updated webrev:

http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.02/

I have included newly suggested changes. On test side, I had to cleanup test java/time/zone/TCKZoneRulesProvider.java (test_getAvailableGroupIds) as ZoneRulesProvider.getAvailableZoneIds() returns a non-modifiable list, the tests for ZoneId.getAvailableZoneIds() is present in java/time/tck/java/time/TCKZoneId.java ().

Thanks, Bhanu

-----Original Message----- From: Stephen Colebourne [mailto:scolebourne@joda.org] Sent: Friday, May 06, 2016 3:54 PM To: core-libs-dev Subject: Re: RFR 8066291: ZoneIdPrinterParser can be optimized

The set of zones can only increase, it cannot decrease as there is no removal mechanism. As such, the size of the set is a proxy for the number you describe.

One other point. The method that most users will call to get the set of ZoneIds is ZoneId.getAvailableZoneIds(). That method delegates to the one on ZoneRulesProvider. As such, we can change the method on ZoneRulesProvider to return an immutable set while still keeping the method commonly used by users returning a mutable set. The incompatibility impact caused by this would be vanishingly small. To me, this is by far the best way to address this problem, as it avoids a new method.

Thus, I propose:

1) Add a new field private static volatile Set<String> ZONE_IDS;

2) Synchronize/lock registerProvider0() to ensure only one thread is in there at a time.

3) At the end of registerProvider0() add all of the existing and new IDs to a new HashSet wrapped in Collections.unmodifiableSet(combinedSet) and change ZONE_IDS to point at the new set.

4) Change ZoneRulesProvider.getAvailableZoneIds() to return ZONE_IDS. Change the spec to indicate the result is unmodifiable.

5) Change ZoneId.getAvailableZoneIds() to return new HashSet(ZoneRulesProvider.getAvailableZoneIds())

Code changes:

ZoneId:

public static Set<String> getAvailableZoneIds() { return new HashSet(ZoneRulesProvider.getAvailableZoneIds()); }

ZoneRulesProvider:

private static volatile Set<String> ZONE_IDS;

@return the unmodifiable set of zone IDs, not null public static Set<String> getAvailableZoneIds() { return ZONE_IDS; }

private static synchronized void registerProvider0(ZoneRulesProvider provider) { for (String zoneId : provider.provideZoneIds()) { Objects.requireNonNull(zoneId, "zoneId"); ZoneRulesProvider old = ZONES.putIfAbsent(zoneId, provider); if (old != null) { throw new ZoneRulesException( "Unable to register zone as one already registered with that ID: " + zoneId + ", currently loading from provider: " + provider); } } Set<String> combinedSet = new HashSet(ZONES.keySet()); ZONE_IDS = Collections.unmodifiableSet(combinedSet); }

Stephen

On 5 May 2016 at 19:48, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi,

Using the current number of ZoneIDs to avoid the recompilation of the cache is a bit weak anyway, though it seems unlikely that a ZoneID would be added and one deleted without being noticed.

An alternative would be a API that returned a number that would change every time the set of ZoneIds changed. That would be more suitable both as a new API and as something to trigger updates to the cache.

I'd rather see a localized implementation with a simple model.

Roger

On 5/5/2016 1:43 PM, Stephen Colebourne wrote:

...

On reflection, both your and my solution have a race.

the size method, is a clear check-then-act

the read-only method uses Collections.unmodifiableSet() which only decorates the underlying set, thus is still check-thern-act

(the current implementation does not have a race condition, as the data is copied, thus the size will match the data)

There is no pleasant way to solve this that I can see. This is my best attempt:

1) Add a new field

private static final CopyOnWriteArrayList<String> ZONE_IDS;

2) At the end of registerProvider0() add all of the new IDs to the list (must be outside the loop as otherwise CopyOnWriteArrayList will be slow)

3) Add a new method getAvailableZoneIdList() that returns the list wrapped in Collections.unmodifiableList()

4) In the calling code, query the size, and then use subList(0, size) to lock the size from race conditions.

A variation would be

1) Add a new field

private static volatile Set<String> ZONE_IDS;

2) Synchronize/lock registerProvider0() to ensure only one thread is in there at a time.

3) At the end of registerProvider0() add all of the new IDs to the set, storing Collections.unmodifiableSet(combinedSet)

4) Add a new method getAvailableZoneIdSet() that returns the unmodifiable set

5) Change the calling code to use the new method, but no other changes

Stephen

On 5 May 2016 at 16:53, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi Stephen,

The aspect of the current implementation that is problematic is the copying of the set, its not just single object creation but an entry for every ZoneID.

Adding a size method exposing some internal state increases the possibility that when it is used independently it will be out of sync. Not a big issue, but one to avoid when designing an API.

Exposing a mutable Hashset was probably a mistake but one that cannot be corrected now. The optics aren't concerning to me.

SharedSecrets are much messier and not appropriate.

Adding a method to provide what was originally needed is a cleaner solution.

Roger

On 5/5/2016 11:27 AM, Stephen Colebourne wrote:

...

I fail to see why adding a new read-only method alongside the existing method adds any more value to the API than adding a new size method. At least with the size method the API is still sensible - a mutable and immutable method alongside each other shouts out that a mistake was made. A size method is more subtle about the mistake (plenty of APIs have a size method alongside a collection method).

Finally, a read-only method involves object creation, thus has worse performance than adding a size method.

The only other alternative is perhaps to use SharedSecrets? I don't know what possibilities there are there. If not SharedSecrets, then no matter what, we are adding "a trivial method to the public API used only for an optimization".

Stephen

On 5 May 2016 at 15:23, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi Bhanu,

Adding a trivial method to the public API used only for an optimization is not a good fix for this issue.

A better fix was suggested to add a non-copying read-only version of

ZoneRulesProvider.getAvailableZoneIds()

Please revise the fix to instead implement and use:

/** * Gets a readonly set of available zone IDs. * <p> * These IDs are the string form of a {@link ZoneId}. * * @return a unmodifiable copy of the set of zone IDs, not null */ public static Set<String> getReadOnlyAvailableZoneIds() { return Collections.unmodifiableSet(ZONES.keySet()); }

Roger

On 5/5/2016 5:10 AM, Bhanu Gopularam wrote:

Hi all,

Please review fix following issue

Bug Id : https://bugs.openjdk.java.net/browse/JDK-8066291

Solution: provided new method to get size of available zone ids

webrev : http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.00

Thanks,

Bhanu

Fine by me, just awaiting an additional reviewer. Stephen On 9 May 2016 at 12:42, Bhanu Gopularam <bhanu.prakash.gopularam@oracle.com> wrote:

Here is the updated webrev:

http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.03/

Please review the changes.

Thanks, Bhanu

-----Original Message----- From: Stephen Colebourne [mailto:scolebourne@joda.org] Sent: Monday, May 09, 2016 4:43 PM To: core-libs-dev Subject: Re: RFR 8066291: ZoneIdPrinterParser can be optimized

One point - the Javadoc for the method on ZoneRulesProvider

@return a modifiable copy of the set of zone IDs, not null

needs to change to

@return the unmodifiable set of zone IDs, not null

I'd welcome a second review on the volatile / thread-safety aspects, although I think it is pretty simple and safe.

Stephen

On 9 May 2016 at 11:58, Bhanu Gopularam <bhanu.prakash.gopularam@oracle.com> wrote:

...

Hi all,

Here is the updated webrev:

http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.02/

I have included newly suggested changes. On test side, I had to cleanup test java/time/zone/TCKZoneRulesProvider.java (test_getAvailableGroupIds) as ZoneRulesProvider.getAvailableZoneIds() returns a non-modifiable list, the tests for ZoneId.getAvailableZoneIds() is present in java/time/tck/java/time/TCKZoneId.java ().

Thanks, Bhanu

-----Original Message----- From: Stephen Colebourne [mailto:scolebourne@joda.org] Sent: Friday, May 06, 2016 3:54 PM To: core-libs-dev Subject: Re: RFR 8066291: ZoneIdPrinterParser can be optimized

The set of zones can only increase, it cannot decrease as there is no removal mechanism. As such, the size of the set is a proxy for the number you describe.

One other point. The method that most users will call to get the set of ZoneIds is ZoneId.getAvailableZoneIds(). That method delegates to the one on ZoneRulesProvider. As such, we can change the method on ZoneRulesProvider to return an immutable set while still keeping the method commonly used by users returning a mutable set. The incompatibility impact caused by this would be vanishingly small. To me, this is by far the best way to address this problem, as it avoids a new method.

Thus, I propose:

1) Add a new field private static volatile Set<String> ZONE_IDS;

2) Synchronize/lock registerProvider0() to ensure only one thread is in there at a time.

3) At the end of registerProvider0() add all of the existing and new IDs to a new HashSet wrapped in Collections.unmodifiableSet(combinedSet) and change ZONE_IDS to point at the new set.

4) Change ZoneRulesProvider.getAvailableZoneIds() to return ZONE_IDS. Change the spec to indicate the result is unmodifiable.

5) Change ZoneId.getAvailableZoneIds() to return new HashSet(ZoneRulesProvider.getAvailableZoneIds())

Code changes:

ZoneId:

public static Set<String> getAvailableZoneIds() { return new HashSet(ZoneRulesProvider.getAvailableZoneIds()); }

ZoneRulesProvider:

private static volatile Set<String> ZONE_IDS;

@return the unmodifiable set of zone IDs, not null public static Set<String> getAvailableZoneIds() { return ZONE_IDS; }

private static synchronized void registerProvider0(ZoneRulesProvider provider) { for (String zoneId : provider.provideZoneIds()) { Objects.requireNonNull(zoneId, "zoneId"); ZoneRulesProvider old = ZONES.putIfAbsent(zoneId, provider); if (old != null) { throw new ZoneRulesException( "Unable to register zone as one already registered with that ID: " + zoneId + ", currently loading from provider: " + provider); } } Set<String> combinedSet = new HashSet(ZONES.keySet()); ZONE_IDS = Collections.unmodifiableSet(combinedSet); }

Stephen

On 5 May 2016 at 19:48, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi,

Using the current number of ZoneIDs to avoid the recompilation of the cache is a bit weak anyway, though it seems unlikely that a ZoneID would be added and one deleted without being noticed.

An alternative would be a API that returned a number that would change every time the set of ZoneIds changed. That would be more suitable both as a new API and as something to trigger updates to the cache.

I'd rather see a localized implementation with a simple model.

Roger

On 5/5/2016 1:43 PM, Stephen Colebourne wrote:

...

On reflection, both your and my solution have a race.

the size method, is a clear check-then-act

the read-only method uses Collections.unmodifiableSet() which only decorates the underlying set, thus is still check-thern-act

(the current implementation does not have a race condition, as the data is copied, thus the size will match the data)

There is no pleasant way to solve this that I can see. This is my best attempt:

1) Add a new field

private static final CopyOnWriteArrayList<String> ZONE_IDS;

2) At the end of registerProvider0() add all of the new IDs to the list (must be outside the loop as otherwise CopyOnWriteArrayList will be slow)

3) Add a new method getAvailableZoneIdList() that returns the list wrapped in Collections.unmodifiableList()

4) In the calling code, query the size, and then use subList(0, size) to lock the size from race conditions.

A variation would be

1) Add a new field

private static volatile Set<String> ZONE_IDS;

2) Synchronize/lock registerProvider0() to ensure only one thread is in there at a time.

3) At the end of registerProvider0() add all of the new IDs to the set, storing Collections.unmodifiableSet(combinedSet)

4) Add a new method getAvailableZoneIdSet() that returns the unmodifiable set

5) Change the calling code to use the new method, but no other changes

Stephen

On 5 May 2016 at 16:53, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi Stephen,

The aspect of the current implementation that is problematic is the copying of the set, its not just single object creation but an entry for every ZoneID.

Adding a size method exposing some internal state increases the possibility that when it is used independently it will be out of sync. Not a big issue, but one to avoid when designing an API.

Exposing a mutable Hashset was probably a mistake but one that cannot be corrected now. The optics aren't concerning to me.

SharedSecrets are much messier and not appropriate.

Adding a method to provide what was originally needed is a cleaner solution.

Roger

On 5/5/2016 11:27 AM, Stephen Colebourne wrote:

...

I fail to see why adding a new read-only method alongside the existing method adds any more value to the API than adding a new size method. At least with the size method the API is still sensible - a mutable and immutable method alongside each other shouts out that a mistake was made. A size method is more subtle about the mistake (plenty of APIs have a size method alongside a collection method).

Finally, a read-only method involves object creation, thus has worse performance than adding a size method.

The only other alternative is perhaps to use SharedSecrets? I don't know what possibilities there are there. If not SharedSecrets, then no matter what, we are adding "a trivial method to the public API used only for an optimization".

Stephen

On 5 May 2016 at 15:23, Roger Riggs <Roger.Riggs@oracle.com> wrote: > > Hi Bhanu, > > Adding a trivial method to the public API used only for an > optimization is not a good fix for this issue. > > A better fix was suggested to add a non-copying read-only version > of > > ZoneRulesProvider.getAvailableZoneIds() > > Please revise the fix to instead implement and use: > > /** > * Gets a readonly set of available zone IDs. > * <p> > * These IDs are the string form of a {@link ZoneId}. > * > * @return a unmodifiable copy of the set of zone IDs, not null > */ > public static Set<String> getReadOnlyAvailableZoneIds() { > return Collections.unmodifiableSet(ZONES.keySet()); > } > > Roger > > > > On 5/5/2016 5:10 AM, Bhanu Gopularam wrote: > > Hi all, > > > > Please review fix following issue > > > > Bug Id : https://bugs.openjdk.java.net/browse/JDK-8066291 > > > > Solution: provided new method to get size of available zone ids > > > > webrev : > http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.0 > 0 > > > > Thanks, > > Bhanu > >

Hi, The behavioral change to getAvailableZoneIds will get the greatest attention by the folks concerned with compatibility. Be prepared to make the case that the change will not affect existing applications. Having to modify an existing compatibility test is a warning that it is likely to be an issue. Especially when compared to the alternative to introduce a new method with the desired behavior. Roger On 5/9/2016 6:58 AM, Bhanu Gopularam wrote:

Hi all,

Here is the updated webrev:

http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.02/

I have included newly suggested changes. On test side, I had to cleanup test java/time/zone/TCKZoneRulesProvider.java (test_getAvailableGroupIds) as ZoneRulesProvider.getAvailableZoneIds() returns a non-modifiable list, the tests for ZoneId.getAvailableZoneIds() is present in java/time/tck/java/time/TCKZoneId.java ().

Thanks, Bhanu

-----Original Message----- From: Stephen Colebourne [mailto:scolebourne@joda.org] Sent: Friday, May 06, 2016 3:54 PM To: core-libs-dev Subject: Re: RFR 8066291: ZoneIdPrinterParser can be optimized

The set of zones can only increase, it cannot decrease as there is no removal mechanism. As such, the size of the set is a proxy for the number you describe.

One other point. The method that most users will call to get the set of ZoneIds is ZoneId.getAvailableZoneIds(). That method delegates to the one on ZoneRulesProvider. As such, we can change the method on ZoneRulesProvider to return an immutable set while still keeping the method commonly used by users returning a mutable set. The incompatibility impact caused by this would be vanishingly small. To me, this is by far the best way to address this problem, as it avoids a new method.

Thus, I propose:

1) Add a new field private static volatile Set<String> ZONE_IDS;

2) Synchronize/lock registerProvider0() to ensure only one thread is in there at a time.

3) At the end of registerProvider0() add all of the existing and new IDs to a new HashSet wrapped in Collections.unmodifiableSet(combinedSet) and change ZONE_IDS to point at the new set.

4) Change ZoneRulesProvider.getAvailableZoneIds() to return ZONE_IDS. Change the spec to indicate the result is unmodifiable.

5) Change ZoneId.getAvailableZoneIds() to return new HashSet(ZoneRulesProvider.getAvailableZoneIds())

Code changes:

ZoneId:

public static Set<String> getAvailableZoneIds() { return new HashSet(ZoneRulesProvider.getAvailableZoneIds()); }

ZoneRulesProvider:

private static volatile Set<String> ZONE_IDS;

@return the unmodifiable set of zone IDs, not null public static Set<String> getAvailableZoneIds() { return ZONE_IDS; }

private static synchronized void registerProvider0(ZoneRulesProvider provider) { for (String zoneId : provider.provideZoneIds()) { Objects.requireNonNull(zoneId, "zoneId"); ZoneRulesProvider old = ZONES.putIfAbsent(zoneId, provider); if (old != null) { throw new ZoneRulesException( "Unable to register zone as one already registered with that ID: " + zoneId + ", currently loading from provider: " + provider); } } Set<String> combinedSet = new HashSet(ZONES.keySet()); ZONE_IDS = Collections.unmodifiableSet(combinedSet); }

Stephen

On 5 May 2016 at 19:48, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi,

Using the current number of ZoneIDs to avoid the recompilation of the cache is a bit weak anyway, though it seems unlikely that a ZoneID would be added and one deleted without being noticed.

An alternative would be a API that returned a number that would change every time the set of ZoneIds changed. That would be more suitable both as a new API and as something to trigger updates to the cache.

I'd rather see a localized implementation with a simple model.

Roger

On 5/5/2016 1:43 PM, Stephen Colebourne wrote:

...

On reflection, both your and my solution have a race.

the size method, is a clear check-then-act

the read-only method uses Collections.unmodifiableSet() which only decorates the underlying set, thus is still check-thern-act

(the current implementation does not have a race condition, as the data is copied, thus the size will match the data)

There is no pleasant way to solve this that I can see. This is my best attempt:

1) Add a new field

private static final CopyOnWriteArrayList<String> ZONE_IDS;

2) At the end of registerProvider0() add all of the new IDs to the list (must be outside the loop as otherwise CopyOnWriteArrayList will be slow)

3) Add a new method getAvailableZoneIdList() that returns the list wrapped in Collections.unmodifiableList()

4) In the calling code, query the size, and then use subList(0, size) to lock the size from race conditions.

A variation would be

1) Add a new field

private static volatile Set<String> ZONE_IDS;

2) Synchronize/lock registerProvider0() to ensure only one thread is in there at a time.

3) At the end of registerProvider0() add all of the new IDs to the set, storing Collections.unmodifiableSet(combinedSet)

4) Add a new method getAvailableZoneIdSet() that returns the unmodifiable set

5) Change the calling code to use the new method, but no other changes

Stephen

On 5 May 2016 at 16:53, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi Stephen,

The aspect of the current implementation that is problematic is the copying of the set, its not just single object creation but an entry for every ZoneID.

Adding a size method exposing some internal state increases the possibility that when it is used independently it will be out of sync. Not a big issue, but one to avoid when designing an API.

Exposing a mutable Hashset was probably a mistake but one that cannot be corrected now. The optics aren't concerning to me.

SharedSecrets are much messier and not appropriate.

Adding a method to provide what was originally needed is a cleaner solution.

Roger

On 5/5/2016 11:27 AM, Stephen Colebourne wrote:

...

I fail to see why adding a new read-only method alongside the existing method adds any more value to the API than adding a new size method. At least with the size method the API is still sensible - a mutable and immutable method alongside each other shouts out that a mistake was made. A size method is more subtle about the mistake (plenty of APIs have a size method alongside a collection method).

Finally, a read-only method involves object creation, thus has worse performance than adding a size method.

The only other alternative is perhaps to use SharedSecrets? I don't know what possibilities there are there. If not SharedSecrets, then no matter what, we are adding "a trivial method to the public API used only for an optimization".

Stephen

On 5 May 2016 at 15:23, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi Bhanu,

Adding a trivial method to the public API used only for an optimization is not a good fix for this issue.

A better fix was suggested to add a non-copying read-only version of

ZoneRulesProvider.getAvailableZoneIds()

Please revise the fix to instead implement and use:

/** * Gets a readonly set of available zone IDs. * <p> * These IDs are the string form of a {@link ZoneId}. * * @return a unmodifiable copy of the set of zone IDs, not null */ public static Set<String> getReadOnlyAvailableZoneIds() { return Collections.unmodifiableSet(ZONES.keySet()); }

Roger

On 5/5/2016 5:10 AM, Bhanu Gopularam wrote:

Hi all,

Please review fix following issue

Bug Id : https://bugs.openjdk.java.net/browse/JDK-8066291

Solution: provided new method to get size of available zone ids

webrev : http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.00

Thanks,

Bhanu