RFR 8066291: ZoneIdPrinterParser can be optimized

[Roger Riggs]

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