RFR 8066291: ZoneIdPrinterParser can be optimized

[Stephen Colebourne]

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