Re: BiCollector

[Kirk Pepperdine]

> On Jun 19, 2018, at 9:11 AM, Zheka Kozlov  wrote:
> 
> The function you propose is just a binary variant of mapping:
> 
> Collector mapping(
>  Function mapper,
>  Collector downstream);
> 
> (omitted '? super' for readability)
> 
> So, it is logical to use the name biMapping:
> 
> Collector biMapping(
>  Function mapper1,
>  Function mapper2,
>  Collector downstream1,
>  Collector downstream2,
>  BiFunction finisher);

+1


> 
> 
> 2018-06-19 7:38 GMT+07:00 John Rose :
> 
>> On Jun 18, 2018, at 2:29 PM, Brian Goetz  wrote:
>>> 
>>> "bisecting" sounds like it sends half the elements to one collector and
>> half to the other …
>> 
>> The main bisection or splitting operation that's relevant to a stream is
>> what
>> a spliterator does, so this is a concern.
>> 
>> Nobody has mentioned "unzipping" yet; this is a term of art which applies
>> to streams
>> of tuples.  The image of a zipper is relatively clear and unambiguous, and
>> the tradition
>> is pretty strong.  https://en.wikipedia.org/wiki/
>> Convolution_(computer_science)
>> 
>> The thing we are looking at differs in two ways from classic "unzipping":
>> First, the
>> two collectors themselves convert the same T elements to whatever internal
>> value
>> (T1, T2) is relevant.  Second, we are looking at a new terminal operation
>> (a collector) which
>> consolidates the results from both of streams (a notional Stream and
>> Stream,
>> if you like), rather than delivering the streams as a pair of outputs.
>> 
>> The classic "unzip" operation applies "fst" and "snd" (or some other
>> conventional
>> set of access functions) to each T-element of the input stream.  Since we
>> don't
>> have a privileged 2-tuple type (like Pair) in Java, the user would
>> need
>> to nominate those two functions explicitly, either by folding them into a
>> "mapping"
>> on each collector, or as a utility overloading like this:
>> 
>>   unzipping(
>>Function f1,  // defaults to identity
>>Collector c1,
>>Function f2,  // defaults to identity
>>Collector c2,
>>BiFunction finisher) {
>> return toBoth(mapping(f1, c1), mapping(f2, c2));
>>  }
>> 
>> 
>>> "tee" might be a candidate, though it doesn't follow the `ing
>> convention.  "teeing" sounds dumb.
>> 
>> 
>> "tee" sounds asymmetrical.  "diverting" or "detouring" are "*ing" words
>> that might
>> express asymmetrical disposition of derivative streams.
>> 
>> An asymmetrical operation might be interesting if it could fork off a
>> stream of
>> its own.  It would have to have a side-effecting void-producing terminal
>> operation,
>> so the main (undiverted) stream could continue to progress at the top
>> level of
>> the expression.
>> 
>> interface Stream {
>>  default Stream diverting(Consumer> tee) { … }
>> }
>> 
>> values.stream().diverting(s2->s2.forEach(System.out::
>> println)).filter(…).collect(…);
>> 
>> Or (and this might be a sweet spot) a symmetric stream-tee operation could
>> materialize two sibling streams and rejoin their results with a bifunction:
>> 
>> class Collectors {
>>  static  Stream unzipping(
>>Function, R1> f1,
>>Function, R2> f2,
>>BiFunction finisher)
>> { … }
>> }
>> 
>> values.stream().unzipping(
>>s1->s1.forEach(System.out::println),
>>s2->s2.filter(…).collect(…),
>>(void1, r2)->r2
>>);
>> 
>> This would allow each "fork child" of the stream to continue to use the
>> Stream API instead of the more restrictive Collector operators.
>> 
>> Optimal code generation for forked/unzipped/teed streams would be tricky,
>> requiring simultaneous loop control logic for each stream.
>> To me that's a feature, not a bug, since hand-writing ad hoc
>> simultaneous loops is a pain.
>> 
>> My $0.02.
>> 
>> — John

Re: BiCollector

[Zheka Kozlov]

The function you propose is just a binary variant of mapping:

Collector mapping(
  Function mapper,
  Collector downstream);

(omitted '? super' for readability)

So, it is logical to use the name biMapping:

Collector biMapping(
  Function mapper1,
  Function mapper2,
  Collector downstream1,
  Collector downstream2,
  BiFunction finisher);


2018-06-19 7:38 GMT+07:00 John Rose :

> On Jun 18, 2018, at 2:29 PM, Brian Goetz  wrote:
> >
> > "bisecting" sounds like it sends half the elements to one collector and
> half to the other …
>
> The main bisection or splitting operation that's relevant to a stream is
> what
> a spliterator does, so this is a concern.
>
> Nobody has mentioned "unzipping" yet; this is a term of art which applies
> to streams
> of tuples.  The image of a zipper is relatively clear and unambiguous, and
> the tradition
> is pretty strong.  https://en.wikipedia.org/wiki/
> Convolution_(computer_science)
>
> The thing we are looking at differs in two ways from classic "unzipping":
> First, the
> two collectors themselves convert the same T elements to whatever internal
> value
> (T1, T2) is relevant.  Second, we are looking at a new terminal operation
> (a collector) which
> consolidates the results from both of streams (a notional Stream and
> Stream,
> if you like), rather than delivering the streams as a pair of outputs.
>
> The classic "unzip" operation applies "fst" and "snd" (or some other
> conventional
> set of access functions) to each T-element of the input stream.  Since we
> don't
> have a privileged 2-tuple type (like Pair) in Java, the user would
> need
> to nominate those two functions explicitly, either by folding them into a
> "mapping"
> on each collector, or as a utility overloading like this:
>
>unzipping(
> Function f1,  // defaults to identity
> Collector c1,
> Function f2,  // defaults to identity
> Collector c2,
> BiFunction finisher) {
>  return toBoth(mapping(f1, c1), mapping(f2, c2));
>   }
>
>
> > "tee" might be a candidate, though it doesn't follow the `ing
> convention.  "teeing" sounds dumb.
>
>
> "tee" sounds asymmetrical.  "diverting" or "detouring" are "*ing" words
> that might
> express asymmetrical disposition of derivative streams.
>
> An asymmetrical operation might be interesting if it could fork off a
> stream of
> its own.  It would have to have a side-effecting void-producing terminal
> operation,
> so the main (undiverted) stream could continue to progress at the top
> level of
> the expression.
>
> interface Stream {
>   default Stream diverting(Consumer> tee) { … }
> }
>
> values.stream().diverting(s2->s2.forEach(System.out::
> println)).filter(…).collect(…);
>
> Or (and this might be a sweet spot) a symmetric stream-tee operation could
> materialize two sibling streams and rejoin their results with a bifunction:
>
> class Collectors {
>   static  Stream unzipping(
> Function, R1> f1,
> Function, R2> f2,
> BiFunction finisher)
> { … }
> }
>
> values.stream().unzipping(
> s1->s1.forEach(System.out::println),
> s2->s2.filter(…).collect(…),
> (void1, r2)->r2
> );
>
> This would allow each "fork child" of the stream to continue to use the
> Stream API instead of the more restrictive Collector operators.
>
> Optimal code generation for forked/unzipped/teed streams would be tricky,
> requiring simultaneous loop control logic for each stream.
> To me that's a feature, not a bug, since hand-writing ad hoc
> simultaneous loops is a pain.
>
> My $0.02.
>
> — John

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Martin Buchholz]

On Mon, Jun 18, 2018 at 10:19 PM, David Holmes 
wrote:

>
> I'm unsure why we're trying to make this particular class
> unsafe-publication-proof, but I guess it's generally a good thing.


The particular field we're changing is a """final""" field, and it would be
pretty embarrassing to grab a stale value of that -  the wrong lock!

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[David Holmes]

On 19/06/2018 3:08 PM, Martin Buchholz wrote:

Latest version looks like this:

      public Object clone() {
          try {
              @SuppressWarnings("unchecked")
              CopyOnWriteArrayList clone =
                  (CopyOnWriteArrayList) super.clone();
              clone.resetLock();
+            // Unlike in readObject, here we cannot 
visibility-piggyback on the

+            // volatile write in setArray().
+            VarHandle.releaseFence();
              return clone;
          } catch (CloneNotSupportedException e) {
              // this shouldn't happen, since we are Cloneable
              throw new InternalError();
          }
      }

The store we worry about is a hypothetical subsequent racy publication 
of the new object, so we put the release fence at the end of the 
pseudo-constructor.


Thanks for clarifying.

I'm unsure why we're trying to make this particular class 
unsafe-publication-proof, but I guess it's generally a good thing.


David



On Mon, Jun 18, 2018 at 9:01 PM, David Holmes > wrote:


On 19/06/2018 6:01 AM, Doug Lea wrote:

Hang on! A releasing store does the "release" before the store not
after it. The whole point being if you see the result of the store
then you are guaranteed to see all previous stores. The above can
reorder the lockField store with whatever stores come before it.

David
-

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Martin Buchholz]

Latest version looks like this:

 public Object clone() {
 try {
 @SuppressWarnings("unchecked")
 CopyOnWriteArrayList clone =
 (CopyOnWriteArrayList) super.clone();
 clone.resetLock();
+// Unlike in readObject, here we cannot visibility-piggyback
on the
+// volatile write in setArray().
+VarHandle.releaseFence();
 return clone;
 } catch (CloneNotSupportedException e) {
 // this shouldn't happen, since we are Cloneable
 throw new InternalError();
 }
 }

The store we worry about is a hypothetical subsequent racy publication of
the new object, so we put the release fence at the end of the
pseudo-constructor.


On Mon, Jun 18, 2018 at 9:01 PM, David Holmes 
wrote:

> On 19/06/2018 6:01 AM, Doug Lea wrote:
>
> Hang on! A releasing store does the "release" before the store not after
> it. The whole point being if you see the result of the store then you are
> guaranteed to see all previous stores. The above can reorder the lockField
> store with whatever stores come before it.
>
> David
> -
>
>

Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[David Holmes]

Discussions on the CSR request have led to further changes to the 
documentation involving nests and the new nest-related method. There are 
no semantic changes here just clearer explanations.


Incremental webrev: 
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v7-incr/


