RFR 8066291: ZoneIdPrinterParser can be optimized
Roger Riggs
Roger.Riggs at Oracle.com
Fri May 6 17:15:16 UTC 2016
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 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
>>>>>>
>>>>>>
More information about the core-libs-dev
mailing list