RFR 8190505, typo in test/jdk/ProblemList.txt

[Felix Yang]

Please review a minor patch to correct typo in test/jdk/ProblemList.txt. 
Change "Hashmap" to "HashMap".


diff -r 4a35a00eb001 test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt    Wed Nov 01 16:45:28 2017 +0100
+++ b/test/jdk/ProblemList.txt    Wed Nov 01 23:34:44 2017 -0400
@@ -320,7 +320,7 @@

 org/omg/CORBA/OrbPropertiesTest.java                8175177 
generic-all


-javax/rmi/PortableRemoteObject/ConcurrentHashmapTest.java 8080643 
generic-all
+javax/rmi/PortableRemoteObject/ConcurrentHashMapTest.java 8080643 
generic-all


 javax/rmi/ssl/SSLSocketParametersTest.sh 8162906 generic-all


Issue: https://bugs.openjdk.java.net/browse/JDK-8190505

Thanks,

Felix

Re: [Bug][JDK10] Fix duplicated "a" occurrences in docs

[Ivan Gerasimov]

If it's not too late, I'd lake to take part in this feast :)

Here's a handful of some more word duplicates:

http://cr.openjdk.java.net/~igerasim/double-trouble-2/00/webrev/

With kind regards,

Ivan


On 10/30/17 6:16 PM, Stuart Marks wrote:
No, Roger checked in the "the the" patch before Christophe posted a "a 
a" followup patch. :-)


s'marks

On 10/30/17 6:00 PM, David Holmes wrote:
Does this duplicate the the patch Christoph sent regarding the the? 
;-) Roger was handling that one.


David

On 31/10/2017 10:29 AM, Stuart Marks wrote:

Hi all,

Please review the patch below. I'm sending this for review because 
it's expanded considerably beyond the original patch that Christoph 
had submitted. I ran a full-text search over the java.base source 
files looking for "the the" and "a a" and it found a bunch of 
additional occurrences that were split across line breaks. I also 
fixed up a few other places that I happened to notice.


Christoph wrote:
I wasn't sure about the capitalization change on EventObject. 
What's the
rule of thumb in cases where the class does not follow the 
guidelines for

docs[1]? Sticking to the overall class scheme or aligning everything?


Good question. The first rule of styling is to stick with the 
prevailing style in the file. In this case, changing just the text 
of that one @return tag would have conflicted with the style in use 
in the rest of the file, even though that style is arguably 
incorrect; the rest of the file uses an initial capital and ends the 
description with a period (full stop), whereas the recommended style 
is to use a noun phrase (a sentence fragment), without an initial 
capital, and without a trailing period (since there is no sentence 
following).


However, the file is small, and there were only three or four other 
places that needed to be changed in order to bring the file into 
compliance with the preferred style, so I just fixed them all.



Thanks,

s'marks



# HG changeset patch
# User smarks
# Date 1509409127 25200
#  Mon Oct 30 17:18:47 2017 -0700
# Node ID d2823a0c8dfab9e95e5c482794a96864b2592aad
# Parent  d87f89c74f54873f273c4fbd8e6d49d7cbf3930b
8190382: fix small typographic errors in comments
Reviewed-by: XXX
Contributed-by: christoph.dr...@freenet.de

diff -r d87f89c74f54 -r d2823a0c8dfa 
src/java.base/share/classes/java/io/FilePermission.java
--- a/src/java.base/share/classes/java/io/FilePermission.java Mon 
Oct 30 07:06:49 2017 -0700
+++ b/src/java.base/share/classes/java/io/FilePermission.java Mon 
Oct 30 17:18:47 2017 -0700

