Re: We don't need jdk.internal.ref.Cleaner any more

[Peter Levart]

On 02/17/2016 01:17 AM, Kim Barrett wrote:

On Feb 8, 2016, at 5:46 AM, Peter Levart  wrote:
If special-casing in ReferenceHandler is removed then an opportunity opens to 
get rid of ReferenceHandler thread altogether. VM/GC could enqueue Reference(s) 
to their respective ReferenceQueue(s) directly, so everyone could enjoy the 
benefits sun.misc.Cleaner has…

I don't think that's likely.

That would require the VM/GC to acquire all the queue locks, and we
certainly wouldn't want to do that in a STW pause, so an intermediate
thread is kind of unavoidable.  [GC already needs to acquire the
pending list lock, for interaction with the reference queue thread.]

[Or change to lock-free enqueuing and wakeup of waiters, but I think
the current preference would be to avoid pulling even something like
that into a STW phase.]



Yeah, probably this would be to much to do in a STW pause. It's better 
to leave as much processing to a background thread as possible because 
it can be performed concurrently with application threads.


Regards, Peter

Re: RFR: 8072727 - add variation of Stream.iterate() that's finite

[Tagir F. Valeev]

Hello, Stuart!

Thank you for your comments.

SM> I'd suggest focusing on the API first before worrying about how to track the
SM> stream state with booleans, etc. Is the API convenient to use, and how well 
does
SM> it support the use cases we envision for it?

As Brian already noted, the most benefit of such signature is the
resemblance to the good old for loop. Also it's good, because the
lambdas don't need to maintain external mutable state in this case
(the state is encapsulated in the current element). Most of your
proposed examples, however, need to do it as they don't receive the
existing state. Also I see no reason to create a method which is
compatible with iterator::hasNext/iterator::next or even
spliterator::tryAdvance. If you already have a spliterator, you can
create a stream using StreamSupport.stream(spliterator, false). If you
have an iterator, you can convert it to spliterator using
Spliterators.spliterator[UnknownSize]. Well, this probably looks ugly,
but more flexible and signals that some low-level stuff is performed.

Supplier is definitely bad in terms of performance.
Creating a new stream is not nearly free. To illustrate this I wrote a
simple benchmark which compares .flatMapToInt(OptionalInt::stream)
with Java8 way .filter(OptionalInt::isPresent).mapToInt(OptionalInt::getAsInt)

Here's source code and full output:
http://cr.openjdk.java.net/~tvaleev/jmh/optionalstream/

Benchmark   (n)  Mode  Cnt  Score Error  
Units
OptionalTest.testOptionalFilterGet   10  avgt   30  0,171 ±   0,011  
us/op
OptionalTest.testOptionalFilterGet 1000  avgt   30  6,295 ±   0,046  
us/op
OptionalTest.testOptionalFilterGet  100  avgt   30  12597,706 ±  69,214  
us/op
OptionalTest.testOptionalStream  10  avgt   30  0,330 ±   0,002  
us/op
OptionalTest.testOptionalStream1000  avgt   30 27,552 ±   0,577  
us/op
OptionalTest.testOptionalStream 100  avgt   30  30837,240 ± 812,420  
us/op

Involving intermediate streams makes the thing at least twice slower.
Surely this delay could become negligible in some scenarios, but I
think it's inacceptable to enforce users to create new source with a
bunch of streams. At least primitive specializations will become
meaningless in this case: boxing would eat much less time compared to
stream creation.

As for elements drawn from the queue, it's much better to use existing
takeWhile method:

queue.stream().takeWhile(x -> x.equals(sentinel));

True, such approach will not include the sentinel element to the
result, and there's no easy way to do it with current API. Probably
additional method (takeWhileInclusive?) could be considered to solve
such problems. Still, I think, drawing from the queue is not the
problem which should be solved with new iterate() method.

As for Collatz conjecture, it's quite easy to iterate without trailing
one:

IntStream.iterate(start, val -> val != 1,
  val -> val % 2 == 0 ? val / 2 : val * 3 + 1)

If having one is desired, then it would be easier just to append one
to the stream (even if Collatz conjecture is false we will have an
infinite stream, so appended one will never appear):

IntStream.concat(
  IntStream.iterate(start, val -> val != 1,
  val -> val % 2 == 0 ? val / 2 : val * 3 + 1),
  IntStream.of(1))

A side note: having IntStream.append(int... numbers) would be really
nice:

IntStream.iterate(start, val -> val != 1,
  val -> val % 2 == 0 ? val / 2 : val * 3 + 1).append(1)

Another approach would be to introduce a special stop value (for
example, -1):

IntStream.iterate(start, val -> val != -1,
  val -> val == 1 ? -1 : val % 2 == 0 ? val / 2 : val * 3 + 1)

This stream produces Collatz series, including the trailing one.

As for Craps, I never heard about such game. If I understood the rules
correctly, it's good to represent the state as separate object and
define state transition via its method. Something like this should
work:

Random r = new Random();
IntSupplier dice = () -> r.nextInt(6)+r.nextInt(6)+2;
class State {
  int roll, point;
  
  State(int roll, int point) {
this.roll = roll;
this.point = point;
  }
  
  State() {
this(dice.getAsInt(), 0);
  }
  
  boolean isStopRound() {
return roll == 7 || (point == 0 && (roll > 10 || roll < 4)) || (point != 0 
&& roll == point);
  }
  
  State next() {
return isStopRound() ? null : new State(dice.getAsInt(), point == 0 ? roll 
: point);
  }
}
Stream.iterate(new State(), Objects::nonNull, State::next)
  .mapToInt(state -> state.roll)
  .forEach(System.out::println);

With best regards,
Tagir Valeev.


SM> In particular, I can imagine a number of cases where it would be very 
helpful to
SM> be able to support an empty stream, or where the computation to produce the
SM> first element is the same as the computation to produce subsequent elements.
SM> Requiring a value for the first stream element is at odds with that.

SM> Here are some ideas for use cases to 

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

[Kim Barrett]

> On Feb 16, 2016, at 7:20 PM, Kim Barrett  wrote:
> src/java.base/share/classes/java/lang/ref/Reference.java
> 
> After removal of the special handling of removing Cleaner objects from
> the pending list, do we still need to catch OOMEs from the pending
> list?  If not sure, feel free to defer that in another RFE for cleanup.

Never mind.  OOME could result from attempting to construct and throw 
InterruptedException.

RFR (JAXP) 8146237: PREFER from Features API taking precedence over catalog file

[huizhe wang]

Hi,

Please review a fix on missing property settings read from catalog 
files. Values from catalog files shall override any other settings 
including those through the API, e.g. 
builder().with(CatalogFeatures.Feature.PREFER, "system").


JBS: https://bugs.openjdk.java.net/browse/JDK-8146237
webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8146237/webrev/

Thanks,
Joe

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

[Kim Barrett]