(don't worry if you don't see a v6, it didn't really exist).

Full webrev: 
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v7/


Specdiffs updated in place at: 
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/specs/


Summary of changes:

- The definition of a nest etc is moved to the class-level javadoc of 
java.lang.Class, along with some other edits provided by Alex Buckley to 
pave the way for future updates

- The nest-related methods are written in a more clear and consistent way

Thanks,
David
-

On 12/06/2018 3:16 PM, David Holmes wrote:

Here is one further minor update from the CSR discussions:

http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v5-incr/src/java.base/share/classes/java/lang/Class.java.cdiff.html 



Thanks,
David

On 25/05/2018 3:52 PM, David Holmes wrote:
Here are the further minor updates so far in response to all the 
review comments.


Incremental corelibs webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/ 



Full corelibs webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/

Change summary:

src/java.base/share/classes/jdk/internal/reflect/Reflection.java
- remove inaccurate pseudo-assertion comment

test/jdk/java/lang/reflect/Nestmates/SampleNest.java
- code cleanup: <> operator

test/jdk/java/lang/reflect/Nestmates/TestReflectionAPI.java
- code cleanup: streamify duplicate removals

test/jdk/java/lang/invoke/PrivateInterfaceCall.java
- use consistent @bug number

Thanks,
David

On 22/05/2018 8:15 PM, David Holmes wrote:

Here are the updates so far in response to all the review comments.

Incremental webrev: 
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2-incr/ 



Full webrev: 
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2/


Change summary:

src/java.base/share/classes/java/lang/Class.java
- getNesthost:
   - change "any error" -> "any linkage error" as runtime errors will 
propagate. [This needs ratifying by EG]
   - add clarification that primitive and array classes are not 
explicitly members of any nest and so form singleton nests

   - add clarification that all nestmates are in the same package
   - re-word @return text to exclude the "royal 'we'"
- fix javadoc cross references

---

Moved reflection API tests from 
test/hotspot/jtreg/runtime/Nestmates/reflectionAPI/ to 
test/jdk/java/lang/reflect/Nestmates/


---

java/lang/reflect/Nestmates/TestReflectionAPI.java

Run tests twice to show that failure reasons remain the same.

---

test/jdk/jdk/lambda/vm/InterfaceAccessFlagsTest.java

Disable test via annotation rather than commenting out.

---

src/java.base/share/classes/jdk/internal/reflect/Reflection.java

- Fix indent for nestmate access check.
- Remove unnecessary local variable

---

src/java.base/share/classes/sun/invoke/util/VerifyAccess.java

- Replace myassert with proper assert

---

Thanks,
David

On 15/05/2018 10:52 AM, David Holmes wrote:
This review is being spread across four groups: langtools, 
core-libs, hotspot and serviceability. This is the specific review 
thread for core-libs - webrev:


http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/

See below for full details - including annotated full webrev guiding 
the review.


The intent is to have JEP-181 targeted and integrated by the end of 
this month.


Thanks,
David
-

The nestmates project (JEP-181) introduces new classfile attributes 
to identify classes and interfaces in the same nest, so that the VM 
can perform access control based on those attributes and so allow 
direct private access between nestmates without requiring javac to 
generate synthetic accessor methods. These access control changes 
also extend to core reflection and the MethodHandle.Lookup contexts.


Direct private calls between nestmates requires a more general 
calling context than is permitted by invokespecial, and so the JVMS 
is updated to allow, and javac updated to use, invokevirtual and 
invokeinterface for private class and interface method calls 
respectively. These changed semantics also extend to MethodHandle 
findXXX operations.


At this time we are only concerned with static nest definitions, 
which map to a top-level class/interface as the nest-host and all 
its nested types as nest-members.


Please see the JEP for further details.

JEP: https://bugs.openjdk.java.net/browse/JDK-8046171
Bug: https://bugs.openjdk.java.net/browse/JDK-8010319
CSR: https://bugs.openjdk.java.net/browse/JDK-8197445

All of the specification changes have been previously been worked 
out by the Valhalla Project Expert Group, and the implementation 
reviewed by the various 

Re: BiCollector

[Kirk Pepperdine]

> On Jun 19, 2018, at 1:21 AM, Brian Goetz  wrote:
> 
> distributing?
> 
> On 6/18/2018 6:04 PM, Chris Hegarty wrote:
>> 
>>> On 18 Jun 2018, at 22:29, Brian Goetz  wrote:
>>> 
>>> "bisecting" sounds like it sends half the elements to one collector and 
>>> half to the other ...
>>> 
>>> "tee" might be a candidate, though it doesn't follow the `ing convention.  
>>> "teeing" sounds dumb.

But teeing (https://en.wikipedia.org/wiki/Tee_(command) 
) is sort of what you’re doing so 
it sounds just fine. .. I’ve always thought it like tapping but this maybe less 
descriptive than split which of course would be confused with Spliterator.

— Kirk

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[David Holmes]

On 19/06/2018 6:01 AM, Doug Lea wrote:

On 06/18/2018 11:22 AM, Martin Buchholz wrote:


 Or better, lockField.set in resetLock could instead be setRelease.
 Except it is using reflection, not VarHandles. So it could be preceded
 with VarHandle.releaseFence(), although it is hard to think of a
 scenario in which it could matter.


You mean followed by, not preceded by?

           try {
              lockField.set(this, new Object());
+            java.lang.invoke.VarHandle.releaseFence();
          } catch (IllegalAccessException e) {
              throw new Error(e);
          }



OK. Followed by is a little better in that it orders any field write
before any write of this, not just the array copy before lock field write.


Hang on! A releasing store does the "release" before the store not after 
it. The whole point being if you see the result of the store then you 
are guaranteed to see all previous stores. The above can reorder the 
lockField store with whatever stores come before it.


David
-


-Doug

Re: Proposal: optimization of Map.copyOf and Collectors.toUnmodifiableMap

[Stuart Marks]

On 6/17/18 4:23 PM, Peter Levart wrote:
My attempt to optimize the Map.copyOf() was also using just public APIs (with 
the addition of an internal class).


Well, it does speed-up Map.copyOf() by 30%. But I agree there are other 
approaches that could go even further. In particular when all intermediary 
copies could be avoided.


Here's one approach (which still uses just public APIs) and avoids intermediary 
copying:


You mentioned "using just public APIs" a couple times. Are you viewing that as a 
constraint? I don't think it is.



http://cr.openjdk.java.net/~plevart/jdk-dev/UnmodifiableMap_copyOf/webrev.02/

Why choosing Map.forEach for dumping the content of the map instead of iteration 
over entrySet? Because in some Map implementations this is the most direct 
iteration without producing garbage (for example in IdentityHashMap), but in the 
worst case (default Map method for example) it is delegated to iteration over 
entrySet.


Sure, entrySet().toArray() does a lot of allocation and copying. Using forEach() 
can avoid this, but it doesn't necessarily preserve the right invariants. In 
particular, if the source is a concurrent map, the number of times forEach's 
BiConsumer is called might not match size(), if the map has changed size. So, 
the forEach approach has to deal with the possibility of it not being called 
exactly size() times, whereas we know that the array from toArray() can't change 
size (and that its contents are consistent, for some definition of consistent).


This change also publishes a reference to the Map under construction before its 
constructor has returned. I'm not sure of all the memory model implications of this.


This change also hands out an object that has access to the new Map instance's 
internals. You're aware of this, of course, because you snapshot the current 
thread and null it out later, saying


// prevent naughty Map implementations from modifying MapN after
// it is fully constructed

So there are potential security vulnerabilities here, which requires some 
serious thought. What you have *seems* reasonable, but my experience is that 
things that are subtle but that seem reasonable actually end up having security 
holes.


I'm trying to think of some alternatives.

The issue with the forEach() approach on an arbitrary map is that you have to 
hand out a BiConsumer, and malicious code can call it even after forEach() has 
returned. What's necessary is to have a way to shut off the BiConsumer before 
reading out what it's accumulated. I don't know of a simple, fast, and correct 
way to do this. (Choose any two!) The "enabled" state (whether a Thread instance 
or just a boolean) will probably have to be a volatile that must be checked 
every time. What are the performance implications? Anyway, while it seems 
promising, the forEach() approach seems like it leads off into the weeds.


Unless you can verify the trustworthiness of the source. Suppose you check the 
source map to see if its getClass() == HashMap.class. Then you can be reasonably 
assured that forEach() won't misuse the BiConsumer. You can probably also rely 
on size() being stable. (Probably also include LinkedHashMap.)


The toUnmodifiableMap collectors can benefit from this if they're changed to use 
Map::copyOf, as in your current webrev.


You could also provide a similar fast path for ConcurrentHashMap. It'd have to 
be resilient against size changes, but you can trust that it won't misuse the 
BiConsumer.


Finally, other map implementations will just have to suffer through the 
multi-copy slow path.


s'marks

RFR: 8203629: Produce events in the JDK without a dependency on jdk.jfr

[Erik Gahlin]

Hi,

Could I have a review of an enhancement that will make it possible to 
add JFR events to java.base, and possibly other modules in the JDK, 
without a compile time dependency on jdk.jfr.


Bug:
https://bugs.openjdk.java.net/browse/JDK-8203629

Webrev:
http://cr.openjdk.java.net/~egahlin/8203629.0

Testing:
Tests in test/jdk/jdk/jfr

The functionality is a prerequisite for an upcoming enhancement that 
will add JFR events to the security libraries.


In short, jdk.jfr.Event (located in jdk.jfr) extends 
jdk.internal.event.Event (located in java.base). Metadata for events, 
such as labels and descriptions, are added using a mirror event class 
located in jdk.jfr module. If the fields of the mirror class and the 
event class don't match an InternalError is thrown.


To illustrate how the mechanism can be used, see the following example 
(not meant to be checked in).


http://cr.openjdk.java.net/~egahlin/8203629.example

Since the implementation of jdk.internal.event.Event is empty, the JIT 
will typically eliminate all traces of JFR functionality unless Flight 
Recorder is enabled.


Thanks
Erik

Re: RFR 8198669: Refactor annotation array value parsing to reduce duplication

[Joseph D. Darcy]

Looks good Liam; thanks,

-Joe

On 6/15/2018 10:58 AM, Liam Miller-Cushon wrote:

Hello,

This change is a follow-up to JDK-7183985 which replaces the interior of
parseClassArray, parseEnumArray, and parseAnnotationArray with a shared
method that is passed a lambda for the type-specific parsing logic.

bug: https://bugs.openjdk.java.net/browse/JDK-8198669
webrev: http://cr.openjdk.java.net/~cushon/8198669/webrev.00/

Liam

Re: Proposal: optimization of Map.copyOf and Collectors.toUnmodifiableMap

[Stuart Marks]

On 6/18/18 7:25 AM, Roger Riggs wrote:
In regard to new SharedSecret interfaces, one option is move shared (but 
private) implementation classes
to a jdk.internal.xx package (not exported).  This only works well if they are 
not tightly coupled to other

package private classes.

SpinedBuffer might be a good candidate, I have some IO cases in mind that could 
benefit from

the allocation/reallocation savings.  (ByteArrayOutputStream for 1).


Yes, SpinedBuffer is a good candidate for this. There's nothing special about it 
that ties it to streams. It was just put into java.util.stream because streams 
were its initial (and currently only) users.


A SharedSecret would be helpful for things like new private factory methods for 
the unmodifiable collections, such as ones that might assume ownership of an 
array instead of copying it.


s'marks

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

[Stuart Marks]

On 6/17/18 1:50 AM, Peter Levart wrote:

It's a little strange that the generator function is used to construct an
empty array (at least in the default method, but overrides will likely do the
same if they want to avoid pre-zeroing overhead) in order to just extract the
array's type from it. Someone could reasonably expect the provided function
to be called with the exact length of needed array. The
Collection.toArray(T[]) at least gives user an option to actually fully use
the provided array, if user provides the correct length.


This is actually what we're trying to avoid. The toArray(T[]) API has to deal
with the cases where the array isn't the right length, and it reallocs or
inserts an extra null in cases where it isn't. To avoid this, people do

coll.toArray(new MyClass[coll.size()])

which turns out to be an anti-pattern. We could try to teach people to write

coll.toArray(new MyClass[0])

instead, and this works, but it's quite non-obvious. ("Why do I need to create a
zero-length array first?") (#include Tagir's comment about caching zero-length
instances) Instead we want to direct people to write

