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