> On Feb 16, 2016, at 11:15 AM, Peter Levart  wrote:
> 
> Hello everybody,
> 
> Thanks for looking into this and for all your comments. Now that it seems we 
> have a consensus that replacing internal Cleaner with public Cleaner is not a 
> wrong idea, I created an issue for it [1] so that we may officially review 
> the implementation. I also created an updated webrev [2] which fixes some 
> comments in usages that were not correct any more after the change. I also 
> took the liberty to modify the CleanerImpl.InnocuousThreadFactory to give 
> names to threads in accordance to the style used in some other thread 
> factories (Timer-0, Timer-1, ..., Cleaner-0, Cleaner-1, ..., etc.). This 
> gives the thread of internal Cleaner instance, which is now constructed as 
> part of the boot-up sequence, a stable name: "Cleaner-0".
> 
> If you feel that internal Cleaner instance should have a Thread with 
> MAX_PRIORITY, I can incorporate that too.
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8149925
> [2] 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.03/
> 
> I re-ran the java/lang/ref and java/nio tests and all pass except two ignored 
> tests:

I think some of the existing uses of sun.misc.Cleaner were really just
a matter of convenience and the previous lack of
java.lang.ref.Cleaner.  An example of this that I have personal
knowledge of the history for is the CallSite cleanup in
MethodHandleNatives.  It's good to see those converted.

As for the others, if nobody wants to defend the special handling by
the reference processing thread, I'm certainly happy to see it
removed. However, I recall Roger saying there were existing tests that
failed when certain uses of sun.misc.Cleaner were replaced with
java.lang.ref.Cleaner. I don't remember the details, but maybe Roger
does.

-- 
src/java.base/share/classes/java/lang/ref/Reference.java

After removal of the special handling of removing Cleaner objects from
the pending list, do we still need to catch OOMEs from the pending
list?  If not sure, feel free to defer that in another RFE for cleanup.

--
src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java

I don't understand why CleanerImpl needs to be changed to public in
order to provide access to the new drainQueue. Wouldn't it be better
to add Cleaner.drainQueue?

Some of the other changes here don't seem related to the problem at
hand. Are these proposed miscellaneous cleanups? I'm specifically
looking at the CleanerCleanable class. And I'm not sure why the
thread's constructor argument is being changed, requiring CleanerImpl
to implement Runnable.

-- 
src/java.base/share/classes/sun/nio/fs/NativeBuffer.java
  76 Cleaner.Cleanable cleanable() {
  77 return cleanable;
  78 }

[pre-existing, but if we're changing things anyway...]

I'm kind of surprised by an accessor function (both here and in the
original code) rather than a cleanup function.  Is there a use case
for anything other than buffer.cleanable().clean()?

Similarly for the DirectBuffer interface.

--

Re: We don't need jdk.internal.ref.Cleaner any more

[Kim Barrett]

> On Feb 8, 2016, at 5:46 AM, Peter Levart  wrote:
> If special-casing in ReferenceHandler is removed then an opportunity opens to 
> get rid of ReferenceHandler thread altogether. VM/GC could enqueue 
> Reference(s) to their respective ReferenceQueue(s) directly, so everyone 
> could enjoy the benefits sun.misc.Cleaner has…

I don't think that's likely.

That would require the VM/GC to acquire all the queue locks, and we
certainly wouldn't want to do that in a STW pause, so an intermediate
thread is kind of unavoidable.  [GC already needs to acquire the
pending list lock, for interaction with the reference queue thread.]

[Or change to lock-free enqueuing and wakeup of waiters, but I think
the current preference would be to avoid pulling even something like
that into a STW phase.]

Re: We don't need jdk.internal.ref.Cleaner any more

[Mandy Chung]

> On Feb 15, 2016, at 9:24 PM, David Holmes  wrote:
> 
>> This patch looks correct to me.  The innocuous thread created for common 
>> cleaner is of Thread.MAX_PRIORITY - 2 priority whereas the reference handler 
>> thread is of MAX_PRIORITY priority.   I don’t know if this would impact any 
>> real-world application or whether worth having a dedicated thread with 
>> MAX_PRIORTY (David Holmes may have recommendation to this).
> 
> How did you know I would read this? :)

Magic ball :)

> 
> Thread priority has no meaning on Solaris, Linux or BSD/OSX by default.
> 
> Only Windows actually applies thread priorities under normal conditions. The 
> difference between MAX_PRIORITY and MAX_PRIORITY-2 is the former uses 
> THREAD_PRIORITY_HIGHEST and the latter THREAD_PRIORITY_ABOVE_NORMAL. Without 
> any real/reasonable workloads for benchmarking it is all somewhat arbitrary. 
> Arguably the Reference handler thread has more work to do in general so might 
> be better given the higher priority.


Thanks that’s useful. I never spent the time to look at the implementation.

Mandy

Re: JDK 9 RFR of JDK-8149915: enabling validate-annotations feature for xsd schema with annotation causes NPE

[huizhe wang]

On 2/16/2016 2:13 PM, Langer, Christoph wrote:

Hi Joe,

thanks for your great suggestions. I'll come up with a new webrev tomorrow.

However, I'm still wondering if the change for the default value of 
XML_SECURITY_PROPERTY_MANAGER in XMLDocumentFragmentScannerImpl should be done 
anyway as I guess an XMLObject is more expected rather than a String from other 
places in the code that could make use of such a default...? Is that true?


You're right, the default value was incorrectly set to the default value 
of a property manager. "null" would be good in this case since there's 
no need to create a new instance here.


Best,
Joe



Best
Christoph

-Original Message-
From: huizhe wang [mailto:huizhe.w...@oracle.com]
Sent: Dienstag, 16. Februar 2016 21:03
To: Langer, Christoph 
Cc: core-libs-dev@openjdk.java.net
Subject: Re: JDK 9 RFR of JDK-8149915: enabling validate-annotations feature 
for xsd schema with annotation causes NPE

Hi Christoph,

At initialization of XSDHandler (in reset), note there are two local
variables for the security / property manager, change them to instance
variables, then when you need to create annotation validator, set both
on the validator, e.g.:

  fAnnotationValidator.setProperty(SECURITY_MANAGER,
fSecurityManager);
fAnnotationValidator.setProperty(XML_SECURITY_PROPERTY_MANAGER,
fSecurityPropertyMgr);

That should fix the issue.

There are a bunch of rawtypes/unchecked warnings in this class, bonus
but not required :-)