coll.toArray(MyClass[]::new)

which creates an array of the right type and requested length.


The argument about using (and re-using) a method so that a method reference
can be passed to the method without any unchecked casts is equally true for
any of the 3 alternatives shown - the method itself might need unchecked
casts, but its usage not:

static List[] array(int len) static Class>
elementType() static Class[]> arrayType()


In principle all of these are possible. I don't see these as equal, though. It's 
quite common to have to create arrays of a generic type, either inline with 
unchecked casts, or possibly refactored into a method. I very rarely see Class 
objects of generic types.



But I can see that you want to align the API with Stream.toArray, while still
 providing the optimal implementation. It's just that the API doesn't fit the
 usecase. The function approach makes more sense in the Stream API where it
is explicitly explained that it may be used to construct arrays needed to
hold intermediary results of partitioned parallel execution too, but in
Collection API it is usually the case to just provide a copy of the
underlying data structure in the most optimal way (without pre-zeroing
overhead) and for that purpose, 2nd and 3rd alternatives are a better fit.


Sure, the Stream API has additional uses for holding intermediate results. That
doesn't imply that Collection.toArray(generator) doesn't meet its use case
(which I described above).

I also don't see how class type tokens are a better fit. A type token is
"natural" if you're thinking of implementing it in terms of Arrays.copyOf() --
which it is right now, but that's an implementation detail.


Suppose Stream took a different approach and used the 2nd or 3rd approach
(which is universal). Would then Collection API also get the same method?


I'm not sure where this is headed. I'm pretty sure we considered using type
tokens for Stream.toArray() and rejected them in favor of toArray(generator). If
there had been some reason to use a type token instead, maybe we would have used
them, in which case we'd consider modifying Collection.toArray() would take a 
type token as well. But I'm not aware of such a reason, so



It might have been the case in the past when Java generics were introduced,
that class literals like List.class  would just confuse users,
because most aspects of such type token would be erased and there were fears
that enabling them might limit options for reified generics in the future.
But long years have passed and java programmers have generally become
acquainted with Java generics and erasure to the point where it doesn't
confuse them any more, while reifying Java generics has been explored further
in Valhalla to the point where it has become obvious that erasure of
reference types is here to stay.

Java could enable use of class literals like List.class without fear
that such literals would make users write buggy code or that such uses would
limit options for Java in the future. Quite the contrary, they would enable
users to write type-safer code than what they can write today. In the light
of future Java  (valhalla specialized generics), where such class literals
make even more sense, enabling generic class literals could be viewed as a
stepping stone that has some purpose in the short term too.

Again, I'm not sure where this is headed.

I certainly would not propose changing the language to allow generic class 
literals in order to support the present use case (creating an array from a 
collection).


As a longer term proposal, generic class literals might or might not be 
worthwhile on their own merits. It's certainly an irritant not having them; I 
could imagine some uses for them. But there are probably some downsides (such as 
unsoundness) that will need to be thought through.


s'marks

Re: BiCollector

[John Rose]

On Jun 18, 2018, at 2:29 PM, Brian Goetz  wrote:
> 
> "bisecting" sounds like it sends half the elements to one collector and half 
> to the other …

The main bisection or splitting operation that's relevant to a stream is what
a spliterator does, so this is a concern.

Nobody has mentioned "unzipping" yet; this is a term of art which applies to 
streams
of tuples.  The image of a zipper is relatively clear and unambiguous, and the 
tradition
is pretty strong.  https://en.wikipedia.org/wiki/Convolution_(computer_science)

The thing we are looking at differs in two ways from classic "unzipping":  
First, the
two collectors themselves convert the same T elements to whatever internal value
(T1, T2) is relevant.  Second, we are looking at a new terminal operation (a 
collector) which
consolidates the results from both of streams (a notional Stream and 
Stream,
if you like), rather than delivering the streams as a pair of outputs.

The classic "unzip" operation applies "fst" and "snd" (or some other 
conventional
set of access functions) to each T-element of the input stream.  Since we don't
have a privileged 2-tuple type (like Pair) in Java, the user would need
to nominate those two functions explicitly, either by folding them into a 
"mapping"
on each collector, or as a utility overloading like this:

   unzipping(
Function f1,  // defaults to identity
Collector c1,
Function f2,  // defaults to identity
Collector c2,
BiFunction finisher) {
 return toBoth(mapping(f1, c1), mapping(f2, c2));
  }


> "tee" might be a candidate, though it doesn't follow the `ing convention.  
> "teeing" sounds dumb.


"tee" sounds asymmetrical.  "diverting" or "detouring" are "*ing" words that 
might
express asymmetrical disposition of derivative streams.

An asymmetrical operation might be interesting if it could fork off a stream of
its own.  It would have to have a side-effecting void-producing terminal 
operation,
so the main (undiverted) stream could continue to progress at the top level of
the expression.

interface Stream {
  default Stream diverting(Consumer> tee) { … }
}

values.stream().diverting(s2->s2.forEach(System.out::println)).filter(…).collect(…);

Or (and this might be a sweet spot) a symmetric stream-tee operation could
materialize two sibling streams and rejoin their results with a bifunction:

class Collectors {
  static  Stream unzipping(
Function, R1> f1,
Function, R2> f2,
BiFunction finisher) { … }
}

values.stream().unzipping(
s1->s1.forEach(System.out::println),
s2->s2.filter(…).collect(…),
(void1, r2)->r2
);

This would allow each "fork child" of the stream to continue to use the
Stream API instead of the more restrictive Collector operators.

Optimal code generation for forked/unzipped/teed streams would be tricky,
requiring simultaneous loop control logic for each stream.
To me that's a feature, not a bug, since hand-writing ad hoc
simultaneous loops is a pain.

My $0.02.

— John

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

[Brian Goetz]

+1 from me.

> On Jun 14, 2018, at 9:02 PM, Stuart Marks  wrote:
> 
> Hi all,
> 
> I wanted to restart review of the changeset for bug JDK-8060192 [1]. The 
> latest webrev is here: [2].
> 
> The previous review was from December 2017: [3]. The only difference between 
> the current changeset and the previous one is the removal of the overriding 
> implementations (see explanation below). Much of the discussion in the 
> previous review thread was around performance and the shape of the API.
> 
> # Performance
> 
> I previously had done some benchmarking on this and reported the results: 
> [4]. (I've recently re-done the benchmarks and conclusions are essentially 
> the same.)
> 
> The upshot is that implementations that use Arrays.copyOf() are the fastest, 
> probably because it's an intrinsic. It can avoid zero-filling the freshly 
> allocated array because it knows the entire array contents will be 
> overwritten. This is true regardless of what the public API looks like. The 
> default implementation calls generator.apply(0) to get a zero-length array 
> and then simply calls toArray(T[]). This goes through the Arrays.copyOf() 
> fast path, so it's essentially the same speed as toArray(new T[0]).
> 
> The overrides I had previously provided in specific implementation classes 
> like ArrayList actually are slower, because the allocation of the array is 
> done separately from filling it. This necessitates the extra zero-filling 
> step. Thus, I've removed the overrides.
> 
> # API Shape
> 
> There was some discussion about whether it would be preferable to have a 
> Class parameter as a type token for the array component type or the array 
> type itself, instead of using an IntFunction generator. Most of this boils 
> down to what people are comfortable with. However, there are a few points 
> that make the generator function approach preferable.
> 
> One point in favor of using a generator is that Stream already has a similar 
> toArray(generator) method.
> 
> Comparing this to the type token alternatives, for simple tasks like 
> converting Collection to String[], things are about equal:
> 
>toArray(String[]::new)
>toArray(String.class)
>toArray(String[].class)
> 
> However, things are hairier if the element type of the collection is generic, 
> such as Collection>. The result type is a generic array 
> List[]. Using class literals or array constructor references will 
> necessitate using an unchecked cast, because none of these can be used to 
> represent a generic type.
> 
> However, it's possible to use a method reference to provide a properly 
> generic IntFunction. This would enable the toArray(generator) method to be 
> used without any unchecked casts. (The method it refers to might need have an 
> unchecked cast within it, though.) Such a method could also be reused for the 
> corresponding Stream.toArray(generator) method.
> 
> For these reasons I'd like to proceed with adding toArray(generator) API.
> 
> Thanks,
> 
> s'marks
> 
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8060192
> 
> [2] http://cr.openjdk.java.net/~smarks/reviews/8060192/webrev.3/
> 
> [3] 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050325.html
> 
> [4] 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050585.html
> 

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

[Stuart Marks]

Tagir Valeev wrote:


If you are going to create long-living array which is likely to be empty,
it's good to deduplicate them to spare the heap space. This can be easily
done via existing toArray method like
collection.toArray(MyClass.EMPTY_ARRAY) assuming we have an empty array
constant. We use this pattern very often in IntelliJ IDEA source code (e.
g. think of method which returns an array of Java member annotations; often
there are none). New generator methods is much less convenient for this:
toArray(s -> s == 0?MyClass.EMPTY_ARRAY:new MyClass[s]). I'm actually
missing a method accepting an empty array instead of generator in the
Stream API. And I don't think that new collection method is useful. The
existing one is perfect.


Rémi Forax wrote:


we can expect the VM to not allocate the array if once inlined the code is
   new MyClass[0].getClass()

if it's not the case, i think it's a better idea to tweak the VM rather than 
try to change an API based on a pattern that should not be necessary.


I think the two of you are talking about different things. Tagir is concerned 
about *long-lived* zero-length arrays, whereas Rémi is talking about the 
possibility of short-circuiting the allocation of a zero-length array if it's 
replaced by a nonzero-length array and thus has an extremely short life.


Tagir, if your use case is that you know you are creating lots of long-lived 
zero-length arrays, and you want to deduplicate them, then sure, using 
toArray(MyClass.EMPTY_ARRAY) is a fine thing to do. There are a bunch of 
assumptions here about the longevity and frequency of creation of such arrays. 
Having a shared empty array might indeed be the right thing for the IntelliJ 
IDEA code base, but that doesn't mean it's true in general. The 
toArray(generator) might be perfectly fine for many code bases even if it isn't 
suitable for the one you have in mind.


