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.00 >>>>>> >>>>>> >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Bhanu >>>>>> >>>>>> >>