For the test, in the past (jaxp standalone), we've created tests for
each issue, nowadays we'd prefer to consolidate them. We don't have
"SchemaTest" yet, so you may name the test "SchemaTest", then for the
@summary, make it general, e.g. "Test Schema creation", and move the
specific comment to the test case and make it specific to the issue:

 /*
  * @bug 8149915
  * Verifies that the annotation validator is initialized with the security 
manager for schema
  * creation with http://apache.org/xml/features/validate-annotations=true.
  */

  @Test
  public void testValidation() throws Exception {


The test is new, the copyright year would be "2016," instead of "2014,
2016,".

Best,
Joe

On 2/16/2016 4:49 AM, Langer, Christoph wrote:

Hi,

can you please review the following change for JDK-8149915:

http://cr.openjdk.java.net/~clanger/webrevs/8149915.0/

I'm not sure if it is the right thing to set the default for 
XML_SECURITY_PROPERTY_MANAGER to a new XMLSecurityPropertyManager() object instead of the 
String "all". But it must not be a String, I think.

And also I don't know if my approach to add a field "fSecurityManager" to 
XML11Configuration.

Please advise and let me know how I can do it better.

Thanks in advance and best regards
Christoph

Re: [OpenJDK 2D-Dev] JDK 9 RFR of JDK-8149896: Remove unnecessary values in FloatConsts and DoubleConsts

[Jim Graham]

Laurent may not technically be a reviewer, but he pretty much owns that 
code so if he's happy then I'm happy (but I didn't look at any of the 
other code outside of Marlin if you are keeping count of "full 
reviewers"...).


...jim

On 2/16/2016 2:08 AM, Laurent Bourgès wrote:

Hello,

Joe, I tested your changes in the Marlin renderer as it works well !


> A quick note on the 2d changes, several constants (and a copy from a 
package-private method from java.lang) were used to initialize a double value 
POWER_2_TO_32; this can be accomplished more directly using a hexadecimal 
floating-point literal.
>
> Please review the webrev
>
>http://cr.openjdk.java.net/~darcy/8149896.0/
...
My favourite change is this one:

-private static final double POWER_2_TO_32 =
FloatMath.powerOfTwoD(32);
+private static final double POWER_2_TO_32 = 0x1.0p32;

and the removal of the method on FloatMath.


I agree it is simpler and exactly what I needed; I wasn't aware of such
literals:
https://blogs.oracle.com/darcy/entry/hexadecimal_floating_point_literals

(I am not a reviewer)

Thanks for the fix,
Laurent

RE: JDK 9 RFR of JDK-8149915: enabling validate-annotations feature for xsd schema with annotation causes NPE

[Langer, Christoph]

Hi Joe,

thanks for your great suggestions. I'll come up with a new webrev tomorrow.

However, I'm still wondering if the change for the default value of 
XML_SECURITY_PROPERTY_MANAGER in XMLDocumentFragmentScannerImpl should be done 
anyway as I guess an XMLObject is more expected rather than a String from other 
places in the code that could make use of such a default...? Is that true?

Best
Christoph

-Original Message-
From: huizhe wang [mailto:huizhe.w...@oracle.com]
Sent: Dienstag, 16. Februar 2016 21:03
To: Langer, Christoph 
Cc: core-libs-dev@openjdk.java.net
Subject: Re: JDK 9 RFR of JDK-8149915: enabling validate-annotations feature 
for xsd schema with annotation causes NPE

Hi Christoph,

At initialization of XSDHandler (in reset), note there are two local
variables for the security / property manager, change them to instance
variables, then when you need to create annotation validator, set both
on the validator, e.g.:

 fAnnotationValidator.setProperty(SECURITY_MANAGER,
fSecurityManager);
fAnnotationValidator.setProperty(XML_SECURITY_PROPERTY_MANAGER,
fSecurityPropertyMgr);

That should fix the issue.

There are a bunch of rawtypes/unchecked warnings in this class, bonus
but not required :-)

For the test, in the past (jaxp standalone), we've created tests for
each issue, nowadays we'd prefer to consolidate them. We don't have
"SchemaTest" yet, so you may name the test "SchemaTest", then for the
@summary, make it general, e.g. "Test Schema creation", and move the
specific comment to the test case and make it specific to the issue:

/*
 * @bug 8149915
 * Verifies that the annotation validator is initialized with the security 
manager for schema
 * creation with http://apache.org/xml/features/validate-annotations=true.
 */

 @Test
 public void testValidation() throws Exception {


The test is new, the copyright year would be "2016," instead of "2014,
2016,".

Best,
Joe

On 2/16/2016 4:49 AM, Langer, Christoph wrote:
> Hi,
>
> can you please review the following change for JDK-8149915:
>
> http://cr.openjdk.java.net/~clanger/webrevs/8149915.0/
>
> I'm not sure if it is the right thing to set the default for 
> XML_SECURITY_PROPERTY_MANAGER to a new XMLSecurityPropertyManager() object 
> instead of the String "all". But it must not be a String, I think.
>
> And also I don't know if my approach to add a field "fSecurityManager" to 
> XML11Configuration.
>
> Please advise and let me know how I can do it better.
>
> Thanks in advance and best regards
> Christoph
>

Re: RFR 9: 8149750 Decouple sun.misc.Signal from the base module

[Roger Riggs]

Hi David,

Webrev updated in place:
   http://cr.openjdk.java.net/~rriggs/webrev-signal-9149750/

On 2/14/2016 11:55 PM, David Holmes wrote:

Hi Roger,

A few mostly minor comments ...

On 13/02/2016 3:47 AM, Roger Riggs wrote:

Please review moving the functionality of sun.misc.Signal to
jdk.internal.misc and
creating a wrapper sun.misc.Signal for existing external uses.

+++ This time including the hotspot changes to update the target of the
upcalls.

A new replacement API will be considered separately.

The update includes a test that passes with or without the changes.
(Except for an NPE instead of SEGV if null is passed).

Webrev:
   jdk: http://cr.openjdk.java.net/~rriggs/webrev-signal-9149750/


I take it we don't care about reorder files any more?
There does not seem to be any particular guidance about how/when to use 
them on Solaris.

So it seemed as reasonable to remove the entries as to update them.


---

src/java.base/share/classes/sun/misc/Signal.java

The @since should not be changed to 9. You only changed the 
implementation not the API. Conversely the renamed class should 
arguably be @since 9 and not @since 1.2. But it is wrong to say 
sun.misc.Signal is @since 9.
The new sun.misc.Signal is a wrapper around the original implementation 
now in jdk.internal.misc.
I had modified the mercurial commands to rename sun.misc.Signal to 
jdk.internal.misc to preserve the history

of the implementation.

I will correct sun.misc.Signal to retain the original @since and use 
@since 9 for the jdk.internal.misc


The *Handler.of method name reads strangely to me - "for" would be 
better but I presume that is not allowed.

The xx.of ()  pattern has been used recently.


---

src/java.base/share/classes/jdk/internal/misc/Signal.java

Not sure I understand the role of NativeSignalHandler any more - given 
it can't actually be used ??
They are used to retain the previous handler and can be used to restore 
that handler.

The ability to invoke the handler directly was removed as a simplification.


---

test/sun/misc/SunMiscSignalTest.java

Based on our email discussion this test can not be testing that the 
handler actually gets invoked - as we know it does not in some cases. 
I have doubts that sun.misc.Signal ever intended to support all the 
signals you are testing.
There were no tests for sun.misc.Signal.  I created the tests based on 
the current implementations.
They can be updated to reflect current support for handle, raise, and 
whether a signal handler is called
and of course -Xrs.  It should allow detection of unintended changes in 
behavior.





hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-9149750/


Hotspot changes are fine.


Thanks, Roger


Thanks,
David
-


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

Thanks, Roger

JEP 260: https://bugs.openjdk.java.net/browse/JDK-8132928

Re: RFR 9: 8149750 Decouple sun.misc.Signal from the base module

[Roger Riggs]

Hi Chris,

Webrev updated in place:
   http://cr.openjdk.java.net/~rriggs/webrev-signal-9149750/

On 2/15/2016 6:22 AM, Chris Hegarty wrote:

On 12/02/16 17:47, Roger Riggs wrote:

Please review moving the functionality of sun.misc.Signal to
jdk.internal.misc and
creating a wrapper sun.misc.Signal for existing external uses.

+++ This time including the hotspot changes to update the target of the
upcalls.

A new replacement API will be considered separately.

The update includes a test that passes with or without the changes.
(Except for an NPE instead of SEGV if null is passed).

Webrev:
   jdk: http://cr.openjdk.java.net/~rriggs/webrev-signal-9149750/


Overall looks ok, and satisfies the requirement of JEP 260.

It took me a while to satisfy myself that it is ok to "ignore"
the passed Signal in the wrapper's 'handle' methods. The assumption
is that this wrapper's signal field and the passes signal are, MUST,
represent the same underlying signal. Maybe an assert to make this
explicit?

The jdk.internal.misc.Signal passed to the wrapper's methods needs to be
mapped to the corresponding sun.misc.Signal but instead of maintaining a map
the s.m.Signal instance is kept in the wrapper of the s.m.SignalHandler.


Looking at j.i.m.Signal., I can see that you explicitly check
for the 'SIG' prefix and prepend it if not there, but toString() also
prepends it. Will getName also be impacted by this ( it may now have
the name prepended with 'SIG', where it previously was not ).

Yes, there was a bug there; reverting to backward compatible behavior.
HotSpot recognizes different forms of signal names on Windows and 
Posix.  [1]
To be compatible with previous versions, the "SIG" prefix allowed by the 
HotSpot change

(only on Posix) is not supported and throws IllegalArgumentException.

Thanks, Roger

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




hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-9149750/


Looks fine.

-Chris.

Re: RFR: 8072727 - add variation of Stream.iterate() that's finite

[Brian Goetz]

Regarding wildcards: you definitely want Predicte, which is 
what we use elsewhere.  For UnaryOperator, you're stuck with invariance.


On 2/16/2016 8:32 AM, Paul Sandoz wrote:

Hi Tagir,

The implementation approach looks good. I agree with Brian on the 
documentation, esp. about the connection to a for loop.

Testing-wise adding to the spliterator data sources is ok. Because you have 
done that there is no need to explicitly use SpliteratorTestHelper in 
IterateTest, as that is reasonably covered by SpliteratorTest. Since you have 
the data providers for streams just run it through exerciseStream instead.

Regarding wild cards we could clean this up later and include other methods 
[1]. It’s easy to go overboard on this, recommend sticking to ? extends/? super 
where appropriate.  The existing iterate methods use UnaryOperator, thus there 
is no wiggle room there, am i ok with reusing that for the 3-arg method.

Regarding the existing two-args method, my inclination is to keep the implementations 
separate from the 3-args, but feel free to change to using abstract spliterator (reduce 
layering) using a similar code pattern (no need for "finished" or overriding 
forEachRemaining). Then you can remove Streams.NONE and it’s sneaky erased usage :-)

Paul.

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


On 14 Feb 2016, at 15:53, Tagir F. Valeev  wrote:

Hello!

I wanted to work on foldLeft, but Brian asked me to take this issue
instead. So here's webrev:
http://cr.openjdk.java.net/~tvaleev/webrev/8072727/r1/

I don't like iterator-based Stream source implementations, so I made
them AbstractSpliterator-based. I also implemented manually
forEachRemaining as, I believe, this improves the performance in
non-short-circuiting cases.

I also decided to keep two flags (started and finished) to track the
state. Currently existing implementation of infinite iterate() does
not use started flag, but instead reads one element ahead for
primitive streams. This seems wrong to me and may even lead to
unexpected exceptions (*). I could get rid of "started" flag for
Stream.iterate() using Streams.NONE, but this would make object
implementation different from primitive implementations. It would also
be possible to keep single three-state variable (byte or int,
NOT_STARTED, STARTED, FINISHED), but I doubt that this would improve
the performance or footprint. Having two flags looks more readable to
me.

Currently existing two-arg iterate methods can now be expressed as a
partial case of the new method:

public static Stream iterate(final T seed, final UnaryOperator f) {
return iterate(seed, x -> true, f);
}
(same for primitive streams). I may do this if you think it's
reasonable.

I created new test class and added new iterate sources to existing
data providers.

Please review and sponsor!

With best regards,
Tagir Valeev.

(*) Consider the following code:

int[] data = {1,2,3,4,-1};
IntStream.iterate(0, x -> data[x])
 .takeWhile(x -> x >= 0)
 .forEach(System.out::println);

Currently this unexpectedly throws an AIOOBE, because
IntStream.iterate unnecessarily tries to read one element ahead.

Re: RFR (S) 8149835: StringConcatFactory should emit classes with the same package as the host class

[Andrej Golovnin]

Hi Aleksey,

>  http://cr.openjdk.java.net/~shade/8149835/webrev.jdk.01/

701 return (pkg != null ? pkg.getName().replace(".", "/") + 
"/" : "") + "Stubs$$StringConcat";
702 } else {
703 return hostClass.getName().replace(".", "/") + 
"$$StringConcat”;

Maybe you should use here the character based String#replace()-method as it is 
faster and
does not produce as much garbage as the CharSequence based method does.

Best regards,
Andrej Golovnin

Re: RFR[9] 8068686: Remove meta-index support

[Mandy Chung]

On Feb 16, 2016, at 11:19 AM, Chris Hegarty  wrote:
> 
> The Modular Run-Time image, integrated in b41, no longer has JAR files,
> see JEP 220 [1], The meta-index is no longer generated or used. This
> issue proposes to remove the meta-index support from the runtime and
> the build.
> 
> http://cr.openjdk.java.net/~chegar/8068686/
> https://bugs.openjdk.java.net/browse/JDK-8068686

Looks good.

I assume at some point the classlist under jdk/make/tools/classlist will be 
updated.  They currently reference several sun.misc types that you have removed 
or renamed.

Mandy

RFR (S) 8149835: StringConcatFactory should emit classes with the same package as the host class

[Aleksey Shipilev]

Hi,

Please review a StringConcatFactory fix that makes generated String
concat stubs to be named "$$StringConcat/", instead of
"java/lang/String$$Concat/":
  https://bugs.openjdk.java.net/browse/JDK-8149835

Among other things, this helps to dodge the failure caught by stricter
access controls in Jake (JDK-8149165), and hopefully caters for future
stricter Unsafe.defineAnonymousClass access controls (JDK-8058575).

Webrevs:
  http://cr.openjdk.java.net/~shade/8149835/webrev.jdk.01/
  http://cr.openjdk.java.net/~shade/8149835/webrev.langtools.01/

Testing: JPRT, java/lang/String; + copying SCF code to jake forest and
running with it.

Cheers,
-Aleksey

Re: JDK 9 RFR of JDK-8149915: enabling validate-annotations feature for xsd schema with annotation causes NPE

[huizhe wang]

Hi Christoph,

At initialization of XSDHandler (in reset), note there are two local 
variables for the security / property manager, change them to instance 
variables, then when you need to create annotation validator, set both 
on the validator, e.g.:


fAnnotationValidator.setProperty(SECURITY_MANAGER, 
fSecurityManager);
fAnnotationValidator.setProperty(XML_SECURITY_PROPERTY_MANAGER, 
fSecurityPropertyMgr);


That should fix the issue.

There are a bunch of rawtypes/unchecked warnings in this class, bonus 
but not required :-)