You also mention the lack of a  T[] Stream.toArray(T[]) method. This would 
seem to help the case of sharing a zero-length array, but 
Collection.toArray(T[]) also has odd semantics that we didn't want to replicate 
in streams. The point of that method is to *reuse* the argument array. The odd 
semantics are that, when the argument array is longer than necessary, null is 
added after the last written element. Instead of replicating the semantics of 
Collection.toArray(T[]) on Stream, we ended up with Stream.toArray(generator) 
instead.


Now maybe the Stream.toArray() overloads aren't sufficient for what you want to 
do, in which case you might want to propose something. But that sounds like a 
different discussion.


s'marks

Re: RFR(s): 8060192: Add default method Collection.toArray(generator)

[Stuart Marks]

Hi Rémi,

On 6/15/18 12:26 AM, Remi Forax wrote:

The overrides I had previously provided in specific implementation classes like
ArrayList actually are slower, because the allocation of the array is done
separately from filling it. This necessitates the extra zero-filling step. Thus,
I've removed the overrides.


for ArrayList, you may use a code like this,
T[] toArray(IntFunction generator) {
 return (T[]) Arrays.copyOf(elementData, size, 
generator.apply(0).getClass());
   }
so you win only one comparison (yeah !), which can be perfectly predicted, so 
you should not see any perf difference :)


True, this will probably work better than the previous code (allocate correct 
size, fill with System.arraycopy) but it doesn't seem likely to be any faster 
than the default method.



List.class or List[].class do not work either.


I think they can be made to work (see other sub-thread with Peter Levart) but I 
don't see any advantages going in that direction.



For these reasons I'd like to proceed with adding toArray(generator) API.


so thumb up for me !


Great!

s'marks

Re: RFR JDK-8066709 Make some JDK system properties read only

[Brent Christian]

Hi, Roger

On 6/18/18 7:31 AM, Roger Riggs wrote:


The CSR and Webrev are updated.

webrev:
   http://cr.openjdk.java.net/~rriggs/webrev-static-property-8066709


* src/java.base/share/classes/java/lang/System.java :

Should the @implNote with the list of cached properties be added 
everywhere the @apiNote is being added ?  Right now the @implNote is 
only added to getProperties().



* src/java.base/share/classes/jdk/internal/util/StaticProperty.java :

Nit:

  45 private StaticProperty() {
  46
  47 }

Maybe put this all on one line?


Otherwise, the change looks good to me.

-Brent

Re: BiCollector

[Paul Sandoz]

Hi Peter,

Existing composing collectors tend to unpack all the functions of the input 
collector ahead of time, i don’t recall being too concerned about this at the 
time. It does allow for more robust null checking, something we were less 
diligent about doing.

Paul.

> On Jun 17, 2018, at 3:04 AM, Peter Levart  wrote:
> 
> Hi Tagir,
> 
> I don't know if this is important, but in your approach the particular 
> functions of the sub-collectors are retrieved eagerly even if they are later 
> not used. This should typically not present a problem as Collector(s) should 
> be prepared to be used in any scenario (parallel or serial). But anyway in my 
> approach, the sub-functions of the given collectors are retrived lazily, each 
> time the Stream implementation needs them - the dynamics of invoking 
> sub-collector methods to retrieve the functions is therefore unchanged when a 
> collector is used directly to collect a Stream or as a sub-collector in the 
> bi-collection scenario.
> 
> What do you think of that particular detail? Paul?
> 
> Regards, Peter
> 
> On 06/15/18 13:26, Tagir Valeev wrote:
>> Hello!
>> 
>> I created a preliminary webrev of my own implementation (no testcases yet):
>> http://cr.openjdk.java.net/~tvaleev/patches/pairing/webrev/ 
>> 
>> If anybody wants to sponsor my implementation, I will happily log an issue 
>> and write tests.
>> 
>> The name "pairing" was invented by me, but as I'm not a native English 
>> speaker I cannot judge whether it's optimal, so better ideas are welcome.
>> Also I decided to remove accumulator types from public type variables. They 
>> do not add anything to type signature, only clutter it 
>> increasing the number of type parameters from 4 to 6. I think it was a 
>> mistake to expose the accumulator type parameter in other cascading 
>> collectors 
>> like filtering(), collectingAndThen(), groupingBy(), etc. I'm not insisting 
>> though, if you feel that conformance to existing collectors is 
>> more important than simplicity.
>> 
>> With best regards,
>> Tagir Valeev.
>> 
>> On Fri, Jun 15, 2018 at 5:05 AM Brian Goetz > > wrote:
>> 
>> > Well, I don't see the need to pack the two results into a Map.Entry 
>> > (or any similar) container as a drawback. 
>> 
>>  From an "integrity of the JDK APIs" perspective, it is unquestionably a 
>> drawback.  These items are not a Key and an associated Value in a Map; 
>> it's merely pretending that Map.Entry really means "Pair".  There's a 
>> reason we don't have a Pair class in the JDK (and no, let's not reopen 
>> that now); using something else as a Pair proxy that is supposed to have 
>> specific semantics is worse. (It's fine to do this in your own code, but 
>> not in the JDK. Different standards for code that has different audiences.)
>> 
>> Tagir's proposed sidestepping is nice, and it will also play nicely with 
>> records, because then you can say:
>> 
>>   record NameAndCount(String name, int count);
>> 
>>   stream.collect(pairing(collectName, collectCount, NameAndCount::new));
>> 
>> and get a more properly abstract result out.  And its more in the spirit 
>> of existing Collectors.  If you want to use Map.Entry as an 
>> _implementation detail_, that's fine.
>> 
>> I can support this form.
>> 
>> > I also don't see a larger abstraction like BiStream as a natural fit 
>> > for a similar thing. 
>> 
>> I think the BiStream connection is mostly tangential.  We tried hard to 
>> support streams of (K,V) pairs when we did streams, as Paul can attest, 
>> but it was a huge complexity-inflater and dropping this out paid an 
>> enormous simplification payoff.
>> 
>> With records, having streams of tuples will be simpler to represent, but 
>> no more performant; it will take until we get to value types and 
>> specialized generics to get the performance we want out of this.
>> 
>> 
> 

Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit

[Martin Buchholz]

yes, the proposed new methods would use nulls differently.  The original
get() behavior of creating a mapping was a mistake, inconsistent with Map.
Yes, it will cause confusion.  But it's already confusing.

On Mon, Jun 18, 2018 at 1:45 PM, David Lloyd  wrote:

> On Mon, Jun 18, 2018 at 12:53 PM, Martin Buchholz 
> wrote:
> > On Mon, Jun 18, 2018 at 10:21 AM, Tony Printezis  >
> > wrote:
> >
> >> Martin,
> >>
> >> You are forgiven. :-)
> >>
> >> So, the (valid, I think) issue with getIfPresent(), as originally
> proposed
> >> by Peter, was that if get() was overridden in the subclass to somehow
> >> transform the returned value, getIfPresent() wouldn’t apply the same
> >> transformation.
> >>
> >> Doesn’t your compute() method have essentially the same issue? Apart
> from
> >> that, I personally like this proposal as I agree: one look-up is always
> >> better than two.
> >>
> >>
> > A non-prototype implementation would delegate compute into ThreadLocalMap
> > itself, where there is no risk of overriding.
>
> I don't think overriding is the risk; the risk would be compute*
> behaving inconsistently with regards to an overridden get*.
>
>
> --
> - DML
>

Re: BiCollector

[Brian Goetz]

distributing?

On 6/18/2018 6:04 PM, Chris Hegarty wrote:



On 18 Jun 2018, at 22:29, Brian Goetz  wrote:

"bisecting" sounds like it sends half the elements to one collector and half to 
the other ...

"tee" might be a candidate, though it doesn't follow the `ing convention.  
"teeing" sounds dumb.

Doesn’t follow the convention either, but:

   fanout
   fanningOut

-Chris.

Re: BiCollector

[Chris Hegarty]

> On 18 Jun 2018, at 22:29, Brian Goetz  wrote:
> 
> "bisecting" sounds like it sends half the elements to one collector and half 
> to the other ...
> 
> "tee" might be a candidate, though it doesn't follow the `ing convention.  
> "teeing" sounds dumb.

Doesn’t follow the convention either, but:

  fanout 
  fanningOut

-Chris.

Re: BiCollector

[James Laskey]

Replicator (as in DNA)

Sent from my iPhone

> On Jun 18, 2018, at 6:49 PM, James Laskey  wrote:
> 
> Bifurcate 
> 
> Sent from my iPhone
> 
>> On Jun 18, 2018, at 6:29 PM, Brian Goetz  wrote:
>> 
>> "bisecting" sounds like it sends half the elements to one collector and half 
>> to the other ...
>> 
>> "tee" might be a candidate, though it doesn't follow the `ing convention.  
>> "teeing" sounds dumb.
>> 
>> 
>> 
>>> On 6/15/2018 7:36 PM, Paul Sandoz wrote:
>>> Hi Tagir,
>>> 
>>> This is looking good.
>>> 
>>> My current favorite name for the factory method is “bisecting” since we are 
>>> dividing the collector into two collectors, the results of which are then 
>>> merged.
>>> Suggested first paragraph of the factory method:
>>> 
>>>  "Returns a collector that passes the input elements to two specified 
>>> collectors and merges their results with the specified merge function.”
>>> 
>>> Paul.
>>> 
 On Jun 15, 2018, at 4:26 AM, Tagir Valeev  wrote:
 
 Hello!
 
 I created a preliminary webrev of my own implementation (no testcases yet):
 http://cr.openjdk.java.net/~tvaleev/patches/pairing/webrev/
 If anybody wants to sponsor my implementation, I will happily log an issue 
 and write tests.
 
 The name "pairing" was invented by me, but as I'm not a native English 
 speaker I cannot judge whether it's optimal, so better ideas are welcome.
 Also I decided to remove accumulator types from public type variables. 
 They do not add anything to type signature, only clutter it
 increasing the number of type parameters from 4 to 6. I think it was a 
 mistake to expose the accumulator type parameter in other cascading 
 collectors
 like filtering(), collectingAndThen(), groupingBy(), etc. I'm not 
 insisting though, if you feel that conformance to existing collectors is
 more important than simplicity.
 
 With best regards,
 Tagir Valeev.
 
> On Fri, Jun 15, 2018 at 5:05 AM Brian Goetz  
> wrote:
> 
> Well, I don't see the need to pack the two results into a Map.Entry
> (or any similar) container as a drawback.
 From an "integrity of the JDK APIs" perspective, it is unquestionably a
 drawback.  These items are not a Key and an associated Value in a Map;
 it's merely pretending that Map.Entry really means "Pair".  There's a
 reason we don't have a Pair class in the JDK (and no, let's not reopen
 that now); using something else as a Pair proxy that is supposed to have
 specific semantics is worse. (It's fine to do this in your own code, but
 not in the JDK. Different standards for code that has different audiences.)
 
 Tagir's proposed sidestepping is nice, and it will also play nicely with
 records, because then you can say:
 
  record NameAndCount(String name, int count);
 
  stream.collect(pairing(collectName, collectCount, NameAndCount::new));
 
 and get a more properly abstract result out.  And its more in the spirit
 of existing Collectors.  If you want to use Map.Entry as an
 _implementation detail_, that's fine.
 
 I can support this form.
 
