RFR 8066291: ZoneIdPrinterParser can be optimized

[Bhanu Gopularam]

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 at 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 at 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 at 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 at 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
>>>>>
>>>>>
>