Re: [1] RFR(XXS): 8197927: Can't set mandatory 'java.vendor.version' property to empty string

[Volker Simonis]

On Fri, Feb 16, 2018 at 8:17 PM,  wrote:

> 2018/2/16 10:59:57 -0800, volker.simo...@gmail.com:
> > On Fri, Feb 16, 2018 at 7:02 PM, mark.reinh...@oracle.com wrote:
> >> Of course it's possible.  The specification need merely say that
> >> `java.vendor.version` is a standard system property that may, or may
> >> not, have a value.  (Or, if you like, whose value may be `null`.)
> >
> > Sorry but I still don't get it. Do you agree that you can't assign NULL
> to
> > a system property because you'll get a NPE?
>
> I agree that `System.setProperty("java.vendor.version", null)` will
> throw an NPE, but I don't see how this fact is relevant.
>
> > You could of course not assign it at all (as it is done now) in which
> case
> > System.getProperty("java.vendor.version") would return NULL. But that
> means
> > "it is not defined" which is different from "is has no value".
>
> Let's not get caught up in fine philosophical distinctions between "not
> defined", "has no value", and `null`.  Either `System::getProperty`
> returns a non-`null` value, or it doesn't.  That's all that matters.
>
> >You can
> > still call System.getProperties().containsKey​("java.vendor.version")
> and
> > it would return false which violates that specification because it
> mandates
> > that a property with the "java.vendor.version" exists.
>
> The current specification mandates this.  That's precisely the bug here.
> We can revise it so that it doesn't.
>
>
Re-targeted to 11 with priority P2 and reassigned to you.

Thanks,
Volker


> - Mark
>

Re: FileOutputStream.available() and pipe EOF

[Basin Ilya]

>> or instead of relying on an undocumented behaviour
Fine. Isn't the goal of Java to behave equally on various OSes? If there's a 
difference, we should adopt the best variant AND document it. And in this case 
it is to throw
an exception on EOF. I don't know about Solaris, but on Windows it's a matter 
of what to do when PeekNamedPipe() returns EOF. We can even add a JVM option to 
restore the
old behavior, like it was with String.split().


On 11.02.2018 0:35, Basin Ilya wrote:
> Unfortunately, read() is
> 1) uninterruptilbe
> 2) Unlike sockets, close() or even Thread.stop() won't cancel a read
> pipe operation on Windows
> 
> 
> 11.02.2018 0:27, Remi Forax пишет:
>> Hi Basin,
>> or instead of relying on an undocumented behaviour, you can use any 
>> overloads of read(), if it returns -1, it's the ends of the stream.
>>
>> cheers,
>> Rémi
>>
>> - Mail original -
>>> De: "Basin Ilya" 
>>> À: "core-libs-dev" 
>>> Envoyé: Samedi 10 Février 2018 22:15:18
>>> Objet: FileOutputStream.available() and pipe EOF
>>
>>> Hi list.
>>>
>>> My question relates to streams returned by
>>> java.lang.Process.getInputStream()
>>>
>>> On Linux calling available() after the other side of the pipe was closed
>>> will throw:
>>>
>>> java.io.IOException: Stream Closed
>>>
>>> This is very handy, because you can distinguish EOF and a pause in
>>> transmission.
>>>
>>> On Windows same Oracle JDK version 1.8.0_161 behaves in a traditional
>>> manner and available() returns 0 in both cases. Will this ever change?

Re: [PATCH] RFR Bug-pending: Enable Hotspot to Track Native Memory Usage for Direct Byte Buffers

[Adam Farley8]

Hi Paul,

> Hi Adam,
>
> From reading the thread i cannot tell if this is part of a wider 
solution including some yet to be proposed HotSpot changes.

The wider solution would need to include some Hotspot changes, yes.
I'm proposing raising a bug, committing the code we have here to
"set the stage", and then we can invest more time later
if the concept goes down well and the community agrees to pursue
the full solution.

As an aside, I tried submitting a big code set (including hotspot
changes) months ago, and I'm *still* struggling to find someone to
commit the thing, so I figured I'd try a more gradual, staged approach
this time.

>
> As is i would be resistant to adding such standalone internal wrapper 
methods to Unsafe that have no apparent benefit within the OpenJDK itself 
since it's a maintenance burden.