> I also don't see a larger abstraction like BiStream as a natural fit
> for a similar thing.
 I think the BiStream connection is mostly tangential.  We tried hard to
 support streams of (K,V) pairs when we did streams, as Paul can attest,
 but it was a huge complexity-inflater and dropping this out paid an
 enormous simplification payoff.
 
 With records, having streams of tuples will be simpler to represent, but
 no more performant; it will take until we get to value types and
 specialized generics to get the performance we want out of this.
 
 
>> 
> 

Re: BiCollector

[James Laskey]

Bifurcate 

Sent from my iPhone

> On Jun 18, 2018, at 6:29 PM, Brian Goetz  wrote:
> 
> "bisecting" sounds like it sends half the elements to one collector and half 
> to the other ...
> 
> "tee" might be a candidate, though it doesn't follow the `ing convention.  
> "teeing" sounds dumb.
> 
> 
> 
>> On 6/15/2018 7:36 PM, Paul Sandoz wrote:
>> Hi Tagir,
>> 
>> This is looking good.
>> 
>> My current favorite name for the factory method is “bisecting” since we are 
>> dividing the collector into two collectors, the results of which are then 
>> merged.
>>  Suggested first paragraph of the factory method:
>> 
>>   "Returns a collector that passes the input elements to two specified 
>> collectors and merges their results with the specified merge function.”
>> 
>> Paul.
>>  
>>> On Jun 15, 2018, at 4:26 AM, Tagir Valeev  wrote:
>>> 
>>> Hello!
>>> 
>>> I created a preliminary webrev of my own implementation (no testcases yet):
>>> http://cr.openjdk.java.net/~tvaleev/patches/pairing/webrev/
>>> If anybody wants to sponsor my implementation, I will happily log an issue 
>>> and write tests.
>>> 
>>> The name "pairing" was invented by me, but as I'm not a native English 
>>> speaker I cannot judge whether it's optimal, so better ideas are welcome.
>>> Also I decided to remove accumulator types from public type variables. They 
>>> do not add anything to type signature, only clutter it
>>> increasing the number of type parameters from 4 to 6. I think it was a 
>>> mistake to expose the accumulator type parameter in other cascading 
>>> collectors
>>> like filtering(), collectingAndThen(), groupingBy(), etc. I'm not insisting 
>>> though, if you feel that conformance to existing collectors is
>>> more important than simplicity.
>>> 
>>> With best regards,
>>> Tagir Valeev.
>>> 
 On Fri, Jun 15, 2018 at 5:05 AM Brian Goetz  wrote:
 
 Well, I don't see the need to pack the two results into a Map.Entry
 (or any similar) container as a drawback.
>>>  From an "integrity of the JDK APIs" perspective, it is unquestionably a
>>> drawback.  These items are not a Key and an associated Value in a Map;
>>> it's merely pretending that Map.Entry really means "Pair".  There's a
>>> reason we don't have a Pair class in the JDK (and no, let's not reopen
>>> that now); using something else as a Pair proxy that is supposed to have
>>> specific semantics is worse. (It's fine to do this in your own code, but
>>> not in the JDK. Different standards for code that has different audiences.)
>>> 
>>> Tagir's proposed sidestepping is nice, and it will also play nicely with
>>> records, because then you can say:
>>> 
>>>   record NameAndCount(String name, int count);
>>> 
>>>   stream.collect(pairing(collectName, collectCount, NameAndCount::new));
>>> 
>>> and get a more properly abstract result out.  And its more in the spirit
>>> of existing Collectors.  If you want to use Map.Entry as an
>>> _implementation detail_, that's fine.
>>> 
>>> I can support this form.
>>> 
 I also don't see a larger abstraction like BiStream as a natural fit
 for a similar thing.
>>> I think the BiStream connection is mostly tangential.  We tried hard to
>>> support streams of (K,V) pairs when we did streams, as Paul can attest,
>>> but it was a huge complexity-inflater and dropping this out paid an
>>> enormous simplification payoff.
>>> 
>>> With records, having streams of tuples will be simpler to represent, but
>>> no more performant; it will take until we get to value types and
>>> specialized generics to get the performance we want out of this.
>>> 
>>> 
> 

Re: BiCollector

[Jacob Glickman]

Agreed.  Not sure if this has been suggested, but what about duplex(ing)?

On Mon, Jun 18, 2018 at 5:29 PM Brian Goetz  wrote:

> "bisecting" sounds like it sends half the elements to one collector and
> half to the other ...
>
> "tee" might be a candidate, though it doesn't follow the `ing
> convention.  "teeing" sounds dumb.
>
>
>
> On 6/15/2018 7:36 PM, Paul Sandoz wrote:
> > Hi Tagir,
> >
> > This is looking good.
> >
> > My current favorite name for the factory method is “bisecting” since we
> are dividing the collector into two collectors, the results of which are
> then merged.
> >
> > Suggested first paragraph of the factory method:
> >
> >"Returns a collector that passes the input elements to two specified
> collectors and merges their results with the specified merge function.”
> >
> > Paul.
> >
> >
> >> On Jun 15, 2018, at 4:26 AM, Tagir Valeev  wrote:
> >>
> >> Hello!
> >>
> >> I created a preliminary webrev of my own implementation (no testcases
> yet):
> >> http://cr.openjdk.java.net/~tvaleev/patches/pairing/webrev/
> >> If anybody wants to sponsor my implementation, I will happily log an
> issue and write tests.
> >>
> >> The name "pairing" was invented by me, but as I'm not a native English
> speaker I cannot judge whether it's optimal, so better ideas are welcome.
> >> Also I decided to remove accumulator types from public type variables.
> They do not add anything to type signature, only clutter it
> >> increasing the number of type parameters from 4 to 6. I think it was a
> mistake to expose the accumulator type parameter in other cascading
> collectors
> >> like filtering(), collectingAndThen(), groupingBy(), etc. I'm not
> insisting though, if you feel that conformance to existing collectors is
> >> more important than simplicity.
> >>
> >> With best regards,
> >> Tagir Valeev.
> >>
> >> On Fri, Jun 15, 2018 at 5:05 AM Brian Goetz 
> wrote:
> >>
> >>> Well, I don't see the need to pack the two results into a Map.Entry
> >>> (or any similar) container as a drawback.
> >>   From an "integrity of the JDK APIs" perspective, it is unquestionably
> a
> >> drawback.  These items are not a Key and an associated Value in a Map;
> >> it's merely pretending that Map.Entry really means "Pair".  There's a
> >> reason we don't have a Pair class in the JDK (and no, let's not reopen
> >> that now); using something else as a Pair proxy that is supposed to have
> >> specific semantics is worse. (It's fine to do this in your own code, but
> >> not in the JDK. Different standards for code that has different
> audiences.)
> >>
> >> Tagir's proposed sidestepping is nice, and it will also play nicely with
> >> records, because then you can say:
> >>
> >>record NameAndCount(String name, int count);
> >>
> >>stream.collect(pairing(collectName, collectCount,
> NameAndCount::new));
> >>
> >> and get a more properly abstract result out.  And its more in the spirit
> >> of existing Collectors.  If you want to use Map.Entry as an
> >> _implementation detail_, that's fine.
> >>
> >> I can support this form.
> >>
> >>> I also don't see a larger abstraction like BiStream as a natural fit
> >>> for a similar thing.
> >> I think the BiStream connection is mostly tangential.  We tried hard to
> >> support streams of (K,V) pairs when we did streams, as Paul can attest,
> >> but it was a huge complexity-inflater and dropping this out paid an
> >> enormous simplification payoff.
> >>
> >> With records, having streams of tuples will be simpler to represent, but
> >> no more performant; it will take until we get to value types and
> >> specialized generics to get the performance we want out of this.
> >>
> >>
>
>

Re: BiCollector

[Brian Goetz]

"bisecting" sounds like it sends half the elements to one collector and 
half to the other ...


"tee" might be a candidate, though it doesn't follow the `ing 
convention.  "teeing" sounds dumb.




On 6/15/2018 7:36 PM, Paul Sandoz wrote:

Hi Tagir,

This is looking good.

My current favorite name for the factory method is “bisecting” since we are 
dividing the collector into two collectors, the results of which are then 
merged.
  
Suggested first paragraph of the factory method:


   "Returns a collector that passes the input elements to two specified 
collectors and merges their results with the specified merge function.”

Paul.
  


On Jun 15, 2018, at 4:26 AM, Tagir Valeev  wrote:

Hello!

I created a preliminary webrev of my own implementation (no testcases yet):
http://cr.openjdk.java.net/~tvaleev/patches/pairing/webrev/
If anybody wants to sponsor my implementation, I will happily log an issue and 
write tests.

The name "pairing" was invented by me, but as I'm not a native English speaker 
I cannot judge whether it's optimal, so better ideas are welcome.
Also I decided to remove accumulator types from public type variables. They do 
not add anything to type signature, only clutter it
increasing the number of type parameters from 4 to 6. I think it was a mistake 
to expose the accumulator type parameter in other cascading collectors
like filtering(), collectingAndThen(), groupingBy(), etc. I'm not insisting 
though, if you feel that conformance to existing collectors is
more important than simplicity.

With best regards,
Tagir Valeev.

On Fri, Jun 15, 2018 at 5:05 AM Brian Goetz  wrote:


Well, I don't see the need to pack the two results into a Map.Entry
(or any similar) container as a drawback.

  From an "integrity of the JDK APIs" perspective, it is unquestionably a
drawback.  These items are not a Key and an associated Value in a Map;
it's merely pretending that Map.Entry really means "Pair".  There's a
reason we don't have a Pair class in the JDK (and no, let's not reopen
that now); using something else as a Pair proxy that is supposed to have
specific semantics is worse. (It's fine to do this in your own code, but
not in the JDK. Different standards for code that has different audiences.)

Tagir's proposed sidestepping is nice, and it will also play nicely with
records, because then you can say:

   record NameAndCount(String name, int count);

   stream.collect(pairing(collectName, collectCount, NameAndCount::new));

and get a more properly abstract result out.  And its more in the spirit
of existing Collectors.  If you want to use Map.Entry as an
_implementation detail_, that's fine.

I can support this form.


I also don't see a larger abstraction like BiStream as a natural fit
for a similar thing.

I think the BiStream connection is mostly tangential.  We tried hard to
support streams of (K,V) pairs when we did streams, as Paul can attest,
but it was a huge complexity-inflater and dropping this out paid an
enormous simplification payoff.