@@ -698,7 +698,7 @@
  if (p2.equals(EMPTY_PATH)) {
  return 0;
  } else if (p2.getName(0).equals(DOTDOT_PATH)) {
-// "." contains p2 iif p2 has no "..". Since a
+// "." contains p2 iff p2 has no "..". Since
  // a normalized path can only have 0 or more
  // ".." at the beginning. We only need to look
  // at the head.
@@ -711,7 +711,7 @@
  } else if (p2.equals(EMPTY_PATH)) {
  int c1 = p1.getNameCount();
  if (!p1.getName(c1 - 1).equals(DOTDOT_PATH)) {
-// "." is inside p1 iif p1 is 1 or more "..".
+// "." is inside p1 iff p1 is 1 or more "..".
  // For the same reason above, we only need to
  // look at the tail.
  return -1;
diff -r d87f89c74f54 -r d2823a0c8dfa 
src/java.base/share/classes/java/lang/invoke/MethodHandle.java
--- a/src/java.base/share/classes/java/lang/invoke/MethodHandle.java 
Mon Oct 30 07:06:49 2017 -0700
+++ b/src/java.base/share/classes/java/lang/invoke/MethodHandle.java 
Mon Oct 30 17:18:47 2017 -0700

@@ -765,7 +765,7 @@
   * In every other case, all conversions are applied 
pairwise,

   * which means that each argument or return value is converted to
   * exactly one argument or return value (or no return value).
- * The applied conversions are defined by consulting the
+ * The applied conversions are defined by consulting
   * the corresponding component types of the old and new
   * method handle types.
   * 
diff -r d87f89c74f54 -r d2823a0c8dfa 
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java
--- 
a/src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java 
Mon Oct 30 07:06:49 2017 -0700
+++ 
b/src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java 
Mon Oct 30 17:18:47 2017 -0700

@@ -194,7 +194,7 @@
  static {
  // In case we need to double-back onto the 
StringConcatFactory during this
  // static initialization, make sure we have the reasonable 
defaults to complete
-// the static initialization properly. After that, actual 
users would use the
+// the static initialization properly. After that, actual 
users would use

  // the proper values we have read from the properties.
  STRATEGY = DEFAULT_STRATEGY;

Re: ThreadPoolExecutor and finalization

[David Holmes]

On 2/11/2017 3:46 AM, Peter Levart wrote:



On 11/01/17 13:34, David Holmes wrote:

On 1/11/2017 10:20 PM, Peter Levart wrote:

On 11/01/17 10:04, David Holmes wrote:

On 1/11/2017 6:16 PM, Peter Levart wrote:

On 11/01/17 02:49, David Holmes wrote:

Hi Roger,

On 31/10/2017 11:58 PM, Roger Riggs wrote:

Hi Peter,

Only native resources that do not map to the heap allocation/gc 
cycle need any kind
of cleanup.  I would work toward a model that encapsulates the 
reference to a native resource
with a corresponding allocation/release mechanism as you've 
described here and in the

thread on zip.

For cleanup purposes, the independence of each resource may 
improve robustness
by avoiding dependencies and opportunities for entanglements and 
bugs due to exceptions

and other failures.

In the case of TPE, the native resources are Threads, which keep 
running even if they are

unreferenced and are kept referenced via ThreadGroups.
I don't know the Executor code well enough to do more than 
speculate, but would suggest

that a cleaner (or similar) should be registered for each thread .


Threads are not native resources to be managed by Cleaners! A live 
Thread can never be cleaned. A dead thread has nothing to clean!


Right, but an idle thread, waiting for a task that will never come 
since the only entry point for submitting tasks is not reachable 
(the pool), may be cleaned...


cleaned? It can be interrupted if you know about it and find locate 
it. But it will not be eligible for cleaning ala Cleaner as it will 
always be strongly reachable.


Ah I see what you meant before. Yes, tracking Thread object with a 
Cleaner does not have any sense. But tracking thread pool object with 
a Cleaner and cleaning (stopping) threads as a result makes sense...


No, because live Threads will keep the thread pool strongly reachable.

If you manage to structure things such that the Threads don't keep the 
pool strongly reachable then you risk having the pool cleaned while 
still actively in use.


Pool is actively in use when it is still reachable. Only in that case 
can new tasks be submitted. When it is not reachable any more, no new 
tasks can be submitted and it can be shutdown():


     /**
  * Initiates an orderly shutdown in which previously submitted
  * tasks are executed, but no new tasks will be accepted...


Didn't we already determine that a Cleaner can't call shutdown() because 
that requires a strong reference it doesn't have?


I think Doug already summed this up. The existing finalize() is 
pointless because when it could be called there is nothing left to be 
"cleaned up". There's no need for any use of Cleaner (even if it could 
do anything useful).


David


Regards, Peter

Re: ThreadPoolExecutor and finalization

[David Holmes]

On 2/11/2017 5:07 AM, Stuart Marks wrote:

On 10/31/17 6:58 PM, David Holmes wrote:
I'm not sure why you say this isn't helpful. It's clearly not helpful 
to *clients* of TPE; but since finalize() is protected, the warning 
is clearly directed at subclasses, and it provides information about 
migrating away from finalization. Should say something different?


It isn't helpful to people subclassing TPE (or any class that defines 
finalize()) because the general strategies described in 
Object.finalize() can't be applied by the subclass independent of the 
class (TPE here) it is subclassing. Unless TPE provides an alternate 
mechanism, the subclass is stuck with finalize().


I don't understand why the subclass has to rely on the mechanism 
provided by its superclass (TPE here). I'm thinking of a case like the 
following.


Suppose a subclass has some independent native resource that it needs to 
clean up. Prior to the introduction of java.lang.ref, the only thing 
available was finalization. The subclass would have to override 
finalize(), do its own cleanup, and then make sure to call 
super.finalize().


With Cleaner in JDK 9, the subclass can refactor its native resources 
into a Cleanable, register it with a Cleaner, and then clean up its 
native resources when the Cleanable's clean() method is called.


Can't this be done independently of TPE and finalization?


Of course it can. The independent part of the subclass can use whatever 
mechanism it likes - it's independent. The point is that if the subclass 
needs to coordinate it's additional cleanup with the shutdown done by 
finalize() then it needs a means to do so.


David


s'marks

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

[Stuart Marks]

Good catch! Thanks, I'll fix it.

s'marks

On 11/1/17 4:01 PM, Stefan Zobel wrote:

Hi Stuart,

only a minor nit. In Map.java, the class-level
Javadoc anchor id="immutable" doesn't match the
href="#unmodifiable" used in the methods.

+ * Unmodifiable Maps

vs.

+ * See Unmodifiable Maps for details.


Regards,
Stefan


2017-11-01 0:49 GMT+01:00 Stuart Marks :

Updated webrev, based on comments from Brian and Roger:

 http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/

s'marks



On 10/30/17 3:50 PM, Stuart Marks wrote:


(also includes 8184690: add Collectors for collecting into unmodifiable
List, Set, and Map)

Hi all,

Here's an updated webrev for this changeset; the previous review thread is
here:


http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html

This webrev includes the following:

* specification revisions to provide clearer definitions of "view"
collections, "unmodifiable" collections, and "unmodifiable views"

* new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods

* new Collectors.toUnmodifiableList, Set, and Map methods

* tests for the new API methods

I've added some assertions that require some independence between the
source collection (or map) and the result of the copyOf() method.

I've made a small but significant change to Set.copyOf compared to the
previous round. Previously, it specified that the first of any equal
elements was preserved. Now, it is explicitly unspecified which of any
equals elements is preserved. This is consistent with Set.addAll,
Collectors.toSet, and the newly added Collectors.toUnmodifiableSet, none of
which specify which of duplicate elements is preserved.

(The outlier here is Stream.distinct, which specifies that the first
element of any duplicates is preserved, if the stream is ordered.)

I've also made some minor wording/editorial changes in response to
suggestions from David Holmes and Roger Riggs. I've kept the wording changes
that give emphasis to "unmodifiable" over "immutable." The term "immutable"
is inextricably intertwined with "persistent" when it comes to data
structures, and I believe we'll be explaining this forever if Java's
"immutable" means something different from everybody else's.

Webrev:

  http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/

Bugs:

  https://bugs.openjdk.java.net/browse/JDK-8177290
  add copy factory methods for unmodifiable List, Set, Map

  https://bugs.openjdk.java.net/browse/JDK-8184690
  add Collectors for collecting into unmodifiable List, Set, and
Map

Thanks,

s'marks

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

[Stuart Marks]

On 11/1/17 1:50 PM, Roger Riggs wrote:

Collection.java: Lines 110, 133, 166
The bold labels probably want to be on their own lines and not terminated by 
"." to look like headings

(or be headings if the CSS supports them)

I'll change these to be actual headings.

List.java: Consistency of markup references to unmodifiable List|Set|Map.
The List.of constructors put the reference on a separate line, but the copyOf 
constructor

does not.  You could probably omit the blank line.
Yeah, I should update all of these at some point. Given that there are 30-odd 
more methods to change, plus I have a pile of other collections doc changes to 
work on, I think I'll do these in a separate doc pass.
(BTW, the copyOf constructor does not always create a copy; I'm not sure if 
the method
name will result in an incorrect assumption but it may be misleading or a spec 
issue.)
Right. I haven't been able to come up with any names that had the right 
semantics and that didn't also have connotations that were misleading in some 
other way. I observe that Guava's Immutable collections have copyOf() methods 
with pretty much the same semantics. The Guava docs do note that their methods 
try to avoid making a copy if they can, but they explicitly say that the 
circumstances under which this occurs are unspecified. I've considered adding an 
@apiNote to this effect, but I haven't been able to convince myself that it 
would be helpful to do so. It would seem to raise new issues that we're 
unwilling to answer, such as exactly when is a copy is made vs. when the 
argument is returned. Better, I think, to have people make a copy whenever they 
think they need a copy, and have the implementation short-circuit this when it can.


s'marks



The same observations are true for Map and Set constructors.

Thanks, Roger




On 10/31/2017 7:49 PM, Stuart Marks wrote:

Updated webrev, based on comments from Brian and Roger:

http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/

s'marks


On 10/30/17 3:50 PM, Stuart Marks wrote:
(also includes 8184690: add Collectors for collecting into unmodifiable 
List, Set, and Map)


Hi all,

Here's an updated webrev for this changeset; the previous review thread is 
here:


http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html

This webrev includes the following:

* specification revisions to provide clearer definitions of "view" 
collections, "unmodifiable" collections, and "unmodifiable views"


* new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods

* new Collectors.toUnmodifiableList, Set, and Map methods

* tests for the new API methods

I've added some assertions that require some independence between the source 
collection (or map) and the result of the copyOf() method.


I've made a small but significant change to Set.copyOf compared to the 
previous round. Previously, it specified that the first of any equal 
elements was preserved. Now, it is explicitly unspecified which of any 
equals elements is preserved. This is consistent with Set.addAll, 
Collectors.toSet, and the newly added Collectors.toUnmodifiableSet, none of 
which specify which of duplicate elements is preserved.


(The outlier here is Stream.distinct, which specifies that the first element 
of any duplicates is preserved, if the stream is ordered.)


I've also made some minor wording/editorial changes in response to 
suggestions from David Holmes and Roger Riggs. I've kept the wording changes 
that give emphasis to "unmodifiable" over "immutable." The term "immutable" 
is inextricably intertwined with "persistent" when it comes to data 
structures, and I believe we'll be explaining this forever if Java's 
"immutable" means something different from everybody else's.


Webrev:

http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/

Bugs:

https://bugs.openjdk.java.net/browse/JDK-8177290
 add copy factory methods for unmodifiable List, Set, Map

https://bugs.openjdk.java.net/browse/JDK-8184690
 add Collectors for collecting into unmodifiable List, Set, and Map

Thanks,

s'marks

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

[Stuart Marks]

On 11/1/17 10:45 AM, Tagir Valeev wrote:

Set.of:

+if (coll instanceof ImmutableCollections.AbstractImmutableSet) {
+return (Set)coll;
+} else {
+return (Set)Set.of(coll.stream().distinct().toArray());

I think that good old Set.of(new HashSet<>(coll).toArray()) would
produce less garbage. distinct() also maintains HashSet internally,
but it removes the SIZED characteristic, so instead of preallocated
array you will have a SpinedBuffer which is less efficient than
AbstractCollection.toArray() implementation which just allocates the
array of exact size. What do you think?


Oh yes, good point. I had initially used stream().distinct() because I wanted to 
use distinct()'s semantics of preserving the first equal element among 
duplicates. But since I removed that requirement from the spec, using a HashSet 
as you suggest is much simpler.



Collectors:

+static final Set CH_UNORDERED_NOID
+= 
Collections.unmodifiableSet(EnumSet.of(Collector.Characteristics.UNORDERED));

Is it really more efficient currently than
Set.of(Collector.Characteristics.UNORDERED)? At least less objects
will be allocated with Set.of


Maybe. I'm just following the same pattern as the nearby characteristics. 
They're singletons, so this hardly makes any difference. What's really needed is 
an unmodifiable EnumSet, which I hope to get to at some point.



+Collector> toUnmodifiableList() {
+return new CollectorImpl<>((Supplier>)
ArrayList::new, List::add,
+   (left, right) -> {
left.addAll(right); return left; },
+   list -> (List)List.of(list.toArray()),
+   CH_NOID);
+}

Isn't it reasonable to use `e -> List.add(Objects.requireNonNull(e))`
instead of simply `List::add`? In this case if null is added, then
failure will occur much earlier, and the failure stacktrace would be
more relevant. The same for Set/Map.


Interesting. Initally I thought this would be a good idea, but then I tried it 
out. With the implementation in my latest webrev, the stack trace is this:



jshell> Arrays.asList(1, 2, null, 
4).stream().collect(Collectors.toUnmodifiableList())

|  java.lang.NullPointerException thrown
|at Objects.requireNonNull (Objects.java:221)
|at ImmutableCollections$ListN. (ImmutableCollections.java:234)
|at List.of (List.java:1039)
|at Collectors.lambda$toUnmodifiableList$6 (Collectors.java:299)
|at ReferencePipeline.collect (ReferencePipeline.java:515)
|at (#2:1)


With the lambda and requireNonNull(), the stack trace is this:


jshell> Arrays.asList(1, 2, null, 
4).stream().collect(Collectors.toUnmodifiableList())

|  java.lang.NullPointerException thrown
|at Objects.requireNonNull (Objects.java:221)
|at Collectors.lambda$toUnmodifiableList$5 (Collectors.java:298)
|at ReduceOps$3ReducingSink.accept (ReduceOps.java:169)
|at Spliterators$ArraySpliterator.forEachRemaining 
(Spliterators.java:948)
|at AbstractPipeline.copyInto (AbstractPipeline.java:484)
|at AbstractPipeline.wrapAndCopyInto (AbstractPipeline.java:474)
|at ReduceOps$ReduceOp.evaluateSequential (ReduceOps.java:913)
|at AbstractPipeline.evaluate (AbstractPipeline.java:234)
|at ReferencePipeline.collect (ReferencePipeline.java:511)
|at (#2:1)


It's true that with lambda+requireNonNull, the NPE will be thrown earlier in 
processing. But the stack trace isn't any clearer (the only user code is at the 
very bottom at location "#2:1") and it's still within the context of the same 
call to the stream's terminal collect() call. So, doesn't seem like it makes 
much difference.




+map ->
(Map)Map.ofEntries(map.entrySet().toArray(new Map.Entry[0])));

It's the same lambda in two versions of toUnmodifiableMap. Isn't it
better to extract it to the constant to prevent duplication in the
bytecode (or at least to method and refer to it via method reference)?


It might be better to do that, but given that this is the code I want to replace 
with a private interface, I don't think it's worth fiddling around with at this 
point.


Thanks,

s'marks



With best regards,
Tagir Valeev.




With best regards,
Tagir Valeev.

On Wed, Nov 1, 2017 at 12:49 AM, Stuart Marks  wrote:

Updated webrev, based on comments from Brian and Roger:

 http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/

s'marks



On 10/30/17 3:50 PM, Stuart Marks wrote:


(also includes 8184690: add Collectors for collecting into unmodifiable
List, Set, and Map)

Hi all,

Here's an updated webrev for this changeset; the previous review thread is
here:


http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html

This webrev includes the following:

* specification revisions to provide clearer definitions of "view"
collections, "unmodifiable" collections, and "unmodifiable views"

*

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

[Stefan Zobel]

Hi Stuart,

only a minor nit. In Map.java, the class-level
Javadoc anchor id="immutable" doesn't match the
href="#unmodifiable" used in the methods.

+ * Unmodifiable Maps

vs.

+ * See Unmodifiable Maps for details.


Regards,
Stefan


2017-11-01 0:49 GMT+01:00 Stuart Marks :
> Updated webrev, based on comments from Brian and Roger:
>
> http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/
>
> s'marks
>
>
>
> On 10/30/17 3:50 PM, Stuart Marks wrote:
>>
>> (also includes 8184690: add Collectors for collecting into unmodifiable
>> List, Set, and Map)
>>
>> Hi all,
>>
>> Here's an updated webrev for this changeset; the previous review thread is
>> here:
>>
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>>
>> This webrev includes the following:
>>
>> * specification revisions to provide clearer definitions of "view"
>> collections, "unmodifiable" collections, and "unmodifiable views"
>>
>> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods
>>
>> * new Collectors.toUnmodifiableList, Set, and Map methods
>>
>> * tests for the new API methods
>>
>> I've added some assertions that require some independence between the
>> source collection (or map) and the result of the copyOf() method.
>>
>> I've made a small but significant change to Set.copyOf compared to the
>> previous round. Previously, it specified that the first of any equal
>> elements was preserved. Now, it is explicitly unspecified which of any
>> equals elements is preserved. This is consistent with Set.addAll,
>> Collectors.toSet, and the newly added Collectors.toUnmodifiableSet, none of
>> which specify which of duplicate elements is preserved.
>>
>> (The outlier here is Stream.distinct, which specifies that the first
>> element of any duplicates is preserved, if the stream is ordered.)
>>
>> I've also made some minor wording/editorial changes in response to
>> suggestions from David Holmes and Roger Riggs. I've kept the wording changes
>> that give emphasis to "unmodifiable" over "immutable." The term "immutable"
>> is inextricably intertwined with "persistent" when it comes to data
>> structures, and I believe we'll be explaining this forever if Java's
>> "immutable" means something different from everybody else's.
>>
>> Webrev:
>>
>>  http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>>
>> Bugs:
>>
>>  https://bugs.openjdk.java.net/browse/JDK-8177290
>>  add copy factory methods for unmodifiable List, Set, Map
>>
>>  https://bugs.openjdk.java.net/browse/JDK-8184690
>>  add Collectors for collecting into unmodifiable List, Set, and
>> Map
>>
>> Thanks,
>>
>> s'marks

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

[Roger Riggs]

Hi Stuart,

A few editorial comments:

Collection.java: Lines 110, 133, 166
The bold labels probably want to be on their own lines and not 
terminated by "." to look like headings

(or be headings if the CSS supports them)

List.java: Consistency of markup references to unmodifiable List|Set|Map.
The List.of constructors put the reference on a separate line, but the 
copyOf constructor

does not.  You could probably omit the blank line.
(BTW, the copyOf constructor does not always create a copy; I'm not sure 
if the method
name will result in an incorrect assumption but it may be misleading or 
a spec issue.)


The same observations are true for Map and Set constructors.

Thanks, Roger




On 10/31/2017 7:49 PM, Stuart Marks wrote:

Updated webrev, based on comments from Brian and Roger:

    http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/

s'marks


On 10/30/17 3:50 PM, Stuart Marks wrote:
(also includes 8184690: add Collectors for collecting into 
unmodifiable List, Set, and Map)


Hi all,

Here's an updated webrev for this changeset; the previous review 
thread is here:


http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html

This webrev includes the following:

* specification revisions to provide clearer definitions of "view" 
collections, "unmodifiable" collections, and "unmodifiable views"


* new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" 
methods


* new Collectors.toUnmodifiableList, Set, and Map methods

* tests for the new API methods

I've added some assertions that require some independence between the 
source collection (or map) and the result of the copyOf() method.


I've made a small but significant change to Set.copyOf compared to 
the previous round. Previously, it specified that the first of any 
equal elements was preserved. Now, it is explicitly unspecified which 
of any equals elements is preserved. This is consistent with 
Set.addAll, Collectors.toSet, and the newly added 
Collectors.toUnmodifiableSet, none of which specify which of 
duplicate elements is preserved.


(The outlier here is Stream.distinct, which specifies that the first 
element of any duplicates is preserved, if the stream is ordered.)


I've also made some minor wording/editorial changes in response to 
suggestions from David Holmes and Roger Riggs. I've kept the wording 
changes that give emphasis to "unmodifiable" over "immutable." The 
term "immutable" is inextricably intertwined with "persistent" when 
it comes to data structures, and I believe we'll be explaining this 
forever if Java's "immutable" means something different from 
everybody else's.


Webrev:

http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/

Bugs:

 https://bugs.openjdk.java.net/browse/JDK-8177290
 add copy factory methods for unmodifiable List, Set, Map

 https://bugs.openjdk.java.net/browse/JDK-8184690
 add Collectors for collecting into unmodifiable List, Set, 
and Map


Thanks,

s'marks

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

[Patrick Reinhart]

In this case I would prefer a non static copyOf() method on the list to create 
a unmodifiable list/set/map, where the optimal factory method can be called. 
This would also solve the problem of a concurrent implementation.

-Patrick


> Am 01.11.2017 um 21:05 schrieb Louis Wasserman :
> 
> I disagree, actually.  Collections with size zero and one are significantly 
> more common than bigger collections.
> 
> In Guava's immutable collection factories (ImmutableList.of(...) etc.), we 
> observed a roughly exponential decline in the number of users of factory 
> methods of each size: if N people created empty lists, ~N/2 created singleton 
> lists, ~N/4 created two-element lists, etc.  We got noticeable pushback from 
> RAM-sensitive customers when we eliminated some specialized singleton 
> collection implementations.
> 
> Our experience has been that specializing for N=0 and N=1 does pay off.  
> Probably not N=2, though?
> 
> On Wed, Nov 1, 2017 at 12:45 PM Patrick Reinhart  > wrote:
> 
> > Am 01.11.2017 um 20:25 schrieb Stuart Marks  > >:
> >
> > On 10/31/17 5:52 PM, Bernd Eckenfels wrote:
> >> Having a List.of(List) copy constructor would save an additional array 
> >> copy in the collector Which uses  (List)List.of(list.toArray())
> >
> > The quickest way to get all the elements from the source collection is to 
> > call toArray() on it. Some copy constructors (like ArrayList's) simply use 
> > the returned array for internal storage. This saves a copy, but it relies 
> > on the source collection's toArray() implementation to be correct. In 
> > particular, the returned array must be freshly created, and that array must 
> > be of type Object[]. If either of these is violated, it will break the 
> > ArrayList.
> >
> > The "immutable" collections behind List.of/copyOf/etc. are fastidious about 
> > allocating their internal arrays in order to ensure that they are 
> > referenced only from within the newly created collection. This requires 
> > making an „extra" copy of the array returned by toArray().
> >
> > An alternative is for the new collection to preallocate its internal array 
> > using the source's size(), and then to copy the elements out. But the 
> > source’s
> > size might change during the copy (e.g., if it’s a concurrent collection) 
> > so this complicates things.
> 
> I think the array „overhead“ would be only for the cases of zero, one and two 
> value implementations. That seems to me not worth of optimizing…
> 
> > I think the only safe way to avoid the extra copy is to create a private 
> > interface that can be used by JDK implementations.
> >
> > s'marks
> 
> -Patrick

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

[Louis Wasserman]

I disagree, actually.  Collections with size zero and one are significantly
more common than bigger collections.

In Guava's immutable collection factories (ImmutableList.of(...) etc.), we
observed a roughly exponential decline in the number of users of factory
methods of each size: if N people created empty lists, ~N/2 created
singleton lists, ~N/4 created two-element lists, etc.  We got noticeable
pushback from RAM-sensitive customers when we eliminated some specialized
singleton collection implementations.

Our experience has been that specializing for N=0 and N=1 does pay off.
Probably not N=2, though?

On Wed, Nov 1, 2017 at 12:45 PM Patrick Reinhart  wrote:

>
> > Am 01.11.2017 um 20:25 schrieb Stuart Marks :
> >
> > On 10/31/17 5:52 PM, Bernd Eckenfels wrote:
> >> Having a List.of(List) copy constructor would save an additional array
> copy in the collector Which uses  (List)List.of(list.toArray())
> >
> > The quickest way to get all the elements from the source collection is
> to call toArray() on it. Some copy constructors (like ArrayList's) simply
> use the returned array for internal storage. This saves a copy, but it
> relies on the source collection's toArray() implementation to be correct.
> In particular, the returned array must be freshly created, and that array
> must be of type Object[]. If either of these is violated, it will break the
> ArrayList.
> >
> > The "immutable" collections behind List.of/copyOf/etc. are fastidious
> about allocating their internal arrays in order to ensure that they are
> referenced only from within the newly created collection. This requires
> making an „extra" copy of the array returned by toArray().
> >
> > An alternative is for the new collection to preallocate its internal
> array using the source's size(), and then to copy the elements out. But the
> source’s
> > size might change during the copy (e.g., if it’s a concurrent
> collection) so this complicates things.
>
> I think the array „overhead“ would be only for the cases of zero, one and
> two value implementations. That seems to me not worth of optimizing…
>
> > I think the only safe way to avoid the extra copy is to create a private
> interface that can be used by JDK implementations.
> >
> > s'marks
>
> -Patrick
>

Re: Confusion (or bugs) regarding the 'UNORDERED' Collector Characteristic

[Stuart Marks]

On 10/27/17 1:56 PM, Chris Dennis wrote:

I’m very confused about what was intended with the ‘UNORDERED’ Collector 
characteristic.  My logical inference was that unordered as a Collector 
characteristic meant that the result of this collector applied to any stream 
was independent of the order of element delivery.  This means that the stream 
backend could safely elide any ordered characteristic of the stream driving the 
collector and open up more possible execution pathways (parallelism for 
example).  The javadoc for unordered is two sentences:

/**
  * Indicates that the collection operation does not commit to preserving
  * the encounter order of input elements.  (This might be true if the
  * result container has no intrinsic order, such as a {@link Set}.)
  */

The first sentence makes sense, although it is phrased in a way that also 
explains the behavior seen if you flag an order sensitive collector as 
unordered.  The second sentence is weird, it implies a relation between 
collectors, encounter order, and an inherent ordering or iteration order of a 
collection object that might be the target of a collection. To me the important 
property of a Collection object with regards to ordering of a Collector that 
fills it is whether reordering the insertions to the collection can change the 
resultant state, for a Set this is clearly true since only the first presented 
element that is equal will be stored.


I think you need to be very careful making any assumptions about which of equal 
elements will end up in a set. Consider the spec of Set.addAll:


Adds all of the elements in the specified collection to this set
if they're not already present.

Suppose the source collection has several non-identical elements that are equal 
to each other. This says nothing about which of them is preserved, and indeed 
there is no mention whatsoever about the identity of equal objects. The entire 
Set spec is defined in terms of equals, not identity. So I think it's mistaken 
to assume that the first of equal but non-identical objects will be stored.


(This is also the reason I changed the specification of Set.copyOf in my recent 
draft, to remove the requirement that the first such element be preserved. 
Set.addAll and Collectors.toSet make no such stipulation.)



There is also a paragraph in the Collector javadoc:

* For collectors that do not have the {@code UNORDERED} characteristic,
* two accumulated results {@code a1} and {@code a2} are equivalent if
* {@code finisher.apply(a1).equals(finisher.apply(a2))}.  For unordered
* collectors, equivalence is relaxed to allow for non-equality related to
* differences in order.  (For example, an unordered collector that accumulated
* elements to a {@code List} would consider two lists equivalent if they
* contained the same elements, ignoring order.)

This seems weird, why would we try to define correctness of a parallel 
reduction against a collector that was sensitive to ordering.


This is indeed a bit odd, but it makes sense. You might have a collector that 
collects into a List, because you want to collect duplicates. But you might not 
actually care about the order; you just want all the elements. The fact that 
List stores elements in some order is irrelevant. In this case such a collector 
would quite reasonably have the UNORDERED characteristic.



Finally, when investigating the properties of the collectors returned from the 
Collectors static factory I was surprised to discover that none of the 
collectors that are truly unordered were marked as such (counting, min, max, 
averaging, summing, summarizing), and the only collector that was marked as 
unordered was Collectors.toSet(), which although it is explicitly marked as 
unordered seems like it really shouldn’t be.

Whats going on here?  Which parts of all this are intended and which (if any) 
are bugs?


Well I think it does make sense for the toSet() collector to be UNORDERED. It 
may be the case, though, that other collectors are lacking the UNORDERED 
characteristic that they should have. That sounds like it might be a bug.


s'marks




Thanks in advance for any enlightenment,

Chris Dennis

P.S. Coincidentally the unordered toSet() collector works perfectly at the 
moment due to the happy interaction of the Spliterator.trySplit() contract and 
the folding/combining behavior of the stream implementations parallel reduction 
algorithm.

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

[Patrick Reinhart]

> Am 01.11.2017 um 20:25 schrieb Stuart Marks :
> 
> On 10/31/17 5:52 PM, Bernd Eckenfels wrote:
>> Having a List.of(List) copy constructor would save an additional array copy 
>> in the collector Which uses  (List)List.of(list.toArray())
> 
> The quickest way to get all the elements from the source collection is to 
> call toArray() on it. Some copy constructors (like ArrayList's) simply use 
> the returned array for internal storage. This saves a copy, but it relies on 
> the source collection's toArray() implementation to be correct. In 
> particular, the returned array must be freshly created, and that array must 
> be of type Object[]. If either of these is violated, it will break the 
> ArrayList.
> 
> The "immutable" collections behind List.of/copyOf/etc. are fastidious about 
> allocating their internal arrays in order to ensure that they are referenced 
> only from within the newly created collection. This requires making an 
> „extra" copy of the array returned by toArray().
> 
> An alternative is for the new collection to preallocate its internal array 
> using the source's size(), and then to copy the elements out. But the source’s
> size might change during the copy (e.g., if it’s a concurrent collection) so 
> this complicates things.

I think the array „overhead“ would be only for the cases of zero, one and two 
value implementations. That seems to me not worth of optimizing…

> I think the only safe way to avoid the extra copy is to create a private 
> interface that can be used by JDK implementations.
> 
> s'marks

-Patrick

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

[Stuart Marks]

On 10/31/17 5:52 PM, Bernd Eckenfels wrote:

Having a List.of(List) copy constructor would save an additional array copy in the 
collector Which uses  (List)List.of(list.toArray())


The quickest way to get all the elements from the source collection is to call 
toArray() on it. Some copy constructors (like ArrayList's) simply use the 
returned array for internal storage. This saves a copy, but it relies on the 
source collection's toArray() implementation to be correct. In particular, the 
returned array must be freshly created, and that array must be of type Object[]. 
If either of these is violated, it will break the ArrayList.


The "immutable" collections behind List.of/copyOf/etc. are fastidious about 
allocating their internal arrays in order to ensure that they are referenced 
only from within the newly created collection. This requires making an "extra" 
copy of the array returned by toArray().


An alternative is for the new collection to preallocate its internal array using 
the source's size(), and then to copy the elements out. But the source's size 
might change during the copy (e.g., if it's a concurrent collection) so this 
complicates things.


I think the only safe way to avoid the extra copy is to create a private 
interface that can be used by JDK implementations.


s'marks

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

[Xueming Shen]

Hi Peter,

I like the idea of moving get/releaseInflter() into CleanableResource, 
though I doubt
how much it can really help the GC it should be a good thing to do to 
remove the strong
reference of ZipFile from stream's cleaner, and the code appears a 
little cleaner as well.


I was debating with myself whether or not the ZipFile.close() should 
throw an
UncheckedIOException (like those java.nio.file.Files methods do). But 
you're right it's

not good to simply ignoring them silently. Now I catch/unwarp the potential
UncheckedIOException from res.clean() and re-throw the embedded ioe.

I need to dig a little to recall the real reason of zsrc==null check in 
ensureOpen()
the comment does not sound updated. res.zsrc is not final and it is set 
to null

when clean up/close. So I keep it for now.

Most (if not all?) "minor nit" has been updated accordingly.

It seems like we might have have put the "finalize()" method back as an 
empty-body

method for compatibility reason. So not done yet :-)

http://cr.openjdk.java.net/~sherman/8185582/webrev/

thanks,
sherman

On 11/1/17, 5:04 AM, Peter Levart wrote:

Hi Sherman,

On 11/01/17 00:25, Xueming Shen wrote:

Hi Peter,

After tried couple implementations it seems the most reasonable 
approach is to
use the coding pattern you suggested to move all pieces into ZSStream 
Ref. Given
we are already using the internal API CleanerFactory it is attractive 
to go a little
further to subclass PhantomCleanable directly (in which I would 
assume we can
save one extra object), but I think I'd better to follow the 
"suggested" clean usage

(to register a runnable into the cleaner) for now.

  39 class ZStreamRef implements Runnable {
  40
  41 private LongConsumer end;
  42 private volatile long address;
  43 private final Cleanable cleanable;
  44
  45 ZStreamRef (Object owner, LongSupplier addr, LongConsumer 
end) {
  46 this.cleanable = 
CleanerFactory.cleaner().register(owner, this);

  47 this.end = end;
  48 this.address = addr.getAsLong();
  49 }
  50

Similar change has been made for the ZipFile cleaner to follow the 
same coding
pattern. The "cleaner" is now renamed from Releaser to 
CleanableResource.


http://cr.openjdk.java.net/~sherman/8185582/webrev/

Thanks,
Sherman


This looks mostly fine. One minor nit is that ZStreamRef.address field 
does not have to be volatile. I checked all usages of 
ZStreamRef.address() and all of them invoke it while holding a lock on 
the ZStreamRef instance. The ZStreamRef.run() which modifies the 
address is also synchronized. The other minor nit is that the 
following ZipFile imports are not needed:


import java.nio.file.Path;
import java.util.Map;
import jdk.internal.misc.JavaIORandomAccessFileAccess;
import static java.util.zip.ZipConstants.*;

(at least my IDEA colors them unused)

Cleaner is modified in this webrev (just one empty line deleted) - 
better not touch this file in this changeset


Additional nits in ZipFile:

- in lines 355, 356 you pull out two res fields into locals, but then 
not use them consistently (lines 372, 389)
- line 403 has a TAB character (is this OK?) and shows incorrectly 
indented in my editor (should I set tab stops differently?)

- line 457 - while should actually be if ?
- in ensureOpen() the check for res.zsrc == null can never succeed 
(CleanableResource.zsrc is a final field. If CleanableResource 
constructor fails, there's no res object and there's no ZipFile object 
either as ZipFile constructor does not do anything for this to escape 
prematurely)
- why don't you let IOExceptions thrown from CleanableResource.run() 
propagate out of ZipFile.close() ?


I would also rename static method Source.close(Source) to 
Source.release(Source) so it would not clash with instance method 
Source.close() which makes it ambiguous when one wants to use 
Source::close method reference (both methods apply). I would also make 
static methods Source.get() and Source.release(Source) package-private 
(currently one is public and the other is private, which needs 
compiler bridges to be invoked) and both are in a private nested class.


Inflater/Deflater/ZipFile now follow the coding pattern as suggested. 
But ZipFileInflaterInputStream still does not. It's not critical since 
failing to register cleanup which releases the inflater back into 
cache would simply mean that Inflater employs its own cleanup and ends 
itself.


And now another thing I would like to discuss. Why an initiative for 
using Cleaner instead of finalization()? Among drawbacks finalization 
has one of the more troubling is that the tracked referent survives 
the GC cycle that initiates its finalization reference processing 
pipeline, so the GC may reclaim the object (and referenced objects) 
only after the finalize() has finished in yet another GC round. 
Cleaner API separates the tracked object from the cleanup function and 
state needed to perform it into distinct instances. The tracked obje

Re: ThreadPoolExecutor and finalization

[Stuart Marks]

On 10/31/17 6:58 PM, David Holmes wrote:
I'm not sure why you say this isn't helpful. It's clearly not helpful to 
*clients* of TPE; but since finalize() is protected, the warning is clearly 
directed at subclasses, and it provides information about migrating away from 
finalization. Should say something different?


It isn't helpful to people subclassing TPE (or any class that defines 
finalize()) because the general strategies described in Object.finalize() can't 
be applied by the subclass independent of the class (TPE here) it is 
subclassing. Unless TPE provides an alternate mechanism, the subclass is stuck 
with finalize().


I don't understand why the subclass has to rely on the mechanism provided by its 
superclass (TPE here). I'm thinking of a case like the following.


Suppose a subclass has some independent native resource that it needs to clean 
up. Prior to the introduction of java.lang.ref, the only thing available was 
finalization. The subclass would have to override finalize(), do its own 
cleanup, and then make sure to call super.finalize().


With Cleaner in JDK 9, the subclass can refactor its native resources into a 
Cleanable, register it with a Cleaner, and then clean up its native resources 
when the Cleanable's clean() method is called.


Can't this be done independently of TPE and finalization?

s'marks

Re: ThreadPoolExecutor and finalization

[Peter Levart]

On 11/01/17 13:34, David Holmes wrote:

On 1/11/2017 10:20 PM, Peter Levart wrote:

On 11/01/17 10:04, David Holmes wrote:

On 1/11/2017 6:16 PM, Peter Levart wrote:

On 11/01/17 02:49, David Holmes wrote:

Hi Roger,

On 31/10/2017 11:58 PM, Roger Riggs wrote:

Hi Peter,

Only native resources that do not map to the heap allocation/gc 
cycle need any kind
of cleanup.  I would work toward a model that encapsulates the 
reference to a native resource
with a corresponding allocation/release mechanism as you've 
described here and in the

thread on zip.

For cleanup purposes, the independence of each resource may 
improve robustness
by avoiding dependencies and opportunities for entanglements and 
bugs due to exceptions

and other failures.

In the case of TPE, the native resources are Threads, which keep 
running even if they are

unreferenced and are kept referenced via ThreadGroups.
I don't know the Executor code well enough to do more than 
speculate, but would suggest

that a cleaner (or similar) should be registered for each thread .


Threads are not native resources to be managed by Cleaners! A live 
Thread can never be cleaned. A dead thread has nothing to clean!


Right, but an idle thread, waiting for a task that will never come 
since the only entry point for submitting tasks is not reachable 
(the pool), may be cleaned...


cleaned? It can be interrupted if you know about it and find locate 
it. But it will not be eligible for cleaning ala Cleaner as it will 
always be strongly reachable.


Ah I see what you meant before. Yes, tracking Thread object with a 
Cleaner does not have any sense. But tracking thread pool object with 
a Cleaner and cleaning (stopping) threads as a result makes sense...


No, because live Threads will keep the thread pool strongly reachable.

If you manage to structure things such that the Threads don't keep the 
pool strongly reachable then you risk having the pool cleaned while 
still actively in use.


Pool is actively in use when it is still reachable. Only in that case 
can new tasks be submitted. When it is not reachable any more, no new 
tasks can be submitted and it can be shutdown():


    /**
 * Initiates an orderly shutdown in which previously submitted
 * tasks are executed, but no new tasks will be accepted...

Regards, Peter

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

[Tagir Valeev]

Hello!

Set.of:

+if (coll instanceof ImmutableCollections.AbstractImmutableSet) {
+return (Set)coll;
+} else {
+return (Set)Set.of(coll.stream().distinct().toArray());

I think that good old Set.of(new HashSet<>(coll).toArray()) would
produce less garbage. distinct() also maintains HashSet internally,
but it removes the SIZED characteristic, so instead of preallocated
array you will have a SpinedBuffer which is less efficient than
AbstractCollection.toArray() implementation which just allocates the
array of exact size. What do you think?

Collectors:

+static final Set CH_UNORDERED_NOID
+= 
Collections.unmodifiableSet(EnumSet.of(Collector.Characteristics.UNORDERED));

Is it really more efficient currently than
Set.of(Collector.Characteristics.UNORDERED)? At least less objects
will be allocated with Set.of

+Collector> toUnmodifiableList() {
+return new CollectorImpl<>((Supplier>)
ArrayList::new, List::add,
+   (left, right) -> {
left.addAll(right); return left; },
+   list -> (List)List.of(list.toArray()),
+   CH_NOID);
+}

Isn't it reasonable to use `e -> List.add(Objects.requireNonNull(e))`
instead of simply `List::add`? In this case if null is added, then
failure will occur much earlier, and the failure stacktrace would be
more relevant. The same for Set/Map.

+map ->
(Map)Map.ofEntries(map.entrySet().toArray(new Map.Entry[0])));

It's the same lambda in two versions of toUnmodifiableMap. Isn't it
better to extract it to the constant to prevent duplication in the
bytecode (or at least to method and refer to it via method reference)?

With best regards,
Tagir Valeev.




With best regards,
Tagir Valeev.

On Wed, Nov 1, 2017 at 12:49 AM, Stuart Marks  wrote:
> Updated webrev, based on comments from Brian and Roger:
>
> http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/
>
> s'marks
>
>
>
> On 10/30/17 3:50 PM, Stuart Marks wrote:
>>
>> (also includes 8184690: add Collectors for collecting into unmodifiable
>> List, Set, and Map)
>>
>> Hi all,
>>
>> Here's an updated webrev for this changeset; the previous review thread is
>> here:
>>
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>>
>> This webrev includes the following:
>>
>> * specification revisions to provide clearer definitions of "view"
>> collections, "unmodifiable" collections, and "unmodifiable views"
>>
>> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods
>>
>> * new Collectors.toUnmodifiableList, Set, and Map methods
>>
>> * tests for the new API methods
>>
>> I've added some assertions that require some independence between the
>> source collection (or map) and the result of the copyOf() method.
>>
>> I've made a small but significant change to Set.copyOf compared to the
>> previous round. Previously, it specified that the first of any equal
>> elements was preserved. Now, it is explicitly unspecified which of any
>> equals elements is preserved. This is consistent with Set.addAll,
>> Collectors.toSet, and the newly added Collectors.toUnmodifiableSet, none of
>> which specify which of duplicate elements is preserved.
>>
>> (The outlier here is Stream.distinct, which specifies that the first
>> element of any duplicates is preserved, if the stream is ordered.)
>>
>> I've also made some minor wording/editorial changes in response to
>> suggestions from David Holmes and Roger Riggs. I've kept the wording changes
>> that give emphasis to "unmodifiable" over "immutable." The term "immutable"
>> is inextricably intertwined with "persistent" when it comes to data
>> structures, and I believe we'll be explaining this forever if Java's
>> "immutable" means something different from everybody else's.
>>
>> Webrev:
>>
>>  http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>>
>> Bugs:
>>
>>  https://bugs.openjdk.java.net/browse/JDK-8177290
>>  add copy factory methods for unmodifiable List, Set, Map
>>
>>  https://bugs.openjdk.java.net/browse/JDK-8184690
>>  add Collectors for collecting into unmodifiable List, Set, and
>> Map
>>
>> Thanks,
>>
>> s'marks

Re: RFR Re: [PATCH] 8178117: Add public state constructors for Int/Long/DoubleSummaryStatistics

[Paul Sandoz]

> On 31 Oct 2017, at 16:46, joe darcy  wrote:
>>> 
>> In that case we need to double (sorry) down on the NaNs and include sum as 
>> well:
>> 
>> *   {@code (min <= max && !isNaN(sum)) || (isNaN(min) && isNaN(max) && 
>> isNaN(sum))}
> 
> A more complete test for the numerical consistency conditions would be 
> something like
> 
>min <= sum/count  <= max
> 
> However, that would require a bit of thought due to potential round-off so 
> this might not be worth the complexity trade-off. (If any of min, sum, and 
> max were NaN, the comparisons would be false.)
> 

Yes, my recollection from the discussions we concluded not to do such checks.

Paul.

Re: [10] RFR 8186046 Minimal ConstantDynamic support

[Dmitry Samersoff]

Paul,

Thank you!

-Dmitry


On 31.10.2017 20:32, Paul Sandoz wrote:
> 
>> On 31 Oct 2017, at 05:58, Dmitry Samersoff  
>> wrote:
>>
>> Paul and Frederic,
>>
>> Thank you.
>>
>> One more question. Do we need to call verify_oop below?
>>
>> 509   { // Check for the null sentinel.
>> ...
>> 517  xorptr(result, result);  // NULL object reference
>> ...
>>
>> 521   if (VerifyOops) {
>> 522  verify_oop(result);
>> 523   }
>>
> 
> I believe it’s harmless.
> 
> When the flag is on it eventually results in a call to the stub generated by 
> generate_verify_oop:
> 
> http://hg.openjdk.java.net/jdk10/hs/file/tip/src/hotspot/cpu/x86/stubGenerator_x86_64.cpp#l1023
> 
> // make sure object is 'reasonable'
> __ testptr(rax, rax);
> __ jcc(Assembler::zero, exit); // if obj is NULL it is OK
> 
> If the oop is null the verification will exit safely.
> 
> Paul.
> 
>> -Dmitry
>>
>>
>> On 31.10.2017 00:56, Frederic Parain wrote:
>>> I’m seeing no issue with rcx being aliased in this code.
>>>
>>> Fred
>>>
 On Oct 30, 2017, at 15:44, Paul Sandoz  wrote:

 Hi,

 Thanks for reviewing.

> On 30 Oct 2017, at 11:05, Dmitry Samersoff  
> wrote:
>
> Paul,
>
> templateTable_x86.cpp:
>
> 564   const Register flags = rcx;
> 565   const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);
>
> Should we use another register for rarg under NOT_LP64 ?
>

 I think it should be ok, it i ain’t an expert here on the interpreter and 
 the calling conventions, so please correct me.

 Some more context:

 +  const Register flags = rcx;
 +  const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);
 +  __ movl(rarg, (int)bytecode());

 The current bytecode code is loaded into “rarg”

 +  call_VM(obj, CAST_FROM_FN_PTR(address, 
 InterpreterRuntime::resolve_ldc), rarg);

 Then “rarg" is the argument to the call to 
 InterpreterRuntime::resolve_ldc, after which it is no longer referred to.

 +#ifndef _LP64
 +  // borrow rdi from locals
 +  __ get_thread(rdi);
 +  __ get_vm_result_2(flags, rdi);
 +  __ restore_locals();
 +#else
 +  __ get_vm_result_2(flags, r15_thread);
 +#endif

 The result from the call is then loaded into flags.

 So i don’t think it matters in this case if rcx is aliased.

 Paul.

> -Dmitry
>
>
> On 10/26/2017 08:03 PM, Paul Sandoz wrote:
>> Hi,
>>
>> Please review the following patch for minimal dynamic constant support:
>>
>> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/
>>  
>> 
>>
>> https://bugs.openjdk.java.net/browse/JDK-8186046 
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8186209 
>> 
>>
>> This patch is based on the JDK 10 unified HotSpot repository. Testing so 
>> far looks good.
>>
>> By minimal i mean just the support in the runtime for a dynamic constant 
>> pool entry to be referenced by a LDC instruction or a bootstrap method 
>> argument. Much of the work leverages the foundations built by invoke 
>> dynamic but is arguably simpler since resolution is less complex.
>>
>> A small set of bootstrap methods will be proposed as a follow on issue 
>> for 10 (these are currently being refined in the amber repository).
>>
>> Bootstrap method invocation has not changed (and the rules are the same 
>> for dynamic constants and indy). It is planned to enhance this in a 
>> further major release to support lazy resolution of bootstrap method 
>> arguments.
>>
>> The CSR for the VM specification is here:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8189199 
>> 
>>
>> the j.l.invoke package documentation was also updated but please 
>> consider the VM specification as the definitive "source of truth" (we 
>> may clean up this area further later on so it becomes more informative, 
>> and that may also apply to duplicative text on MethodHandles/VarHandles).
>>
>> Any AoT-related work will be deferred to a future release.
>>
>> —
>>
>> This patch only supports x64 platforms. There is a small set of changes 
>> specific to x64 (specifically to support null and primitives constants, 
>> as prior to this patch null was used as a sentinel for resolution and 
>> certain primitives types would never have been encountered, such as say 
>> byte).
>>
>> We will need to follow up with the SPARC platform and it is 
>> hoped/anticipated that OpenJDK members responsible for other platforms 
>> (namely ARM and PPC) will separately pr

Re: JDK-8067661: transferTo proposal for Appendable

[Patrick Reinhart]

I tried to put that in my latest proposal:

/**
 * Reads all characters from this readable and writes the characters to
 * the given appendable in the order that they are read. On return, this
 * readable will be at end its data.
 * 
 * This method may block indefinitely reading from the readable, or
 * writing to the appendable. The behavior for the case where the readable
 * and/or appendable is asynchronously closed, or the thread
 * interrupted during the transfer, is highly readable and appendable
 * specific, and therefore not specified.
 * 
 * If an I/O error occurs reading from the readable or writing to the
 * appendable, then it may do so after some characters have been read or
 * written. Consequently the readable may not be at end of its data and
 * one, or both participants may be in an inconsistent state. That in mind
 * all additional measures required by one or both participants in order to
 * eventually free their internal resources have to be taken by the caller
 * of this method.
 *
 * @param  out the appendable, non-null
 * @return the number of characters transferred
 * @throws IOException if an I/O error occurs when reading or writing
 * @throws NullPointerException if {@code out} is {@code null}
 *
 * @since 18.3
 */
default long transferTo(Appendable out) throws IOException {

}

-Patrick

Am 01.11.2017 um 13:42 schrieb Alan Bateman:
> On 16/12/2014 22:54, Pavel Rappo wrote:
>> Hi Patrick, nice to hear from you again! I agree we should consider
>> adding this
>> method. Unfortunately, from the spec point of view I suppose this one
>> will
>> require a lot more chewing. For instance there's nothing that can be
>> closed
>> either in Readable or Appendable (in general case), since neither of
>> them is
>> java.io.Closeable or even java.lang.AutoCloseable. In my opinion, all
>> mentions
>> of 'close' operations should go.
> I agree that transferTo(Writer) is a no-brainer. When changed to be
> more general and Appendable then it might be simplest to just deal
> with the AutoCloseable case in the javadoc.
>
> -Alan

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

[Roger Riggs]

Hi Peter,

The Supplier/Consumer allocator/deallocator version is useful and 
interesting encapsulation.


We should be able to do better than exposing raw native pointers for 
other resources.

They should have better more complete encapsulation including their cleanup.
For example, the recent work on FileDescriptor.
The current state with direct memory pointers floating freely is quite 
sensitive.


I'm hopeful that Panama will support that direction.

In CleanerImp:322, is there a JMM issue with the escape of the reference 
to PhantomCleanableResource
to the cleaner thread before the constructor has finished and published 
the instance?
True, the constructor holds this until the very end of the method but 
what makes sure
the new valuewritten to the resource field will be read by the cleanable 
thread?


Thanks, Roger


On 10/31/2017 11:17 PM, mandy chung wrote:



On 10/31/17 4:07 PM, Peter Levart wrote:

Hi Mandy, Sherman, Roger,

On 10/31/17 00:25, mandy chung wrote:


I played a little with an idea of how such additional Cleaner API 
might look like. Here's what I came up with, together with how this 
would apply to ZipFile/Inflater/Deflater:


http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.02/

So, what do you think of this new Cleaner API extension?


This serves as a good starting point.  I converted the 
ClassLoader.NativeLibrary to use it:

http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev-cleanable/

Looks reasonably well and probably some other improvement I could do.  
I use createResource rather than createLongResource which is 
specialized for an address. We should check out panama how this plays out.


I think it would be useful to iterate on several more use cases and it 
would be better to separate this RFE from JDK-8185582 so that we can 
explore with its own pace.


Mandy

Re: JDK-8067661: transferTo proposal for Appendable

[Alan Bateman]

On 16/12/2014 22:54, Pavel Rappo wrote:

Hi Patrick, nice to hear from you again! I agree we should consider adding this
method. Unfortunately, from the spec point of view I suppose this one will
require a lot more chewing. For instance there's nothing that can be closed
either in Readable or Appendable (in general case), since neither of them is
java.io.Closeable or even java.lang.AutoCloseable. In my opinion, all mentions
of 'close' operations should go.
I agree that transferTo(Writer) is a no-brainer. When changed to be more 
general and Appendable then it might be simplest to just deal with the 
AutoCloseable case in the javadoc.


-Alan

Re: ThreadPoolExecutor and finalization

[David Holmes]

On 1/11/2017 10:20 PM, Peter Levart wrote:

On 11/01/17 10:04, David Holmes wrote:

On 1/11/2017 6:16 PM, Peter Levart wrote:

On 11/01/17 02:49, David Holmes wrote:

Hi Roger,

On 31/10/2017 11:58 PM, Roger Riggs wrote:

Hi Peter,

Only native resources that do not map to the heap allocation/gc 
cycle need any kind
of cleanup.  I would work toward a model that encapsulates the 
reference to a native resource
with a corresponding allocation/release mechanism as you've 
described here and in the

thread on zip.

For cleanup purposes, the independence of each resource may improve 
robustness
by avoiding dependencies and opportunities for entanglements and 
bugs due to exceptions

and other failures.

In the case of TPE, the native resources are Threads, which keep 
running even if they are

unreferenced and are kept referenced via ThreadGroups.
I don't know the Executor code well enough to do more than 
speculate, but would suggest

that a cleaner (or similar) should be registered for each thread .


Threads are not native resources to be managed by Cleaners! A live 
Thread can never be cleaned. A dead thread has nothing to clean!


Right, but an idle thread, waiting for a task that will never come 
since the only entry point for submitting tasks is not reachable (the 
pool), may be cleaned...


cleaned? It can be interrupted if you know about it and find locate 
it. But it will not be eligible for cleaning ala Cleaner as it will 
always be strongly reachable.


Ah I see what you meant before. Yes, tracking Thread object with a 
Cleaner does not have any sense. But tracking thread pool object with a 
Cleaner and cleaning (stopping) threads as a result makes sense...


No, because live Threads will keep the thread pool strongly reachable.

If you manage to structure things such that the Threads don't keep the 
pool strongly reachable then you risk having the pool cleaned while 
still actively in use.


David

David


Regards, Peter



David


Regards, Peter



David
-

For TPE, since Threads do not become unreferenced, the part of the 
spec related to finalize

being called by the finalizer thread is moot.

$.02, Roger

On 10/31/2017 5:24 AM, Peter Levart wrote:

Hi,

Here are some of my thoughts...

On 10/31/17 05:37, David Holmes wrote:

Hi Roger,

On 31/10/2017 12:43 AM, Roger Riggs wrote:

Hi David,

On 10/30/2017 3:31 AM, David Holmes wrote:

Hi Andrej,

On 30/10/2017 5:02 PM, Andrej Golovnin wrote:

Hi David,

On 30. Oct 2017, at 01:40, David Holmes 
 wrote:


Hi Roger,

On 30/10/2017 10:24 AM, Roger Riggs wrote:

Hi,
With the deprecation of Object.finalize its time to look at 
its uses too see if they can be removed or mitigated.


So the nice thing about finalize was that it followed a 
nice/clean/simple OO model where a subclass could override, 
add their own cleanup and then call super.finalize(). With 
finalize() deprecated, and the new mechanism being Cleaners, 
how do Cleaners support such usages?


Instead of ThreadPoolExecutor.finalize you can override 
ThreadPoolExecutor.terminated.


True. Though overriding shutdown() would be the semantic 
equivalent of overriding finalize(). :)


In the general case though finalize() might be invoking a final 
method.


Overriding shutdown() would only work when this method is 
explicitly invoked by user code. Using Cleaner API instead of 
finalization can not employ methods on object that is being 
tracked as that object is already gone when Cleaner invokes the 
cleanup function.


Migrating TPE to Cleaner API might be particularly painful since 
the state needed to perform the shutdown is currently in the TPE 
instance fields and would have to be moved into the cleanup 
function. The easiest way to do that is to:


- rename class ThreadPoolExecutor to package-private 
ThreadPoolExecutorImpl
- create new class ThreadPoolExecutor with same public API 
delegating the functionality to the embedded implementation
- registering Cleaner.Cleanable to perform shutdown of embedded 
executor when the public-facing executor becomes phantom reachable


This would have an added benefit of automatically shut(ing)down() 
the pool when it is not reachable any more but still has idle 
threads.




Anyway I'm not sure we can actually do something to try to move 
away from use of finalize() in TPE. finalize() is only 
deprecated - it is still expected to work as it has always 
done. Existing subclasses that override finalize() must 
continue to work until some point where we say finalize() is 
not only deprecated but obsoleted (it no longer does anything). 
So until then is there actually any point in doing anything? 
Does having a Cleaner and a finalize() method make sense? Does 
it aid in the transition?
As you observe, the alternatives directly using PhantomRefs or 
the Cleanup do not provide
as nice a model.  Unfortunately, that model has been recognized 
to have a number of
issues [1].  Finalization is a poor substitute for explicit 
shutdown, it is unpredictable a

Re: ThreadPoolExecutor and finalization

[Peter Levart]

On 11/01/17 10:04, David Holmes wrote:

On 1/11/2017 6:16 PM, Peter Levart wrote:

On 11/01/17 02:49, David Holmes wrote:

Hi Roger,

On 31/10/2017 11:58 PM, Roger Riggs wrote:

Hi Peter,

Only native resources that do not map to the heap allocation/gc 
cycle need any kind
of cleanup.  I would work toward a model that encapsulates the 
reference to a native resource
with a corresponding allocation/release mechanism as you've 
described here and in the

thread on zip.

For cleanup purposes, the independence of each resource may improve 
robustness
by avoiding dependencies and opportunities for entanglements and 
bugs due to exceptions

and other failures.

In the case of TPE, the native resources are Threads, which keep 
running even if they are

unreferenced and are kept referenced via ThreadGroups.
I don't know the Executor code well enough to do more than 
speculate, but would suggest

that a cleaner (or similar) should be registered for each thread .


Threads are not native resources to be managed by Cleaners! A live 
Thread can never be cleaned. A dead thread has nothing to clean!


Right, but an idle thread, waiting for a task that will never come 
since the only entry point for submitting tasks is not reachable (the 
pool), may be cleaned...


cleaned? It can be interrupted if you know about it and find locate 
it. But it will not be eligible for cleaning ala Cleaner as it will 
always be strongly reachable.


Ah I see what you meant before. Yes, tracking Thread object with a 
Cleaner does not have any sense. But tracking thread pool object with a 
Cleaner and cleaning (stopping) threads as a result makes sense...


Regards, Peter



David


Regards, Peter



David
-

For TPE, since Threads do not become unreferenced, the part of the 
spec related to finalize

being called by the finalizer thread is moot.

$.02, Roger

On 10/31/2017 5:24 AM, Peter Levart wrote:

Hi,

Here are some of my thoughts...

On 10/31/17 05:37, David Holmes wrote:

Hi Roger,

On 31/10/2017 12:43 AM, Roger Riggs wrote:

Hi David,

On 10/30/2017 3:31 AM, David Holmes wrote:

Hi Andrej,

On 30/10/2017 5:02 PM, Andrej Golovnin wrote:

Hi David,

On 30. Oct 2017, at 01:40, David Holmes 
 wrote:


Hi Roger,

On 30/10/2017 10:24 AM, Roger Riggs wrote:

Hi,
With the deprecation of Object.finalize its time to look at 
its uses too see if they can be removed or mitigated.


So the nice thing about finalize was that it followed a 
nice/clean/simple OO model where a subclass could override, 
add their own cleanup and then call super.finalize(). With 
finalize() deprecated, and the new mechanism being Cleaners, 
how do Cleaners support such usages?


Instead of ThreadPoolExecutor.finalize you can override 
ThreadPoolExecutor.terminated.


True. Though overriding shutdown() would be the semantic 
equivalent of overriding finalize(). :)


In the general case though finalize() might be invoking a final 
method.


Overriding shutdown() would only work when this method is 
explicitly invoked by user code. Using Cleaner API instead of 
finalization can not employ methods on object that is being 
tracked as that object is already gone when Cleaner invokes the 
cleanup function.


Migrating TPE to Cleaner API might be particularly painful since 
the state needed to perform the shutdown is currently in the TPE 
instance fields and would have to be moved into the cleanup 
function. The easiest way to do that is to:


- rename class ThreadPoolExecutor to package-private 
ThreadPoolExecutorImpl
- create new class ThreadPoolExecutor with same public API 
delegating the functionality to the embedded implementation
- registering Cleaner.Cleanable to perform shutdown of embedded 
executor when the public-facing executor becomes phantom reachable


This would have an added benefit of automatically shut(ing)down() 
the pool when it is not reachable any more but still has idle 
threads.




Anyway I'm not sure we can actually do something to try to move 
away from use of finalize() in TPE. finalize() is only 
deprecated - it is still expected to work as it has always 
done. Existing subclasses that override finalize() must 
continue to work until some point where we say finalize() is 
not only deprecated but obsoleted (it no longer does anything). 
So until then is there actually any point in doing anything? 
Does having a Cleaner and a finalize() method make sense? Does 
it aid in the transition?
As you observe, the alternatives directly using PhantomRefs or 
the Cleanup do not provide
as nice a model.  Unfortunately, that model has been recognized 
to have a number of
issues [1].  Finalization is a poor substitute for explicit 
shutdown, it is unpredictable and unreliable,

and not guaranteed to be executed.


I'm not trying to support/defend finalize(). But it does support 
the very common OO pattern of "override to specialize then call 
super".


I have been thinking about that and I see two approaches to enable 
subclasses to contribute cleanup 

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

[Peter Levart]

Hi Sherman,

On 11/01/17 00:25, Xueming Shen wrote:

Hi Peter,

After tried couple implementations it seems the most reasonable 
approach is to
use the coding pattern you suggested to move all pieces into ZSStream 
Ref. Given
we are already using the internal API CleanerFactory it is attractive 
to go a little
further to subclass PhantomCleanable directly (in which I would assume 
we can
save one extra object), but I think I'd better to follow the 
"suggested" clean usage

(to register a runnable into the cleaner) for now.

  39 class ZStreamRef implements Runnable {
  40
  41 private LongConsumer end;
  42 private volatile long address;
  43 private final Cleanable cleanable;
  44
  45 ZStreamRef (Object owner, LongSupplier addr, LongConsumer end) {
  46 this.cleanable = CleanerFactory.cleaner().register(owner, 
this);

  47 this.end = end;
  48 this.address = addr.getAsLong();
  49 }
  50

Similar change has been made for the ZipFile cleaner to follow the 
same coding

pattern. The "cleaner" is now renamed from Releaser to CleanableResource.

http://cr.openjdk.java.net/~sherman/8185582/webrev/

Thanks,
Sherman


This looks mostly fine. One minor nit is that ZStreamRef.address field 
does not have to be volatile. I checked all usages of 
ZStreamRef.address() and all of them invoke it while holding a lock on 
the ZStreamRef instance. The ZStreamRef.run() which modifies the address 
is also synchronized. The other minor nit is that the following ZipFile 
imports are not needed:


import java.nio.file.Path;
import java.util.Map;
import jdk.internal.misc.JavaIORandomAccessFileAccess;
import static java.util.zip.ZipConstants.*;

(at least my IDEA colors them unused)

Cleaner is modified in this webrev (just one empty line deleted) - 
better not touch this file in this changeset


Additional nits in ZipFile:

- in lines 355, 356 you pull out two res fields into locals, but then 
not use them consistently (lines 372, 389)
- line 403 has a TAB character (is this OK?) and shows incorrectly 
indented in my editor (should I set tab stops differently?)

- line 457 - while should actually be if ?
- in ensureOpen() the check for res.zsrc == null can never succeed 
(CleanableResource.zsrc is a final field. If CleanableResource 
constructor fails, there's no res object and there's no ZipFile object 
either as ZipFile constructor does not do anything for this to escape 
prematurely)
- why don't you let IOExceptions thrown from CleanableResource.run() 
propagate out of ZipFile.close() ?


I would also rename static method Source.close(Source) to 
Source.release(Source) so it would not clash with instance method 
Source.close() which makes it ambiguous when one wants to use 
Source::close method reference (both methods apply). I would also make 
static methods Source.get() and Source.release(Source) package-private 
(currently one is public and the other is private, which needs compiler 
bridges to be invoked) and both are in a private nested class.


Inflater/Deflater/ZipFile now follow the coding pattern as suggested. 
But ZipFileInflaterInputStream still does not. It's not critical since 
failing to register cleanup which releases the inflater back into cache 
would simply mean that Inflater employs its own cleanup and ends itself.


And now another thing I would like to discuss. Why an initiative for 
using Cleaner instead of finalization()? Among drawbacks finalization 
has one of the more troubling is that the tracked referent survives the 
GC cycle that initiates its finalization reference processing pipeline, 
so the GC may reclaim the object (and referenced objects) only after the 
finalize() has finished in yet another GC round. Cleaner API separates 
the tracked object from the cleanup function and state needed to perform 
it into distinct instances. The tracked object can be reclaimed and the 
cleanup reference processing pipeline initiated in the same GC cycle. 
More heap may be reclaimed earlier.


Unless we are careful and create a cleaning function for one tracked 
object which captures (directly or indirectly) another object which 
registers its own cleaning function but we don't deal with explicit 
cleaning of the 2nd object in the 1st cleaning function.


Take for example the ZipFileInflaterInputStream's cleaning function. It 
captures the ZipFile in order to invoke ZipFile.releaseInflater instance 
method. What this means is that ZipFile will be kept reachable until all 
ZipFileInflaterInputStream's cleaning functions have fired. So we are 
back to the finalization drawback which needs at least 2 GC cycles to 
collect and clean-up what might be done in one go.


I suggest moving the getInflater() and releaseInflater() from ZipFile 
into the CleanableResource so that ZipFileInflaterInputStream's cleaning 
function captures just the CleanableResource and not the ZipFile. 
ZipFile therefore may become eligible for cleanup as soon as all opened 
input streams beco

Re: ThreadPoolExecutor and finalization

[David Holmes]

On 1/11/2017 6:16 PM, Peter Levart wrote:

On 11/01/17 02:49, David Holmes wrote:

Hi Roger,

On 31/10/2017 11:58 PM, Roger Riggs wrote:

Hi Peter,

Only native resources that do not map to the heap allocation/gc cycle 
need any kind
of cleanup.  I would work toward a model that encapsulates the 
reference to a native resource
with a corresponding allocation/release mechanism as you've described 
here and in the

thread on zip.

For cleanup purposes, the independence of each resource may improve 
robustness
by avoiding dependencies and opportunities for entanglements and bugs 
due to exceptions

and other failures.

In the case of TPE, the native resources are Threads, which keep 
running even if they are

unreferenced and are kept referenced via ThreadGroups.
I don't know the Executor code well enough to do more than speculate, 
but would suggest

that a cleaner (or similar) should be registered for each thread .


Threads are not native resources to be managed by Cleaners! A live 
Thread can never be cleaned. A dead thread has nothing to clean!


Right, but an idle thread, waiting for a task that will never come since 
the only entry point for submitting tasks is not reachable (the pool), 
may be cleaned...


cleaned? It can be interrupted if you know about it and find locate it. 
But it will not be eligible for cleaning ala Cleaner as it will always 
be strongly reachable.


David


Regards, Peter



David
-

For TPE, since Threads do not become unreferenced, the part of the 
spec related to finalize

being called by the finalizer thread is moot.

$.02, Roger

On 10/31/2017 5:24 AM, Peter Levart wrote:

Hi,

Here are some of my thoughts...

On 10/31/17 05:37, David Holmes wrote:

Hi Roger,

On 31/10/2017 12:43 AM, Roger Riggs wrote:

Hi David,

On 10/30/2017 3:31 AM, David Holmes wrote:

Hi Andrej,

On 30/10/2017 5:02 PM, Andrej Golovnin wrote:

Hi David,

On 30. Oct 2017, at 01:40, David Holmes 
 wrote:


Hi Roger,

On 30/10/2017 10:24 AM, Roger Riggs wrote:

Hi,
With the deprecation of Object.finalize its time to look at 
its uses too see if they can be removed or mitigated.


So the nice thing about finalize was that it followed a 
nice/clean/simple OO model where a subclass could override, add 
their own cleanup and then call super.finalize(). With 
finalize() deprecated, and the new mechanism being Cleaners, 
how do Cleaners support such usages?


Instead of ThreadPoolExecutor.finalize you can override 
ThreadPoolExecutor.terminated.


True. Though overriding shutdown() would be the semantic 
equivalent of overriding finalize(). :)


In the general case though finalize() might be invoking a final 
method.


Overriding shutdown() would only work when this method is explicitly 
invoked by user code. Using Cleaner API instead of finalization can 
not employ methods on object that is being tracked as that object is 
already gone when Cleaner invokes the cleanup function.


Migrating TPE to Cleaner API might be particularly painful since the 
state needed to perform the shutdown is currently in the TPE 
instance fields and would have to be moved into the cleanup 
function. The easiest way to do that is to:


- rename class ThreadPoolExecutor to package-private 
ThreadPoolExecutorImpl
- create new class ThreadPoolExecutor with same public API 
delegating the functionality to the embedded implementation
- registering Cleaner.Cleanable to perform shutdown of embedded 
executor when the public-facing executor becomes phantom reachable


This would have an added benefit of automatically shut(ing)down() 
the pool when it is not reachable any more but still has idle threads.




Anyway I'm not sure we can actually do something to try to move 
away from use of finalize() in TPE. finalize() is only deprecated 
- it is still expected to work as it has always done. Existing 
subclasses that override finalize() must continue to work until 
some point where we say finalize() is not only deprecated but 
obsoleted (it no longer does anything). So until then is there 
actually any point in doing anything? Does having a Cleaner and a 
finalize() method make sense? Does it aid in the transition?
As you observe, the alternatives directly using PhantomRefs or the 
Cleanup do not provide
as nice a model.  Unfortunately, that model has been recognized to 
have a number of
issues [1].  Finalization is a poor substitute for explicit 
shutdown, it is unpredictable and unreliable,

and not guaranteed to be executed.


I'm not trying to support/defend finalize(). But it does support 
the very common OO pattern of "override to specialize then call 
super".


I have been thinking about that and I see two approaches to enable 
subclasses to contribute cleanup code:


- one simple approach is for each subclass to use it's own 
independent Cleaner/Cleanable. The benefit is that each subclass is 
independent of its super/sub classes, the drawback is that cleanup 
functions are called in arbitrary order, even in different threads. 

Re: ThreadPoolExecutor and finalization

[Peter Levart]

On 11/01/17 02:49, David Holmes wrote:

Hi Roger,

On 31/10/2017 11:58 PM, Roger Riggs wrote:

Hi Peter,

Only native resources that do not map to the heap allocation/gc cycle 
need any kind
of cleanup.  I would work toward a model that encapsulates the 
reference to a native resource
with a corresponding allocation/release mechanism as you've described 
here and in the

thread on zip.

For cleanup purposes, the independence of each resource may improve 
robustness
by avoiding dependencies and opportunities for entanglements and bugs 
due to exceptions

and other failures.

In the case of TPE, the native resources are Threads, which keep 
running even if they are

unreferenced and are kept referenced via ThreadGroups.
I don't know the Executor code well enough to do more than speculate, 
but would suggest

that a cleaner (or similar) should be registered for each thread .


Threads are not native resources to be managed by Cleaners! A live 
Thread can never be cleaned. A dead thread has nothing to clean!


Right, but an idle thread, waiting for a task that will never come since 
the only entry point for submitting tasks is not reachable (the pool), 
may be cleaned...


Regards, Peter



David
-

For TPE, since Threads do not become unreferenced, the part of the 
spec related to finalize

being called by the finalizer thread is moot.

$.02, Roger

On 10/31/2017 5:24 AM, Peter Levart wrote:

Hi,

Here are some of my thoughts...

On 10/31/17 05:37, David Holmes wrote:

Hi Roger,

On 31/10/2017 12:43 AM, Roger Riggs wrote:

Hi David,

On 10/30/2017 3:31 AM, David Holmes wrote:

Hi Andrej,

On 30/10/2017 5:02 PM, Andrej Golovnin wrote:

Hi David,

On 30. Oct 2017, at 01:40, David Holmes 
 wrote:


Hi Roger,

On 30/10/2017 10:24 AM, Roger Riggs wrote:

Hi,
With the deprecation of Object.finalize its time to look at 
its uses too see if they can be removed or mitigated.


So the nice thing about finalize was that it followed a 
nice/clean/simple OO model where a subclass could override, add 
their own cleanup and then call super.finalize(). With 
finalize() deprecated, and the new mechanism being Cleaners, 
how do Cleaners support such usages?


Instead of ThreadPoolExecutor.finalize you can override 
ThreadPoolExecutor.terminated.


True. Though overriding shutdown() would be the semantic 
equivalent of overriding finalize(). :)


In the general case though finalize() might be invoking a final 
method.


Overriding shutdown() would only work when this method is explicitly 
invoked by user code. Using Cleaner API instead of finalization can 
not employ methods on object that is being tracked as that object is 
already gone when Cleaner invokes the cleanup function.


Migrating TPE to Cleaner API might be particularly painful since the 
state needed to perform the shutdown is currently in the TPE 
instance fields and would have to be moved into the cleanup 
function. The easiest way to do that is to:


- rename class ThreadPoolExecutor to package-private 
ThreadPoolExecutorImpl
- create new class ThreadPoolExecutor with same public API 
delegating the functionality to the embedded implementation
- registering Cleaner.Cleanable to perform shutdown of embedded 
executor when the public-facing executor becomes phantom reachable


This would have an added benefit of automatically shut(ing)down() 
the pool when it is not reachable any more but still has idle threads.




Anyway I'm not sure we can actually do something to try to move 
away from use of finalize() in TPE. finalize() is only deprecated 
- it is still expected to work as it has always done. Existing 
subclasses that override finalize() must continue to work until 
some point where we say finalize() is not only deprecated but 
obsoleted (it no longer does anything). So until then is there 
actually any point in doing anything? Does having a Cleaner and a 
finalize() method make sense? Does it aid in the transition?
As you observe, the alternatives directly using PhantomRefs or the 
Cleanup do not provide
as nice a model.  Unfortunately, that model has been recognized to 
have a number of
issues [1].  Finalization is a poor substitute for explicit 
shutdown, it is unpredictable and unreliable,

and not guaranteed to be executed.


I'm not trying to support/defend finalize(). But it does support 
the very common OO pattern of "override to specialize then call 
super".


I have been thinking about that and I see two approaches to enable 
subclasses to contribute cleanup code:


- one simple approach is for each subclass to use it's own 
independent Cleaner/Cleanable. The benefit is that each subclass is 
independent of its super/sub classes, the drawback is that cleanup 
functions are called in arbitrary order, even in different threads. 
But for cleaning-up independent resources this should work.


- the other approach is more involving as it requires the base class 
to establish an infrastructure for contributing cleanup code. For 
this purpose, the i