I'm hoping the fact that the methods are a single line (sans 
comments, descriptors and curly braces) will minimise this burden.

>
> Can you determine if the calls to UNSAFE.freeMemory/allocateMemory come 
from a DBB by looking at the call stack frame above the unsafe call?
>
> Thanks,
> Paul.

Yes that is possible, though I would advise against this because:

 A) Checking the call stack is expensive, and doing this every time we 
allocate native memory is an easy way to slow down a program,
or rack up mips.
and 
 B) deciding which code path we're using based on the stack 
means the DBB class+method (and anything the parsing code 
mistakes for that class+method) can only ever allocate native 
memory for DBBs.

What do you think?

Best Regards

Adam Farley

>
>> On Feb 14, 2018, at 3:32 AM, Adam Farley8  
wrote:
>> 
>> Hi All,
>> 
>> Currently, diagnostic core files generated from OpenJDK seem to lump 
all 
>> of the 
>> native memory usages together, making it near-impossible for someone to 

>> figure 
>> out *what* is using all that memory in the event of a memory leak.
>> 
>> The OpenJ9 VM has a feature which allows it to track the allocation of 
>> native
>> memory for Direct Byte Buffers (DBBs), and to supply that information 
into 
>> the
>> cores when they are generated. This makes it a *lot* easier to find out 