With records, having streams of tuples will be simpler to represent, but
no more performant; it will take until we get to value types and
specialized generics to get the performance we want out of this.

Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit

[David Lloyd]

On Mon, Jun 18, 2018 at 12:53 PM, Martin Buchholz  wrote:
> On Mon, Jun 18, 2018 at 10:21 AM, Tony Printezis 
> wrote:
>
>> Martin,
>>
>> You are forgiven. :-)
>>
>> So, the (valid, I think) issue with getIfPresent(), as originally proposed
>> by Peter, was that if get() was overridden in the subclass to somehow
>> transform the returned value, getIfPresent() wouldn’t apply the same
>> transformation.
>>
>> Doesn’t your compute() method have essentially the same issue? Apart from
>> that, I personally like this proposal as I agree: one look-up is always
>> better than two.
>>
>>
> A non-prototype implementation would delegate compute into ThreadLocalMap
> itself, where there is no risk of overriding.

I don't think overriding is the risk; the risk would be compute*
behaving inconsistently with regards to an overridden get*.


-- 
- DML

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Doug Lea]

On 06/18/2018 11:22 AM, Martin Buchholz wrote:

> Or better, lockField.set in resetLock could instead be setRelease.
> Except it is using reflection, not VarHandles. So it could be preceded
> with VarHandle.releaseFence(), although it is hard to think of a
> scenario in which it could matter.
> 
> 
> You mean followed by, not preceded by?
> 
>           try {
>              lockField.set(this, new Object());
> +            java.lang.invoke.VarHandle.releaseFence();
>          } catch (IllegalAccessException e) {
>              throw new Error(e);
>          }
> 

OK. Followed by is a little better in that it orders any field write
before any write of this, not just the array copy before lock field write.

-Doug

Re: RFR 8204310 : Simpler RandomAccessFile.setLength() on Windows

[Ivan Gerasimov]

A gentle ping :)

Do you think it's good to go now?

With kind regards,

Ivan


On 6/10/18 11:15 PM, Ivan Gerasimov wrote:

Hi Alan!


On 6/6/18 6:57 AM, Alan Bateman wrote:
I think this should be okay but the Windows implementation has a long 
history of biting the fingers of anyone that dares touch it. 
Sometimes unexpected behavior changes only come to light long after 
the change. The recent mails here about Kafka and sparse files is a 
good example of that. So I think it's important to check the test 
coverage before pushing this change. Specifically I think we need to 
check that we have tests that


1. Exercise shrinking, extending, and not change the file length

2. Check getFilePointer when used with setLength.

3. Check how FileChannel behaves when created from a RandomAccessFile 
but the file length is changed after the FileChannel is obtained.


I extended the existing reg. test 
java/io/RandomAccessFile/SetLength.java to cover more cases that 
involve changing the file size.


Also I added another regression test, as you Alan suggested, to check 
that RandomAccessFile and its FileChannel behave consistently in 
various scenarios.


All the tests, including the new ones, pass on all supported platforms.

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8204310
WEBREV: http://cr.openjdk.java.net/~igerasim/8204310/01/webrev/

Would you please help review the fix?



--
With kind regards,
Ivan Gerasimov

Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit

[Martin Buchholz]

On Mon, Jun 18, 2018 at 10:21 AM, Tony Printezis 
wrote:

> Martin,
>
> You are forgiven. :-)
>
> So, the (valid, I think) issue with getIfPresent(), as originally proposed
> by Peter, was that if get() was overridden in the subclass to somehow
> transform the returned value, getIfPresent() wouldn’t apply the same
> transformation.
>
> Doesn’t your compute() method have essentially the same issue? Apart from
> that, I personally like this proposal as I agree: one look-up is always
> better than two.
>
>
A non-prototype implementation would delegate compute into ThreadLocalMap
itself, where there is no risk of overriding.


> Tony
>
>
> —
> Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
>
>
> On June 18, 2018 at 10:13:02 AM, Martin Buchholz (marti...@google.com)
> wrote:
>
> I'm ignoring the direct buffers problem (sorry Tony) and wearing my
> ReentrantReadWriteLock hat.  I still like the idea of adding
> ThreadLocal.compute(Function remappingFunction) and perhaps other such
> map-inspired methods. RRWL wouldn't benefit much, since it already tries to
> minimize use of ThreadLocal, but it would at least allow get() and remove()
> to be coalesced on read-unlock.
>
> RRWL never changes from one non-null value to another, it only switches
> between absent and present.  I'd like to avoid the two lookups due to get
> and remove.  Here's a prototype that does not yet help RRWL, but helps
> callers switching between non-null values, and could probably be extended
> via surgery to ThreadLocalMap:
>
> public T compute(
> java.util.function.Function
> remappingFunction) {
> final Thread currentThread = Thread.currentThread();
> final ThreadLocalMap map = getMap(currentThread);
> final ThreadLocalMap.Entry e = (map == null)
> ? null
> : map.getEntry(this);
> final T oldValue = (e == null) ? null : (T)e.value;
> final T newValue = remappingFunction.apply(oldValue);
> if (newValue == null) {
> if (oldValue != null) {
> map.remove(this);
> }
> } else if (e != null) {
> e.value = newValue;
> } else if (map != null) {
> map.set(this, newValue);
> } else {
> createMap(currentThread, newValue);
> }
> return newValue;
> }
>
>
>
>
>

Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit

[Tony Printezis]

Martin,

You are forgiven. :-)

So, the (valid, I think) issue with getIfPresent(), as originally proposed
by Peter, was that if get() was overridden in the subclass to somehow
transform the returned value, getIfPresent() wouldn’t apply the same
transformation.

Doesn’t your compute() method have essentially the same issue? Apart from
that, I personally like this proposal as I agree: one look-up is always
better than two.

Tony


—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com


On June 18, 2018 at 10:13:02 AM, Martin Buchholz (marti...@google.com)
wrote:

I'm ignoring the direct buffers problem (sorry Tony) and wearing my
ReentrantReadWriteLock hat.  I still like the idea of adding
ThreadLocal.compute(Function remappingFunction) and perhaps other such
map-inspired methods. RRWL wouldn't benefit much, since it already tries to
minimize use of ThreadLocal, but it would at least allow get() and remove()
to be coalesced on read-unlock.

RRWL never changes from one non-null value to another, it only switches
between absent and present.  I'd like to avoid the two lookups due to get
and remove.  Here's a prototype that does not yet help RRWL, but helps
callers switching between non-null values, and could probably be extended
via surgery to ThreadLocalMap:

public T compute(
java.util.function.Function
remappingFunction) {
final Thread currentThread = Thread.currentThread();
final ThreadLocalMap map = getMap(currentThread);
final ThreadLocalMap.Entry e = (map == null)
? null
: map.getEntry(this);
final T oldValue = (e == null) ? null : (T)e.value;
final T newValue = remappingFunction.apply(oldValue);
if (newValue == null) {
if (oldValue != null) {
map.remove(this);
}
} else if (e != null) {
e.value = newValue;
} else if (map != null) {
map.set(this, newValue);
} else {
createMap(currentThread, newValue);
}
return newValue;
}



On Sun, Jun 17, 2018 at 2:20 PM, Peter Levart 
wrote:

> Update: the discussion on concurrent-interest about possible future
> addition of public ThreadLocal.getIfPresent() or ThreadLocal.isPresent()
> has died out, but it nevertheless reached a point where
> ThreadLocal.isPresent() was found the least problematic. So I would like to
> get further with this proposal using the last webrev.04 version of the
> patch that uses ThreadLocal.isPresent() internally - still package-private
> at the moment.
>
> Here's the webrev (unchanged from the day it was published):
>
> http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.04/
>
> Would this version be OK?
>
> Regards, Peter
>
>
>
> On 06/06/18 20:55, Peter Levart wrote:
>
>

RFR: JDK-8205090 Update of the Reader:nullReader() spec for the mark() method

[Patrick Reinhart]

Hi Everybody,

In the process of solving the issue [1] a part of the proper solution
should be a partially change of the Reader.nullReader() specification. I
opened a new CSR to address this. The main change would be to change to
following lines:

|*  The {@code markSupported()} method returns {@code false}. The *
{@code mark()} method does nothing, and the {@code reset()} method *
throws {@code IOException}.|

to this:

|*  The {@code markSupported()} method returns {@code false}. The *
{@code mark()} and {@code reset()} methods throw an {@code IOException}.|

Side note:

I got a additional comment from Krushnareddy Ganapureddy stating:

> additional Note - spec says "ready()), skip(long), and transferTo()
methods all behave as if end of stream has been reached"
> But there is not clear say on these methods [ ready(), skip(),
transferTo() ] what is the expected behavior "if end of stream reached"

We may able to address the issue above too. Any ideas how to address this?


-Patrick


[1] https://bugs.openjdk.java.net/browse/JDK-8204930

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Claes Redestad]

On 2018-06-18 18:09, Peter Levart wrote:



On 06/18/2018 04:47 PM, Claes Redestad wrote:



On 2018-06-18 16:23, Peter Levart wrote:

Hi Claes,

On 06/18/2018 03:54 PM, Claes Redestad wrote:
I'd suggest something simple like this to ensure correctness, while 
keeping the number of volatile reads to a minimum:


http://cr.openjdk.java.net/~redestad/8199435.00/

Then file a follow-up RFE to investigate if we can make these 
fields non-volatile, e.g. using careful fencing as suggested

by Peter.


OK, but the constructor will still need a releaseFence at the end to 
prevent a potential write that unsafely publishes Properties 
instance to float before the volatile writes of 'map' and 'defaults'...


You might want to use Unsafe directly for that since VarHandle could 
cause bootstrap issues.


I don't follow.. which constructor needs a fence in the suggested patch?

/Claes


The Properties constructor:


    private Properties(Properties defaults, int initialCapacity) {
    // use package-private constructor to
    // initialize unused fields with dummy values
    super((Void) null);
    map = new ConcurrentHashMap<>(initialCapacity);
    this.defaults = defaults;

        UNSAFE.storeFence(); // <-- HERE!!
    }


Final fields have bigger guarantee than volatile fields in this respect.

Plain write that follows in program a volatile write, can in theory 
float before the volatile write. So if you publish a Properties 
instance via data race (via plain write), the observer may still see 
uninitialized 'map' and 'defaults' fields.




Right

http://cr.openjdk.java.net/~redestad/8199435.01/

(Yes, using VarHandle.storeStoreFence would do the exact same thing, but 
is not usable from Properties as the VarHandle impl needs to read some 
system properties...)


/Claes

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Peter Levart]

On 06/18/2018 04:47 PM, Claes Redestad wrote:



On 2018-06-18 16:23, Peter Levart wrote:

Hi Claes,

On 06/18/2018 03:54 PM, Claes Redestad wrote:
I'd suggest something simple like this to ensure correctness, while 
keeping the number of volatile reads to a minimum:


http://cr.openjdk.java.net/~redestad/8199435.00/