For the test, in the past (jaxp standalone), we've created tests for 
each issue, nowadays we'd prefer to consolidate them. We don't have 
"SchemaTest" yet, so you may name the test "SchemaTest", then for the 
@summary, make it general, e.g. "Test Schema creation", and move the 
specific comment to the test case and make it specific to the issue:


   /*
* @bug 8149915
* Verifies that the annotation validator is initialized with the security 
manager for schema
* creation with http://apache.org/xml/features/validate-annotations=true.
*/

@Test
public void testValidation() throws Exception {


The test is new, the copyright year would be "2016," instead of "2014, 
2016,".


Best,
Joe

On 2/16/2016 4:49 AM, Langer, Christoph wrote:

Hi,

can you please review the following change for JDK-8149915:

http://cr.openjdk.java.net/~clanger/webrevs/8149915.0/

I'm not sure if it is the right thing to set the default for 
XML_SECURITY_PROPERTY_MANAGER to a new XMLSecurityPropertyManager() object instead of the 
String "all". But it must not be a String, I think.

And also I don't know if my approach to add a field "fSecurityManager" to 
XML11Configuration.

Please advise and let me know how I can do it better.

Thanks in advance and best regards
Christoph

RFR[9] 8068686: Remove meta-index support

[Chris Hegarty]

The Modular Run-Time image, integrated in b41, no longer has JAR files,
see JEP 220 [1], The meta-index is no longer generated or used. This
issue proposes to remove the meta-index support from the runtime and
the build.

http://cr.openjdk.java.net/~chegar/8068686/
https://bugs.openjdk.java.net/browse/JDK-8068686

-Chris.

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

Re: JDK 9 RFR of JDK-8149981: java/util/Formatter/Basic.java fails after JDK-8149896

[Brian Burkhalter]

+2

On Feb 16, 2016, at 10:59 AM, Roger Riggs  wrote:

> +1
> 
> Roger
> 
> On 2/16/2016 1:35 PM, joe darcy wrote:
>> Hello,
>> 
>> *sigh* The one set of regression tests related in JDK-8149896 I forgot run 
>> run before pushing had a dependency on the removed constants in 
>> DoubleConsts. Please review this straightforward change to fix up the 
>> Formatter test template and regenerate the tests:
>> 
>>http://cr.openjdk.java.net/~darcy/8149981.0/
>> 
>> Actual semantic changes in
>> 
>>test/java/util/Formatter/Basic-X.java.template
>>test/java/util/Formatter/Basic.java
>>test/java/util/Formatter/BasicDouble.java
>> 
>> Thanks,
>> 
>> -Joe
> 

Re: JDK 9 RFR of JDK-8149981: java/util/Formatter/Basic.java fails after JDK-8149896

[Roger Riggs]

+1

Roger

On 2/16/2016 1:35 PM, joe darcy wrote:

Hello,

*sigh* The one set of regression tests related in JDK-8149896 I forgot 
run run before pushing had a dependency on the removed constants in 
DoubleConsts. Please review this straightforward change to fix up the 
Formatter test template and regenerate the tests:


http://cr.openjdk.java.net/~darcy/8149981.0/

Actual semantic changes in

test/java/util/Formatter/Basic-X.java.template
test/java/util/Formatter/Basic.java
test/java/util/Formatter/BasicDouble.java

Thanks,

-Joe

Re: [9] RFR: 8149923 invalid javadoc in javax.xml.bind.JAXBContext (introduced by 8138699)

[Lance Andersen]

Hi Miran

Looks OK though you might want to check the spacing on line 454 as it should 
probably be moved over but no additional review needed

Best
Lance
On Feb 16, 2016, at 10:42 AM, Miroslav Kos  wrote:

> Hi Lance,
> could you review this minor patch? It is just minor overlook I didn't catch 
> while doing 8138699.
> 
> Filed a new JBS issue: https://bugs.openjdk.java.net/browse/JDK-8149923
> webrev: http://cr.openjdk.java.net/~mkos/8149923/jaxws.01/
> 
> Thanks
> Miran
> 
> 
> 



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

JDK 9 RFR of JDK-8149981: java/util/Formatter/Basic.java fails after JDK-8149896

[joe darcy]

Hello,

*sigh* The one set of regression tests related in JDK-8149896 I forgot 
run run before pushing had a dependency on the removed constants in 
DoubleConsts. Please review this straightforward change to fix up the 
Formatter test template and regenerate the tests:


http://cr.openjdk.java.net/~darcy/8149981.0/

Actual semantic changes in

test/java/util/Formatter/Basic-X.java.template
test/java/util/Formatter/Basic.java
test/java/util/Formatter/BasicDouble.java

Thanks,

-Joe

Re: RFR: jsr166 jdk9 integration wave 4

[Martin Buchholz]

Finally integrated!  Nothing in the jsr166 queue at the moment.  (I'm
sure we'll fix that ...)

On Thu, Feb 11, 2016 at 2:56 AM, Chris Hegarty  wrote:
>
> On 11 Feb 2016, at 01:37, Martin Buchholz  wrote:
>
>> and ... webrev regenerated
>
> Thank you Martin, this is better.
>
> For completeness, the specdiff has been updated too:
>   
> http://cr.openjdk.java.net/~chegar/jsr166-jdk9-integration-CompletableFuture/CompletionStage.html
>
> -Chris.
>
>> On Wed, Feb 10, 2016 at 3:32 PM, Martin Buchholz  wrote:
>>> The specdiff is very helpful.  It was much easier to see that the
>>> cross-method links from whenComplete to handle could be improved, so
>>> we'll change (modulo reflow)
>>>
>>> --- src/main/java/util/concurrent/CompletionStage.java 24 Jan 2016
>>> 21:22:16 - 1.37
>>> +++ src/main/java/util/concurrent/CompletionStage.java 10 Feb 2016
>>> 23:29:49 -
>>> @@ -736,7 +736,7 @@
>>>  * {@code null} if none) of this stage as arguments.  The returned
>>>  * stage is completed when the action returns.
>>>  *
>>> - * Unlike method {@link #handle}, this method is not designed
>>> + * Unlike method {@link #handle handle}, this method is not designed
>>>  * to translate completion outcomes, so the supplied action should
>>>  * not throw an exception. However, if it does, the following
>>>  * rules apply: if this stage completed normally but the supplied
>>> @@ -762,7 +762,7 @@
>>>  * if none) of this stage as arguments.  The returned stage is completed
>>>  * when the action returns.
>>>  *
>>> - * Unlike method {@link #handle}, this method is not designed
>>> + * Unlike method {@link #handleAsync(BiFunction) handleAsync},
>>> this method is not designed
>>>  * to translate completion outcomes, so the supplied action should
>>>  * not throw an exception. However, if it does, the following
>>>  * rules apply: If this stage completed normally but the supplied
>>> @@ -788,7 +788,7 @@
>>>  * if none) of this stage as arguments.  The returned stage is completed
>>>  * when the action returns.
>>>  *
>>> - * Unlike method {@link #handle}, this method is not designed
>>> + * Unlike method {@link #handleAsync(BiFunction,Executor)
>>> handleAsync}, this method is not designed
>>>  * to translate completion outcomes, so the supplied action should
>>>  * not throw an exception. However, if it does, the following
>>>  * rules apply: If this stage completed normally but the supplied
>>>
>>> On Wed, Feb 10, 2016 at 2:51 PM, Chris Hegarty  
>>> wrote:

 On 10 Feb 2016, at 15:53, Martin Buchholz  wrote:

 Thanks for creating the specdiff, but ... it looks reversed; the green
 is the old and the red is the new!


 D’oh, of course. Updated in-place.

 http://cr.openjdk.java.net/~chegar/jsr166-jdk9-integration-CompletableFuture/CompletionStage.html

 Sorry for our "endless fiddling"; we do have future changes in mind,
 but no spec changes.


 No problem.

 -Chris.

 On Wed, Feb 10, 2016 at 2:50 AM, Chris Hegarty 
 wrote:

 On 02/02/16 15:23, Martin Buchholz wrote:


 On Tue, Feb 2, 2016 at 6:37 AM, Chris Hegarty 
 wrote:



 On 1 Feb 2016, at 18:45, Martin Buchholz  wrote:

 After much debate on what to do when CompleteableFuture.whenComplete
 encounters an exception in both the source and the action, we chose
 what was acceptable to the most people - add the action's exception to
 the source exception as a suppressed exception.  And added usage
 guidelines.  And gave handle "top billing" over whenComplete.


 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/



 This all looks fine to me.

 So I assume you only need a small CCC request for CompletionStage, right?
 Everything else is implementation.



 If you squint you might argue that CompletionStage's contract hasn't
 actually changed,
 but yeah, go ahead and do a CCC for CompletionStage.  Publishing a
 specdiff would be nice - method reordering (for "top billing") has
 made the diffs harder to review.  Thanks.



 Here are the specdiffs that will be used for the CCC, unless there are
 any last minute changes.

 http://cr.openjdk.java.net/~chegar/jsr166-jdk9-integration-CompletableFuture/CompletionStage.html

 -Chris.


>

Re: RFR 7183985: Class.getAnnotation() throws an ArrayStoreException when the annotation class not present

[Joel Borggrén-Franck]

Hi Liam,

Sorry for the delay,

On Sat, 30 Jan 2016 at 04:45 Liam Miller-Cushon  wrote:

> On Thu, Jan 28, 2016 at 11:50 AM, Joel Borggrén-Franck <
> joel.borggren.fra...@gmail.com> wrote:
>
> But, the reason we didn't fix this the last two times we looked at it
>> (that I am aware of) is the compatibility story. In order to fix his
>> you need to figure out two things:
>>
>> - Is this is a spec change, that is, can we get away with throwing an
>> AnnotationFormatError where we would now do? Should we clarify the
>> spec?
>>
>
> Can you clarify which parts of the spec might need to be updated? I can't
> find anything relevant in the jls or jvms. The javadoc for AnnotatedElement
> mentions some conditions under which TypeNotPresentException is thrown:
>
> "If an annotation returned by a method in this interface contains
> (directly or indirectly) a Class-valued member referring to a class that is
> not accessible in this VM, attempting to read the class by calling the
> relevant Class-returning method on the returned annotation will result in a
> TypeNotPresentException."
>
> So throwing TypeNotPresentException when an array-valued annotation
> indirectly references an inaccessible class sounds like the right
> behaviour, and is consistent with how the implementation currently handles
> similar cases.
>

I think you answered my concerns. By the spec I meant either the Java
Language Specification or the normative parts of the javadoc. This seems to
be in line with what is currently specified.


> - Since this is a behaviorally incompatible change, how big is the
>> impact? This is of course a hard question to answer, but if one could
>> do a corpus analysis over a large code base and look for catches of
>> ArrayStoreExceptions when reflecting over annotations, that could be
>> useful. If it turns out that "a lot" of users have adopted to this
>> bug, perhaps it isn't worth fixing? On the other hand I can imagine
>> that this is so uncommon that no one catches either type of error.
>>
>
> I'm working on evaluating the impact. A review of our code base showed
> that handling ArrayStoreException is fairly uncommon. Of the instances I
> found, none of them were in code that was inspecting annotations.
>
> The next step is to start using the patch internally and see if anything
> breaks. I'll update the thread when I have results on that.
>

Great, if the experiment works out I'll help formulate a change request for
compatibility review.

cheers
/Joel

Re: [OpenJDK 2D-Dev] JDK 9 RFR of JDK-8149896: Remove unnecessary values in FloatConsts and DoubleConsts

[joe darcy]

Hi Laurent,

On 2/16/2016 2:08 AM, Laurent Bourgès wrote:

Hello,

Joe, I tested your changes in the Marlin renderer as it works well !


> A quick note on the 2d changes, several constants (and a copy
from a package-private method from java.lang) were used to
initialize a double value POWER_2_TO_32; this can be accomplished
more directly using a hexadecimal floating-point literal.
>
> Please review the webrev
>
> http://cr.openjdk.java.net/~darcy/8149896.0/

...
My favourite change is this one:

-private static final double POWER_2_TO_32 =
FloatMath.powerOfTwoD(32);
+private static final double POWER_2_TO_32 = 0x1.0p32;

and the removal of the method on FloatMath.


I agree it is simpler and exactly what I needed; I wasn't aware of 
such literals:

https://blogs.oracle.com/darcy/entry/hexadecimal_floating_point_literals




Most people don't have cause to use hexadecimal floating-point literals 
very often, but when you need them, they are exactly what you need :-)


Thanks for the review,

-Joe

Re: JDK 9 RFR of JDK-8149896: Remove unnecessary values in FloatConsts and DoubleConsts

[Mike Duigou]

From: joe darcy 
Hello,

The the FloatConsts and DoubleConsts classes, while moved to an 
internal

package recently (JDK-8145990), contain constants now available via the
public API. All such uses of the redundant values should be removed as
well as the redundant constants themselves.

 http://cr.openjdk.java.net/~darcy/8149896.0/


The changes look good to me and it is very tempting to suggest just 
retiring DoubleConstants and FloatConstants altogether and move the 
remaining quite useful constants in to Double and Float.


Mike

RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

[Peter Levart]

Hello everybody,

Thanks for looking into this and for all your comments. Now that it 
seems we have a consensus that replacing internal Cleaner with public 
Cleaner is not a wrong idea, I created an issue for it [1] so that we 
may officially review the implementation. I also created an updated 
webrev [2] which fixes some comments in usages that were not correct any 
more after the change. I also took the liberty to modify the 
CleanerImpl.InnocuousThreadFactory to give names to threads in 
accordance to the style used in some other thread factories (Timer-0, 
Timer-1, ..., Cleaner-0, Cleaner-1, ..., etc.). This gives the thread of 
internal Cleaner instance, which is now constructed as part of the 
boot-up sequence, a stable name: "Cleaner-0".


If you feel that internal Cleaner instance should have a Thread with 
MAX_PRIORITY, I can incorporate that too.


[1] https://bugs.openjdk.java.net/browse/JDK-8149925
[2] 
http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.03/


I re-ran the java/lang/ref and java/nio tests and all pass except two 
ignored tests:


java/nio/Buffer/LimitDirectMemory.sh: Test option to limit direct memory 
allocation
java/nio/file/spi/SetDefaultProvider.java: Unit test for 
java.nio.file.spi.FileSystemProvider


and the following two, which seem to not like my network config. or 
something like that:


java/nio/channels/DatagramChannel/MulticastSendReceiveTests.java: Unit 
test for DatagramChannel's multicast support
java/nio/channels/DatagramChannel/Promiscuous.java: Test for 
interference when two sockets are bound to the same port but joined to 
different multicast groups



Regards, Peter


On 02/16/2016 03:02 PM, Chris Hegarty wrote:

On 15 Feb 2016, at 20:05, Mandy Chung  wrote:


On Feb 15, 2016, at 9:06 AM, Uwe Schindler  wrote:

Hi,


On 15/02/2016 14:57, Chris Hegarty wrote:

Peter,

http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.02/


This patch looks correct to me.  The innocuous thread created for common 
cleaner is of Thread.MAX_PRIORITY - 2 priority whereas the reference handler 
thread is of MAX_PRIORITY priority.

I see your point, but I don’t see this as an issue because threads failing to
allocate memory themselves get involved in cleaning. So pressure is
somewhat off the reference handler thread.

-Chris.

[9] RFR: 8149923 invalid javadoc in javax.xml.bind.JAXBContext (introduced by 8138699)

[Miroslav Kos]

Hi Lance,
could you review this minor patch? It is just minor overlook I didn't 
catch while doing 8138699.


Filed a new JBS issue: https://bugs.openjdk.java.net/browse/JDK-8149923
webrev: http://cr.openjdk.java.net/~mkos/8149923/jaxws.01/

Thanks
Miran

Re: We don't need jdk.internal.ref.Cleaner any more

[Chris Hegarty]

On 15 Feb 2016, at 20:05, Mandy Chung  wrote:

> 
>> On Feb 15, 2016, at 9:06 AM, Uwe Schindler  wrote:
>> 
>> Hi,
>> 
>>> On 15/02/2016 14:57, Chris Hegarty wrote:
 Peter,
 
 http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.02/
 
> 
> This patch looks correct to me.  The innocuous thread created for common 
> cleaner is of Thread.MAX_PRIORITY - 2 priority whereas the reference handler 
> thread is of MAX_PRIORITY priority.

I see your point, but I don’t see this as an issue because threads failing to
allocate memory themselves get involved in cleaning. So pressure is
somewhat off the reference handler thread.

-Chris.

Re: RFR: 8072727 - add variation of Stream.iterate() that's finite

[Paul Sandoz]

Hi Tagir,

The implementation approach looks good. I agree with Brian on the 
documentation, esp. about the connection to a for loop.

Testing-wise adding to the spliterator data sources is ok. Because you have 
done that there is no need to explicitly use SpliteratorTestHelper in 
IterateTest, as that is reasonably covered by SpliteratorTest. Since you have 
the data providers for streams just run it through exerciseStream instead.

Regarding wild cards we could clean this up later and include other methods 
[1]. It’s easy to go overboard on this, recommend sticking to ? extends/? super 
where appropriate.  The existing iterate methods use UnaryOperator, thus there 
is no wiggle room there, am i ok with reusing that for the 3-arg method.

Regarding the existing two-args method, my inclination is to keep the 
implementations separate from the 3-args, but feel free to change to using 
abstract spliterator (reduce layering) using a similar code pattern (no need 
for "finished" or overriding forEachRemaining). Then you can remove 
Streams.NONE and it’s sneaky erased usage :-)

Paul.

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

> On 14 Feb 2016, at 15:53, Tagir F. Valeev  wrote:
> 
> Hello!
> 
> I wanted to work on foldLeft, but Brian asked me to take this issue
> instead. So here's webrev:
> http://cr.openjdk.java.net/~tvaleev/webrev/8072727/r1/
> 
> I don't like iterator-based Stream source implementations, so I made
> them AbstractSpliterator-based. I also implemented manually
> forEachRemaining as, I believe, this improves the performance in
> non-short-circuiting cases.
> 
> I also decided to keep two flags (started and finished) to track the
> state. Currently existing implementation of infinite iterate() does
> not use started flag, but instead reads one element ahead for
> primitive streams. This seems wrong to me and may even lead to
> unexpected exceptions (*). I could get rid of "started" flag for
> Stream.iterate() using Streams.NONE, but this would make object
> implementation different from primitive implementations. It would also
> be possible to keep single three-state variable (byte or int,
> NOT_STARTED, STARTED, FINISHED), but I doubt that this would improve
> the performance or footprint. Having two flags looks more readable to
> me.
> 
> Currently existing two-arg iterate methods can now be expressed as a
> partial case of the new method:
> 
> public static Stream iterate(final T seed, final UnaryOperator f) {
>return iterate(seed, x -> true, f);
> }
> (same for primitive streams). I may do this if you think it's
> reasonable.
> 
> I created new test class and added new iterate sources to existing
> data providers.
> 
> Please review and sponsor!
> 
> With best regards,
> Tagir Valeev.
> 
> (*) Consider the following code:
> 
> int[] data = {1,2,3,4,-1};
> IntStream.iterate(0, x -> data[x])
> .takeWhile(x -> x >= 0)
> .forEach(System.out::println);
> 
> Currently this unexpectedly throws an AIOOBE, because
> IntStream.iterate unnecessarily tries to read one element ahead.
> 

JDK 9 RFR of JDK-8149915: enabling validate-annotations feature for xsd schema with annotation causes NPE

[Langer, Christoph]

Hi,

can you please review the following change for JDK-8149915:

http://cr.openjdk.java.net/~clanger/webrevs/8149915.0/

I'm not sure if it is the right thing to set the default for 
XML_SECURITY_PROPERTY_MANAGER to a new XMLSecurityPropertyManager() object 
instead of the String "all". But it must not be a String, I think.

And also I don't know if my approach to add a field "fSecurityManager" to 
XML11Configuration.

Please advise and let me know how I can do it better.

Thanks in advance and best regards
Christoph

Re: [9] RFR (XS): 8148518: Unsafe.getCharUnaligned() loads aren't folded in case of -XX:-UseUnalignedAccesses

[Vladimir Ivanov]

Vladimir, Aleksey, thanks for the reviews.


Current Unsafe::getCharUnaligned Java implementation isn't fully
optimized by C2: it never constant folds such loads from char fields.
C2 matches the types of a load and a location and does constant folding
only if they match. For getCharUnaligned it sees a load of a short
value, but the field is of type char.


Does the same affect putCharUnaligned?

No, the logic is specific to unsafe loads.

Best regards,
Vladimir Ivanov

RE: DeserializationPermission Proposal

[Peter Firmstone]

Hi Dave,

You've pretty much got it right, although I'm not proposing making the 
permissions fine grained to class name level.


There are different DeSerializationPermission's however:

"PROXY" - controls dynamic proxy creation (Some basic invariants are 
checked prior, the InvocationHandler field object is passed to the 
dynamic proxy's constructor).
"ATOMIC" - controls which domains are allowed to use atomic 
deserialization constructor.
"MARSHAL" - allows deserialization of MarshalledObject instances (Again 
invariants are checked prior).

"EXTERNAL" - allows deserialization of or Externalizable objects.

Serializable classes that are stateless or have only primitive fields 
don't require any permission and remain unchanged.


Serializable classes that contain Object fields or reads information 
directly from the stream require an atomic constructor, identified with 
@AtomicSerial, otherwise they are not Serializable.


There is also an atomic constructor available for Externalizable 
objects, identified with @AtomicExternal, however this isn't required.


AtomicMarshalInputStream also provides a  T readObject(Class type) 
method that reads ahead and prevents deserialization if the object isn't 
the correct type. There's another annotation for readResolve methods to 
identify the replacement object type, as readResolve doesn't allow 
covariant return types.



Some debugging output:

NonActGrp-out: access: access allowed ("java.util.PropertyPermission" 
"sun.net.maxDatagramSockets" "read")
NonActGrp-out: access: access allowed ("java.net.SocketPermission" 
"localhost:4160" "listen,resolve")
NonActGrp-out: access: access allowed ("java.net.SocketPermission" 
"224.0.1.84" "connect,accept,resolve")
NonActGrp-out: access: access allowed ("java.io.FilePermission" 
"\C:\Users\peter\Documents\NetBeansProjects\input-validation-for-serialization\lib\mahalo.jar" 
"read")
NonActGrp-out: access: access allowed ("java.lang.RuntimePermission" 
"accessClassInPackage.sun.reflect")
NonActGrp-out: access: access allowed ("java.net.SocketPermission" 
"localhost:0" "listen,resolve")
NonActGrp-out: access: access allowed ("java.lang.RuntimePermission" 
"accessDeclaredMembers")
NonActGrp-out: access: access allowed 
("net.jini.discovery.DiscoveryPermission" 
"QATestDefaultGroup_medusa_1455617825772")
NonActGrp-out: access: access allowed ("java.io.SerializablePermission" 
"enableSubclassImplementation")
NonActGrp-out: access: access allowed ("java.io.SerializablePermission" 
"enableSubstitution")

NonActGrp-out: access: AccessControlContext invoking the Combiner
NonActGrp-out: access: access allowed 
("org.apache.river.api.io.DeSerializationPermission" "ATOMIC")

NonActGrp-out: access: AccessControlContext invoking the Combiner
NonActGrp-out: access: access allowed 
("org.apache.river.api.io.DeSerializationPermission" "PROXY")

NonActGrp-out: access: AccessControlContext invoking the Combiner
NonActGrp-out: access: access allowed 
("org.apache.river.api.io.DeSerializationPermission" "ATOMIC")
NonActGrp-out: access: access allowed ("java.lang.RuntimePermission" 
"createSecurityManager")
NonActGrp-out: access: access allowed 
("org.apache.river.api.io.DeSerializationPermission" "ATOMIC")
NonActGrp-out: access: access allowed ("java.io.SerializablePermission" 
"enableSubclassImplementation")
NonActGrp-out: access: access allowed ("java.io.SerializablePermission" 
"enableSubstitution")


Regards,

Peter.


Things are getting confused (for me anyway) so I'm going to try and
spell out each idea separately.

Given a class hierarchy H of:

C ->  B ->  A ->  NonSerializableClass ->  Object

where C, B, and A are serializable classes, and protection domain M
which is initiating deserialization, I think the following requirements
definitely apply:

* When M deserializes an instance of A, both M and the protection domain
of A must be sufficient to allow the deserialization of A
* When M deserializes an instance of B, both M and the protection domain
of B must be sufficient to allow the deserialization of B
* When M deserializes an instance of C, both M and the protection domain
of C must be sufficient to allow the deserialization of C
* Extrapolate generally

The following requirements have definite pros and cons, which is what I
think we're touching on in this thread:

* When M deserializes an instance of B (or any subtype thereof), the
protection domain of A must also be sufficient to allow the
deserialization of B
* When M deserializes an instance of C (or any subtype thereof), the
protection domains of both A and B must also be sufficient to allow the
deserialization of C
* Extrapolate generally

Based on the above, the following requirements might or might not make
sense:

* When M deserializes an instance of A, the protection domains of
NonSerializableClass and also Object must also be sufficient to allow
the deserialization of A
* Likewise for B&  C, extrapolate generally

Regardless of which is the best set of requirements, 

Re: [OpenJDK 2D-Dev] JDK 9 RFR of JDK-8149896: Remove unnecessary values in FloatConsts and DoubleConsts

[Laurent Bourgès]

Hello,

Joe, I tested your changes in the Marlin renderer as it works well !


> A quick note on the 2d changes, several constants (and a copy from a
> package-private method from java.lang) were used to initialize a double
> value POWER_2_TO_32; this can be accomplished more directly using a
> hexadecimal floating-point literal.
> >
> > Please review the webrev
> >
> >http://cr.openjdk.java.net/~darcy/8149896.0/
> ...
> My favourite change is this one:
>
> -private static final double POWER_2_TO_32 = FloatMath.powerOfTwoD(32);
> +private static final double POWER_2_TO_32 = 0x1.0p32;
>
> and the removal of the method on FloatMath.
>

I agree it is simpler and exactly what I needed; I wasn't aware of such
literals:
https://blogs.oracle.com/darcy/entry/hexadecimal_floating_point_literals

(I am not a reviewer)

Thanks for the fix,
Laurent

Re: JDK 9 RFR of JDK-8149896: Remove unnecessary values in FloatConsts and DoubleConsts

[Paul Sandoz]

> On 16 Feb 2016, at 07:23, joe darcy  wrote:
> 
> Hello,
> 
> The the FloatConsts and DoubleConsts classes, while moved to an internal 
> package recently (JDK-8145990), contain constants now available via the 
> public API. All such uses of the redundant values should be removed as well 
> as the redundant constants themselves.
> 
> A quick note on the 2d changes, several constants (and a copy from a 
> package-private method from java.lang) were used to initialize a double value 
> POWER_2_TO_32; this can be accomplished more directly using a hexadecimal 
> floating-point literal.
> 
> Please review the webrev
> 
>http://cr.openjdk.java.net/~darcy/8149896.0/
> 

+1

My favourite change is this one:

-private static final double POWER_2_TO_32 = FloatMath.powerOfTwoD(32);
+private static final double POWER_2_TO_32 = 0x1.0p32;

and the removal of the method on FloatMath.

Paul.

Re: JDK 9 RFR of JDK-8149896: Remove unnecessary values in FloatConsts and DoubleConsts

[Aleksey Shipilev]

On 02/16/2016 09:23 AM, joe darcy wrote:
> Please review the webrev
> 
> http://cr.openjdk.java.net/~darcy/8149896.0/

+1, nice cleanup.

-Aleksey
(not a Reviewer)