Fine by me, just awaiting an additional reviewer. Stephen
On 9 May 2016 at 12:42, Bhanu Gopularam <bhanu.prakash.gopula...@oracle.com> wrote: > Here is the updated webrev: > > http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.03/ > > Please review the changes. > > Thanks, > Bhanu > > -----Original Message----- > From: Stephen Colebourne [mailto:scolebou...@joda.org] > Sent: Monday, May 09, 2016 4:43 PM > To: core-libs-dev > Subject: Re: RFR 8066291: ZoneIdPrinterParser can be optimized > > One point - the Javadoc for the method on ZoneRulesProvider > > @return a modifiable copy of the set of zone IDs, not null > > needs to change to > > @return the unmodifiable set of zone IDs, not null > > I'd welcome a second review on the volatile / thread-safety aspects, although > I think it is pretty simple and safe. > > Stephen > > > On 9 May 2016 at 11:58, Bhanu Gopularam > <bhanu.prakash.gopula...@oracle.com> wrote: >> 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:scolebou...@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.ri...@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.ri...@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.ri...@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.0 >>>>>>> 0 >>>>>>> >>>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> Bhanu >>>>>>> >>>>>>> >>>