Then file a follow-up RFE to investigate if we can make these fields 
non-volatile, e.g. using careful fencing as suggested

by Peter.


OK, but the constructor will still need a releaseFence at the end to 
prevent a potential write that unsafely publishes Properties instance 
to float before the volatile writes of 'map' and 'defaults'...


You might want to use Unsafe directly for that since VarHandle could 
cause bootstrap issues.


I don't follow.. which constructor needs a fence in the suggested patch?

/Claes


The Properties constructor:


    private Properties(Properties defaults, int initialCapacity) {
    // use package-private constructor to
    // initialize unused fields with dummy values
    super((Void) null);
    map = new ConcurrentHashMap<>(initialCapacity);
    this.defaults = defaults;

        UNSAFE.storeFence(); // <-- HERE!!
    }


Final fields have bigger guarantee than volatile fields in this respect.

Plain write that follows in program a volatile write, can in theory 
float before the volatile write. So if you publish a Properties instance 
via data race (via plain write), the observer may still see 
uninitialized 'map' and 'defaults' fields.


Regards, Peter

Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit

[Alan Bateman]

On 17/06/2018 22:20, Peter Levart wrote:
Update: the discussion on concurrent-interest about possible future 
addition of public ThreadLocal.getIfPresent() or 
ThreadLocal.isPresent() has died out, but it nevertheless reached a 
point where ThreadLocal.isPresent() was found the least problematic. 
So I would like to get further with this proposal using the last 
webrev.04 version of the patch that uses ThreadLocal.isPresent() 
internally - still package-private at the moment.


Here's the webrev (unchanged from the day it was published):

http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.04/

Would this version be OK?

I think looks quite good.

One small nit is that the update to ThreadLocal.setInitialValue makes it 
look like all locals are registered when setting the initial value. What 
would you think about moving the instanceof check from 
TerminatingThreadLocal.register so that it's a bit more obvious.


-Alan

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Martin Buchholz]

On Mon, Jun 18, 2018 at 7:21 AM, Doug Lea  wrote:

> On 06/18/2018 10:05 AM, Martin Buchholz wrote:
> > There's a long history of cheating and setting final fields in
> > pseudo-constructors readObject and clone, which is well motivated since
> > Java should really support pseudo-constructors in a better way..
> > Cheating has used Unsafe or reflection with setAccessible
> > (CopyOnWriteArrayList.resetLock()).
> >
>
> > Doug - would CopyOnWriteArrayList.clone() be slightly safer if it had a
> > releaseFence() ?
> >
>
> Or better, lockField.set in resetLock could instead be setRelease.
> Except it is using reflection, not VarHandles. So it could be preceded
> with VarHandle.releaseFence(), although it is hard to think of a
> scenario in which it could matter.
>

You mean followed by, not preceded by?

  try {
 lockField.set(this, new Object());
+java.lang.invoke.VarHandle.releaseFence();
 } catch (IllegalAccessException e) {
 throw new Error(e);
 }

Re: [11] RFR: 8042131: DateTimeFormatterBuilder Mapped-values do not work for JapaneseDate

[Stephen Colebourne]

+1
Stephen


On 18 June 2018 at 15:55, Roger Riggs  wrote:
> Hi Naoto,
>
> Looks fine
>
> Regards, Roger
>
>
>
> On 6/15/2018 6:14 PM, Naoto Sato wrote:
>>
>> Hello,
>>
>> Please review the fix to the following issue:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8042131
>>
>> The proposed change is located at:
>>
>> http://cr.openjdk.java.net/~naoto/8042131/webrev.00/
>>
>> The fix is originally contributedy by Toshio Nakamura at IBM [1]. I am
>> sponsoring the fix, with some trivial modifications to the test (diff is
>> attached).
>>
>> Naoto
>>
>> [1]
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-June/053830.html
>
>

Re: [11] RFR: 8042131: DateTimeFormatterBuilder Mapped-values do not work for JapaneseDate

[Roger Riggs]

Hi Naoto,

Looks fine

Regards, Roger


On 6/15/2018 6:14 PM, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8042131

The proposed change is located at:

http://cr.openjdk.java.net/~naoto/8042131/webrev.00/

The fix is originally contributedy by Toshio Nakamura at IBM [1]. I am 
sponsoring the fix, with some trivial modifications to the test (diff 
is attached).


Naoto

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-June/053830.html

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Claes Redestad]

On 2018-06-18 16:23, Peter Levart wrote:

Hi Claes,

On 06/18/2018 03:54 PM, Claes Redestad wrote:
I'd suggest something simple like this to ensure correctness, while 
keeping the number of volatile reads to a minimum:


http://cr.openjdk.java.net/~redestad/8199435.00/

Then file a follow-up RFE to investigate if we can make these fields 
non-volatile, e.g. using careful fencing as suggested

by Peter.


OK, but the constructor will still need a releaseFence at the end to 
prevent a potential write that unsafely publishes Properties instance 
to float before the volatile writes of 'map' and 'defaults'...


You might want to use Unsafe directly for that since VarHandle could 
cause bootstrap issues.


I don't follow.. which constructor needs a fence in the suggested patch?

/Claes

Re: RFR JDK-8066709 Make some JDK system properties read only

[Roger Riggs]

Hi Alan,

Thanks for the review and suggestion.

The CSR and Webrev are updated.

webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-static-property-8066709/

csr:
  https://bugs.openjdk.java.net/browse/JDK-8204235

On 6/16/2018 8:42 AM, Alan Bateman wrote:

On 13/06/2018 15:10, Roger Riggs wrote:

Hi Joe,

The CSR text is still in draft and got out of sync with the open 
review and comments.


The updated apiNote clarifies that changes made through the 
System.*property*  methods

may not affect the value cached during initialization.
The implementation changes affect a very short list of properties.

The note on the methods is to raise awareness that individual 
properties may have

different behavior and may be unpredictable.
Just catching up on this. I checked the CSR and the webrev (the latest 
webrev was generated on June 6 so I hope that is the version we are 
meant to look at).


The updated apiNote (CSR version) looks okay but I think the word 
"internal" needs to be dropped as it comes with too many questions. 
"may be cached during initialization or ..." should be okay. The 
editing has meant the line lengths are a bit inconsistent so I assume 
they can be fixed up before the change is pushed.


I see the original patch to SocksSocketImpl has been mostly reverted. 
It looks correct now. The other usages look okay to me.


-Alan

Re: Proposal: optimization of Map.copyOf and Collectors.toUnmodifiableMap

[Roger Riggs]

Hi Stuart,

In regard to new SharedSecret interfaces, one option is move shared (but 
private) implementation classes
to a jdk.internal.xx package (not exported).  This only works well if 
they are not tightly coupled to other

package private classes.

SpinedBuffer might be a good candidate, I have some IO cases in mind 
that could benefit from

the allocation/reallocation savings.  (ByteArrayOutputStream for 1).

Regards, Roger

On 6/15/2018 5:22 PM, Stuart Marks wrote:

Hi Peter,

Finally getting back to this.

I think there's clearly room for improvement in the creation of the 
unmodifiable collections. Right now the creation paths (mostly) use 
only public APIs, which can result in unnecessary allocation and 
copying. Certainly Map.copyOf does this, but there are also other 
cases as well.


For copying from an unknown map, there are a couple approaches:

* accumulate keys and values in a single ArrayList, which can be 
presized as necessary, but which will grow if necessary; then copy 
elements from a subrange of ArrayList's internal array (similar to 
your MapN(Object[], len) overload)


* accumulate keys and values into a SpinedBuffer, which doesn't 
require copying to grow, which is preferable if for some reason we 
can't pre-size accurately; and then copy the elements out of it


The Collectors.toUnmodifiableMap implementations are known to create 
HashMap instances, so they could pass the HashMap directly to a 
private MapN constructor that in turn could talk directly to HashMap 
to get the keys and values. This avoids allocation of a full-sized 
buffer and one copy.


Note that these techniques involve creating new interfaces, sometimes 
that cross package boundaries. It's a bit of an irritant to have to 
plumb new paths that go through SharedSecrets, but it seems likely to 
be worthwhile if we can avoid bulk allocation and copying steps.


Given these, it doesn't seem to me that the BiBuffer approach helps 
very much. I think there are many other avenues that would be 
worthwhile to explore, and that possibly can provide bigger savings.


s'marks





On 6/11/18 3:57 AM, Peter Levart wrote:

Hi,

Those two methods were added in JDK 10 and they are not very optimal. 
Map.copyOf(Map map) 1st dumps the source map into an array of 
Map.Entry(s) (map.entrySet().toArray(new Entry[0])), which typically 
creates new Map.Entry objects, then passes the array to 
Map.ofEntries(Map.Entry[] entries) factory method that iterates the 
array and constructs a key/value interleaved array from it which is 
passed to ImmutableCollections.MapN constructor, which then 
constructs a linear-probing hash table from it. So each key and value 
is copied 3 times, while several temporary objects are created in the 
process.


One copying step could be eliminated and construction of temporary 
Map.Entry objects too:


http://cr.openjdk.java.net/~plevart/jdk-dev/UnmodifiableMap_copyOf/webrev.01/ 



Collecting stream(s) using Collectors.toUnmodifiableMap() 1st 
collects key/value pairs into a HashMap, then it performs equivalent 
operations as Map.copyOf(hashMap) at the end. Using Map.copyOf() 
directly benefits this collection operation too.


The following benchmark:

http://cr.openjdk.java.net/~plevart/jdk-dev/UnmodifiableMap_copyOf/UnmodifiableMapBench.java 




Shows up to 30% improvement of .copyOf operation with this patch 
applied:


Original:

Benchmark   (size)  Mode Cnt Score Error  
Units
UnmodifiableMapBench.copyOf 10  avgt 10 403.633 ± 
2.640  ns/op
UnmodifiableMapBench.copyOf    100  avgt   10 3489.623 ± 
44.590  ns/op
UnmodifiableMapBench.copyOf   1000  avgt   10 40030.572 ± 
277.075  ns/op
UnmodifiableMapBench.toUnmodifiableMap  10  avgt 10 831.221 ± 
3.816  ns/op
UnmodifiableMapBench.toUnmodifiableMap 100  avgt   10 9783.519 ± 
43.097  ns/op
UnmodifiableMapBench.toUnmodifiableMap    1000  avgt   10 96524.536 ± 
670.818  ns/op


Patched:

Benchmark   (size)  Mode Cnt Score Error  
Units
UnmodifiableMapBench.copyOf 10  avgt 10 264.172 ± 
1.882  ns/op
UnmodifiableMapBench.copyOf    100  avgt   10 2318.974 ± 
15.877  ns/op
UnmodifiableMapBench.copyOf   1000  avgt   10 29291.782 ± 
3139.737  ns/op
UnmodifiableMapBench.toUnmodifiableMap  10  avgt 10 771.221 ± 
65.432  ns/op
UnmodifiableMapBench.toUnmodifiableMap 100  avgt   10 9275.016 ± 
725.722  ns/op
UnmodifiableMapBench.toUnmodifiableMap    1000  avgt   10 82204.342 ± 
851.741  ns/op



