RFR 8066291: ZoneIdPrinterParser can be optimized

Martin Buchholz martinrb at google.com
Fri May 6 18:48:44 UTC 2016 

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