Hi,
It may be useful to include a description of the set as being a 'snapshot'.
Now that registerProvider0 is synchronized, there seems little point to
having ZONES be
a ConcurrentHashMap. An unmodifiable map that is replaced when a new
provider
is added would have the benefits of immutability and not having make a
copy of the zoneids.
Getting the map and adding a new provider would need to be synchronized
but no other operations.
Maybe I'm just overthinking this simple function.
Roger
On 5/9/2016 7:47 AM, Stephen Colebourne wrote:
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