Production of garbage is also reduced, since no Map.Entry temporary 
objects are constructed:


Original:

Benchmark (size)  Mode  Cnt  Score   Error   Units
UnmodifiableMapBench.copyOf:·gc.alloc.rate.norm 10  avgt   10 416.001 
± 0.002    B/op
UnmodifiableMapBench.copyOf:·gc.alloc.rate.norm 100  avgt   10 
2936.005 ± 0.019    B/op
UnmodifiableMapBench.copyOf:·gc.alloc.rate.norm 1000  avgt   10 
28136.059 ±   

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Peter Levart]

Hi Claes,

On 06/18/2018 03:54 PM, Claes Redestad wrote:
I'd suggest something simple like this to ensure correctness, while 
keeping the number of volatile reads to a minimum:


http://cr.openjdk.java.net/~redestad/8199435.00/

Then file a follow-up RFE to investigate if we can make these fields 
non-volatile, e.g. using careful fencing as suggested

by Peter.


OK, but the constructor will still need a releaseFence at the end to 
prevent a potential write that unsafely publishes Properties instance to 
float before the volatile writes of 'map' and 'defaults'...


You might want to use Unsafe directly for that since VarHandle could 
cause bootstrap issues.


Regards, Peter



/Claes

On 2018-06-18 15:27, Claes Redestad wrote:



On 2018-06-18 13:06, Peter Levart wrote:
Adding a volatile read on every read through Properties is likely 
to have some performance impact,


On non-Intel architectures in particular. On intel it would just 
inhibit some JIT optimizations like hoisting the read of field out 
of loop... 


Right, and coincidentally those platforms are where I'd expect the 
current implementation to cause bugs (I've not been able to provoke

any real error on my Intel-based workstations).

I'd be surprised if we'd be much slower than pre-8029891 using 
volatiles (volatile read vs synchronized - even with biased locking),

and we'd still retain the scalability benefits of 8029891.

Ignore my remarks about clone - I'm just back from vacation and have 
apparently forgotten how cloning works. :-)


/Claes

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Doug Lea]

On 06/18/2018 10:05 AM, Martin Buchholz wrote:
> There's a long history of cheating and setting final fields in
> pseudo-constructors readObject and clone, which is well motivated since
> Java should really support pseudo-constructors in a better way..
> Cheating has used Unsafe or reflection with setAccessible
> (CopyOnWriteArrayList.resetLock()).
> 

> Doug - would CopyOnWriteArrayList.clone() be slightly safer if it had a
> releaseFence() ?
> 

Or better, lockField.set in resetLock could instead be setRelease.
Except it is using reflection, not VarHandles. So it could be preceded
with VarHandle.releaseFence(), although it is hard to think of a
scenario in which it could matter.

-Doug

Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit

[Martin Buchholz]

I'm ignoring the direct buffers problem (sorry Tony) and wearing my
ReentrantReadWriteLock hat.  I still like the idea of adding
ThreadLocal.compute(Function remappingFunction) and perhaps other such
map-inspired methods. RRWL wouldn't benefit much, since it already tries to
minimize use of ThreadLocal, but it would at least allow get() and remove()
to be coalesced on read-unlock.

RRWL never changes from one non-null value to another, it only switches
between absent and present.  I'd like to avoid the two lookups due to get
and remove.  Here's a prototype that does not yet help RRWL, but helps
callers switching between non-null values, and could probably be extended
via surgery to ThreadLocalMap:

public T compute(
java.util.function.Function
remappingFunction) {
final Thread currentThread = Thread.currentThread();
final ThreadLocalMap map = getMap(currentThread);
final ThreadLocalMap.Entry e = (map == null)
? null
: map.getEntry(this);
final T oldValue = (e == null) ? null : (T)e.value;
final T newValue = remappingFunction.apply(oldValue);
if (newValue == null) {
if (oldValue != null) {
map.remove(this);
}
} else if (e != null) {
e.value = newValue;
} else if (map != null) {
map.set(this, newValue);
} else {
createMap(currentThread, newValue);
}
return newValue;
}



On Sun, Jun 17, 2018 at 2:20 PM, Peter Levart 
wrote:

> Update: the discussion on concurrent-interest about possible future
> addition of public ThreadLocal.getIfPresent() or ThreadLocal.isPresent()
> has died out, but it nevertheless reached a point where
> ThreadLocal.isPresent() was found the least problematic. So I would like to
> get further with this proposal using the last webrev.04 version of the
> patch that uses ThreadLocal.isPresent() internally - still package-private
> at the moment.
>
> Here's the webrev (unchanged from the day it was published):
>
> http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.04/
>
> Would this version be OK?
>
> Regards, Peter
>
>
>
> On 06/06/18 20:55, Peter Levart wrote:
>
>

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Martin Buchholz]

There's a long history of cheating and setting final fields in
pseudo-constructors readObject and clone, which is well motivated since
Java should really support pseudo-constructors in a better way..
Cheating has used Unsafe or reflection with setAccessible
(CopyOnWriteArrayList.resetLock()).

I haven't had any success actually reproducing any data race failures in
constructors - on modern machines it would only happen if the JIT reordered
the writes.

Doug - would CopyOnWriteArrayList.clone() be slightly safer if it had a
releaseFence() ?

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Claes Redestad]

I'd suggest something simple like this to ensure correctness, while 
keeping the number of volatile reads to a minimum:


http://cr.openjdk.java.net/~redestad/8199435.00/

Then file a follow-up RFE to investigate if we can make these fields 
non-volatile, e.g. using careful fencing as suggested

by Peter.

/Claes

On 2018-06-18 15:27, Claes Redestad wrote:



On 2018-06-18 13:06, Peter Levart wrote:
Adding a volatile read on every read through Properties is likely to 
have some performance impact,


On non-Intel architectures in particular. On intel it would just 
inhibit some JIT optimizations like hoisting the read of field out of 
loop... 


Right, and coincidentally those platforms are where I'd expect the 
current implementation to cause bugs (I've not been able to provoke

any real error on my Intel-based workstations).

I'd be surprised if we'd be much slower than pre-8029891 using 
volatiles (volatile read vs synchronized - even with biased locking),

and we'd still retain the scalability benefits of 8029891.

Ignore my remarks about clone - I'm just back from vacation and have 
apparently forgotten how cloning works. :-)


/Claes

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Claes Redestad]

On 2018-06-18 13:06, Peter Levart wrote:
Adding a volatile read on every read through Properties is likely to 
have some performance impact,


On non-Intel architectures in particular. On intel it would just 
inhibit some JIT optimizations like hoisting the read of field out of 
loop... 


Right, and coincidentally those platforms are where I'd expect the 
current implementation to cause bugs (I've not been able to provoke

any real error on my Intel-based workstations).

I'd be surprised if we'd be much slower than pre-8029891 using volatiles 
(volatile read vs synchronized - even with biased locking),

and we'd still retain the scalability benefits of 8029891.

Ignore my remarks about clone - I'm just back from vacation and have 
apparently forgotten how cloning works. :-)


/Claes

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Peter Levart]

Hi Claes,

On 06/18/2018 12:02 PM, Claes Redestad wrote:



On 2018-06-18 05:45, David Holmes wrote:
Making it "final" to fix unsafe publication only works with actual 
publication after construction. You're making it "final" but then not 
setting it at construction time and instead using UNSAFE.putVolatile 
to get around the fact that it is final! The "final" serves no 
purpose in that regard. Further a putVolatile without corresponding 
getVolatile does not achieve safe publication under the JMM.


Adding a volatile read on every read through Properties is likely to 
have some performance impact,


On non-Intel architectures in particular. On intel it would just inhibit 
some JIT optimizations like hoisting the read of field out of loop...


but it could still be better than before 8029891 where such operations 
were synchronized. Regarding Peter's comments about defaults, I think 
accesses to that field was implicitly guarded by the synchronized 
super.get calls before 8029891 and now needs to be re-examined too.


This was pre- 8029891 code of getProperty for example:

    public String getProperty(String key) {
    Object oval = super.get(key);
    String sval = (oval instanceof String) ? (String)oval : null;
    return ((sval == null) && (defaults != null)) ? 
defaults.getProperty(key) : sval;

    }

So 'defaults' field was accessed out of synchronized super.get() method 
and getProperty was not synchronized. This is pre-existing issue. But 
'map' field is new.


The problem with 'defaults' field is also that is is protected and 
subclasses can access it without synchronization. If it is made 
volatile, getProperty() method will be penalized.


So maybe using weaker memory fences in combination with plain 'property' 
field can make the field behave like it was final, so at least accesses 
to field within Properties class can be made non-racy. If this is done 
for 'properties' then same fences will work for 'map' too and it doesn't 
have to be final.






I think making the actual field(s) volatile is the only JMM compliant 
way to correctly address this.


The only issue is serialization-related readHashTable method (clone 
could be implemented without Unsafe as clone.map.putAll(map)). 


1st you would have to do:

    clone.map = new CHM();

and only then:

    clone.map.putAll(map);

or else you would be adding the elements of the original map to the 
original map (which would actually work with CHM), but then the cloned 
Properties object would share the map with original one. So you do need 
to assign to 'map' field in clone().



Is there no sanctioned way (using e.g. VarHandles) to safely construct 
and publish a transient "final" field in the scope of a readObject? 
I've been shying away from experimenting with VarHandles in places 
like Properties as I'm certain it could lead to bootstrap issues, but 
for something like a deserialization hook it might very well be fine.


Paul would know for sure, but I think this was deliberately prevented in 
VarHandle(s).


Regards, Peter



/Claes

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

[Claes Redestad]

On 2018-06-18 05:45, David Holmes wrote:
Making it "final" to fix unsafe publication only works with actual 
publication after construction. You're making it "final" but then not 
setting it at construction time and instead using UNSAFE.putVolatile 
to get around the fact that it is final! The "final" serves no purpose 
in that regard. Further a putVolatile without corresponding 
getVolatile does not achieve safe publication under the JMM.


Adding a volatile read on every read through Properties is likely to 
have some performance impact, but it could still be better than before 
8029891 where such operations were synchronized. Regarding Peter's 
comments about defaults, I think accesses to that field was implicitly 
guarded by the synchronized super.get calls before 8029891 and now needs 
to be re-examined too.




I think making the actual field(s) volatile is the only JMM compliant 
way to correctly address this.


The only issue is serialization-related readHashTable method (clone 
could be implemented without Unsafe as clone.map.putAll(map)). Is there 
no sanctioned way (using e.g. VarHandles) to safely construct and 
publish a transient "final" field in the scope of a readObject? I've 
been shying away from experimenting with VarHandles in places like 
Properties as I'm certain it could lead to bootstrap issues, but for 
something like a deserialization hook it might very well be fine.


/Claes