>> what is using
>> all that native memory, making memory leak resolution less like some 
dark 
>> art, and
>> more like logical debugging.
>> 
>> To use this feature, there is a native method referenced in 
Unsafe.java. 
>> To open
>> up this feature so that any VM can make use of it, the java code below 
>> sets the 
>> stage for it. This change starts letting people call DBB-specific 
methods 
>> when 
>> allocating native memory, and getting into the habit of using it.
>> 
>> Thoughts?
>> 
>> Best Regards
>> 
>> Adam Farley
>> 
>> P.S. Code:
>> 
>> diff --git 
>> a/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template 
>> b/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
>> --- 
a/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
>> +++ 
b/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
>> @@ -85,7 +85,7 @@
>> // Paranoia
>> return;
>> }
>> -UNSAFE.freeMemory(address);
>> +UNSAFE.freeDBBMemory(address);
>> address = 0;
>> Bits.unreserveMemory(size, capacity);
>> }
>> @@ -118,7 +118,7 @@
>> 
>> long base = 0;
>> try {
>> -base = UNSAFE.allocateMemory(size);
>> +base = UNSAFE.allocateDBBMemory(size);
>> } catch (OutOfMemoryError x) {
>> Bits.unreserveMemory(size, cap);
>> throw x;
>> diff --git a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java 
>> b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
>> --- a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
>> +++ b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
>> @@ -632,6 +632,26 @@
>> }
>> 
>> /**
>> + * Allocates a new block of native memory for DirectByteBuffers, 
of 
>> the
>> + * given size in bytes.  The contents of the memory are 
>> uninitialized;
>> + * they will generally be garbage.  The resulting native pointer 
will
>> + * never be zero, and will be aligned for all value types. Dispose 

>> of
>> + * this memory by calling {@link #freeDBBMemory} or resize it with
>> + * {@link #reallocateDBBMemory}.
>> + *
>> + * @throws RuntimeException if the size is negative or too large 
>> + *  for the native size_t type
>> + *
>> + * @throws OutOfMemoryError if the allocation is refused by the 
>> system
>> + *
>> + * @see #getByte(long)
>> + * @see #putByte(long, byte)
>> + */
>> +public long allocateDBBMemory(long bytes) {
>> +return allocateMemory(bytes);
>> +}
>> + 
>> +/**
>>  * Resizes a new block of native memory, to the given size in 
bytes. 
>> The
>>  * contents of the new block 

Re: RFR: 8194154: JDK crashes parsing path string contains '//' on linux

[Alan Bateman]

On 19/02/2018 05:09, yumin qi wrote:

Thanks!

I need a sponsor for pushing it to jdk. Can you or someone else help 
to push it?
minqi is a jdk committer. If this is you then you should be able to push 
it yourself.


-Alan.

Re: [JDK-6341887] Patch: java.util.zip: Add ByteBuffer methods to Inflater/Deflater

[David Lloyd]

On Sat, Feb 17, 2018 at 3:18 PM, Xueming Shen  wrote:
> Hi David,
>
> Thanks for taking this one :-) some comments here.

Thanks for the review!

> (1) I would assume you might have to do more for ByteBuffer, something like
> [...]
> btw, any reason go unsafe to get the byte[]? instead of
> ByteBuffer.getArray()?

Yes: input should always be settable from a read-only heap buffer
without a performance penalty (i.e. copying the contents).  This btw
is why there is also an explicit check for read-only later on.

> (2) It might be a concerned that you have to warp the input byte[] every time
>  the inflate/deflate(byte[]...) is called. (Yes, I'm constantly hearing 
> people
>  complain that you have to wrap the byte[] to ByteBuffer to use CharsetEn/
>  Decoder, or even the implementation detail inside StringCoder), and it 
> might
>  be taken as a "regression". So it might be desired to wire the 
> "bf.hasArray())
>  path inside de/encode(ByteArray) back to de/encode(byte[], int, int), to 
> avoid
>  the "unnecessary" ByteBuffer wrap.

I can do some testing to see if there is an impact.  I am working on
the basis that the wrap may be optimized away (as it is in certain
similar cases), but Inflater/Deflater is not final (nor are the
inflate/deflate methods) that might not be true in practice.

> (3) Same "wrap concern" argument might go to the setInput(bye[] ...) as well.
>  I'm not sure if it is worth keeping both byte[]/int/int and ByteBuffer 
> as the
>  "input" field of In/Deflater.

Since input is stored on the object, the above theoretical
optimization is much less likely.  Again I can do some testing; it
might be a good idea to have separate byte[]/int/int and ByteBuffer in
the end, though the added expense of checking for and updating the
ByteBuffer position after each call in addition to the byte[]/int/int
might nullify the benefit.  Testing is required...

> (4) assume we keep the "wrap" approach. It appears ByteBuffer.wrap() does
> check the byte[]/off/len and throw an IndexOutOfBoundsException. So it might
> be better to take advantage of that check.

I wanted to however it throws the wrong exception type; I could catch
and rethrow I guess though if you think that is better (assuming we
keep the "wrap" approach as you say).

> (5) Deflater.input need to be initialized to a non-null value.
>  Btw ZipUtil.defaultBuf needs to be "direct"?

It doesn't need to be, but the direct buffer code path is possibly
somewhat "friendlier" to GC since there's no GetPrimitiveArrayCritical
call.

> (6) It might be desired to have some jmh measure to make sure byte[] case
> does not have regression.

I'll work on this when I get a chance (maybe not until Friday though).

One other thought I had this weekend was that I could possibly improve
things by getting the direct buffer address on the Java side, and pass
it in to JNI to avoid the calls to GetDirectBufferAddress.  Another
thought was to eliminate the remaining SetBooleanField calls by using
the remaining two bits in the doInflate/doDeflate methods' return
values.  I'm not sure how expensive these calls are though.  I could
also replace the JNI field reads with more method parameters if this
is a valuable thing to do.

-- 
- DML

Re: RFR: 8197594 - String and character repeat

[Martin Buchholz]

.



On Sun, Feb 18, 2018 at 11:19 AM, Martin Buchholz 
wrote:

>
>  - how many digits to consume after the escape?  How much do we trust
> Unicode to never ever grow beyond 5 hex digits?
>

Oops, I already got it wrong - it's already at 6 hex digits because there
are 17 planes, not 16.  MAX_CODE_POINT is U+10.
Yes, we need a variable width syntax like regex \x{h...h}

And java regex also supports
  \N{name} The character with Unicode character name 'name'
so we could do the same for the java language.
Although it would be a little weird to have every Unicode update make some
previously invalid source files valid.

We could also say "It's 2018 and UTF-8 has won" and simply use UTF-8 in
source files directly.   No Unicode escapes needed.

Re: [PATCH] Reduce Chance Of Mistakenly Early Backing Memory Cleanup

[Ben Walsh]

As requested, here are the results with modifications to the annotations 
on Reference.reachabilityFence. Much more promising ...


* Benchmark 1 *

Test Code :

package org.sample;

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Level;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;

import java.nio.ByteBuffer;

public class ByteBufferBenchmark {

@State(Scope.Benchmark)
public static class ByteBufferContainer {

ByteBuffer bb;

@Setup(Level.Invocation)
public void initByteBuffer() {
bb = ByteBuffer.allocateDirect(1);
}

ByteBuffer getByteBuffer() {
return bb;
}
}

@Benchmark
public void benchmark_byte_buffer_put(ByteBufferContainer bbC) {
 
bbC.getByteBuffer().put((byte)42);
}

}

Results :

- Unmodified Build -

Benchmark   Mode  Cnt Score   
Error  Units
ByteBufferBenchmark.benchmark_byte_buffer_put  thrpt  200  35604933.518 ± 
654975.515  ops/s

- Build With Reference.reachabilityFences Added -

Benchmark   Mode  Cnt Score   
Error  Units  Impact
ByteBufferBenchmark.benchmark_byte_buffer_put  thrpt  200  33100911.857 ± 
747461.951  ops/s  -7.033%

- Build With Reference.reachabilityFences Added And DontInline Replaced 
With ForceInline -

Benchmark   Mode  Cnt Score   
Error  Units  Impact
ByteBufferBenchmark.benchmark_byte_buffer_put  thrpt  200  34836320.294 ± 
640188.408  ops/s  -2.159%

- Build With Reference.reachabilityFences Added And DontInline Removed -

Benchmark   Mode  Cnt Score   
Error  Units  Impact
ByteBufferBenchmark.benchmark_byte_buffer_put  thrpt  200  34740015.332 ± 
556578.542  ops/s  -2.429%


* Benchmark 2 *

Test Code :

package org.sample;

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Level;
import org.openjdk.jmh.annotations.Param;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;

import java.nio.ByteBuffer;

@State(Scope.Benchmark)
public class ByteBufferBenchmark {

@Param({"1", "10", "100", "1000", "1"})
public int L;

@State(Scope.Benchmark)
public static class ByteBufferContainer {

ByteBuffer bb;

@Setup(Level.Invocation)
public void initByteBuffer() {
bb = ByteBuffer.allocateDirect(1);
}

ByteBuffer getByteBuffer() {
return bb;
}
}

@Benchmark
public ByteBuffer benchmark_byte_buffer_put(ByteBufferContainer bbC) {
 
ByteBuffer bb = bbC.getByteBuffer();

for (int i = 0; i < L; i++) {
bb.put((byte)i);
}

return bb;
}

}

Results :

- Unmodified Build -

Benchmark(L)   Mode  Cnt Score  
Error  Units
ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
29303145.752 ± 635979.750  ops/s
ByteBufferBenchmark.benchmark_byte_buffer_put 10  thrpt  200 
24260859.017 ± 528891.303  ops/s
ByteBufferBenchmark.benchmark_byte_buffer_put100  thrpt  200 
8512366.637 ± 136615.070  ops/s
ByteBufferBenchmark.benchmark_byte_buffer_put   1000  thrpt  200 
1323756.037 ±  21485.369  ops/s
ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
145965.305 ±   1301.469  ops/s

- Build With Reference.reachabilityFences Added -

Benchmark(L)   Mode  Cnt Score  
Error  Units  Impact
ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
28893540.122 ± 754554.747  ops/s  -1.398%
ByteBufferBenchmark.benchmark_byte_buffer_put 10  thrpt  200 
15317696.355 ± 231621.608  ops/s  -36.863%
ByteBufferBenchmark.benchmark_byte_buffer_put100  thrpt  200 
2546599.578 ±  32136.873  ops/s  -70.084%
ByteBufferBenchmark.benchmark_byte_buffer_put   1000  thrpt  200 
288832.514 ±   3854.522  ops/s  -78.181%
ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 29747.386 
±214.831  ops/s  -79.620%

- Build With Reference.reachabilityFences Added And DontInline Replaced 
With ForceInline -

Benchmark(L)   Mode  Cnt Score  
Error  Units  Impact
ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
29372326.859 ± 525988.179  ops/s  +0.236%
ByteBufferBenchmark.benchmark_byte_buffer_put 10  thrpt  200 
24326735.480 ± 484358.862  ops/s  +0.272%
ByteBufferBenchmark.benchmark_byte_buffer_put100  thrpt  200 
8492692.912 ± 120924.878  ops/s  -0.231%
ByteBufferBenchmark.benchmark_byte_buffer_put   1000  thrpt  200 
1332131.417 ±  14981.587  ops/s  +0.633%
ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
144990.569 ±   1518.877  ops/s  -0.668%

- Build With 

8196830: publicLookup().findVirtual should not return method handle to AccessibleObject.setAccessible

[Alan Bateman]

AccessibleObject's setAccessible(boolean) is currently not caller 
sensitive but the overrides in Method/Field/Constructor are. This 
awkwardness stems from its constructor being protected and the method 
not being final. It is thus possible to extend the class outside of the 
java.lang.reflect package and override this method (at least one popular 
library does this). Ideally the constructor should have been package 
private and/or the method be final but it's not possible to change this 
after 20 years.


The consequence of the method in the base class not being caller 
sensitive is that it's possible to use a minimally trusted lookup to get 
a method handle to the method. Paul, Mandy and I chatted about this one 
recently. We prototyped changes to the MH implementation to special case 
this method and treat it "as if" it is caller sensitive. This maximizes 
compatibility but has the downside that it makes it harder to audit and 
somewhat fragile. In the end, we concluded it would be simpler to add 
the @CS annotation to this method so that it is treated consistently. 
The downside of this is that custom AccessibleObject implementations 
need to override setAccessible if they want to be invoked using method 
handles obtained from a minimally trusted lookup.


The proposed changes are simple. The removal of "checkMemberAccess" from 
canBeCalledVirtual is just a clean-up because this method is no longer 
needs special casing (it was degraded for Java SE 10 as envisaged in JEP 
176). It's not the goal here to improve the performance of 
canBeCalledVirtual but there may be opportunities to look at that with a 
separate issue:

  http://cr.openjdk.java.net/~alanb/8196830/webrev/

-Alan

Re: RFR: 8194154: JDK crashes parsing path string contains '//' on linux

[yumin qi]

Yes, I am committer.

Brian, do you okay with the version? If no objection, I will push it into
jdk.

Thanks
Yumin

On Mon, Feb 19, 2018 at 2:28 AM, Alan Bateman 
wrote:

> On 19/02/2018 05:09, yumin qi wrote:
>
>> Thanks!
>>
>> I need a sponsor for pushing it to jdk. Can you or someone else help to
>> push it?
>>
> minqi is a jdk committer. If this is you then you should be able to push
> it yourself.
>
> -Alan.
>

Re: [JDK-6341887] Patch: java.util.zip: Add ByteBuffer methods to Inflater/Deflater

[David Lloyd]

On Sun, Feb 18, 2018 at 3:33 AM, Alan Bateman  wrote:
> Thanks for bringing this one up again

Thanks for taking the time to review.

> I see Sherman is looking at implementation so I'll stick with the javadoc
> for now. At some point it will need a security review to ensure that there
> no possibility of tricking the implementation to access memory outside of
> the input/output. That is, we have to assume the user of
> setInput(ByteBuffer) is evil and will change the position/limit during
> deflate operations. I see the patch computes clamps the remainder before
> accessing memory, it will just need a closer look to make sure there are no
> issues. The patch will also need adjustments to make it consistent with the
> existing code but that can come later.

I did write the code with this in mind: that the address should always
be within the bounds of the buffer (be it heap or direct) at the time
of the call, and that the offset into the buffer should not be beyond
its end (or beginning).  So the effect could be a thrown exception but
escaping the buffer _should_ be impossible (by intent anyway; more
eyes are always better for noticing mistakes of course).

> On the APIs then the inflate and deflate methods look okay, I'll less sure
> that  setDcitionary(ByteBuffer) is really needed. Are you adding for
> setDcitionary(ByteBuffer) for consistency?

Yes; it was easy enough to add it so I did.

> The javadoc doesn't currently specify how it works with the buffer, e.g.
> inflate(ByteBuffer) doesn't specify that adds it bytes to the buffer
> starting at its position, it doesn't say if the position is adjusted. The
> javadoc will also need to set expectations on behavior when
> DataFormatException is thrown, is the position guaranteed to be unchanged or
> is it unspecified?

I intended for it to work similarly to the old code.  But I'll go
through the zlib docs and just make sure all the assumptions are still
correct, and the next version will have more concise docs in this
regard and also with regards to the disposition of the buffer in each
case.

-- 
- DML