Re: System.exit deadlock if called within a hook

[David Lloyd]

On Mon, Apr 25, 2022 at 9:25 AM  wrote:
>
> Ok, you're right that it can raise an exception when the calling thread does 
> not have the security privileges needed. But what if it had them ? Right now 
> the deadlock comes from the fact is doesn't got back to the caller with an 
> exception and doesn't end the current thread. My 1st proposal was to have a 
> way to end the current thread. If you think that is not acceptable and prefer 
> the exception solution, then something like throw new 
> IllegalStateException("Shutdown in progress") which is already used when 
> adding new hooks while shutting down and adjust the System.exit method 
> documentation accordingly (might be dangerous since it "breaks" actual 
> documented and implemented contract, might be too heavy for a change).
>
> First things first : do you think this is an issue the JVM has to solve 
> (either using a destroy-current-thread solution, or a 
> throw-IllegalStateException one), or do you think this was a hook misuse and 
> maybe would only need maybe a clearer warning in the hooks's documentation, 
> or do you think we keep everything as it is because I should have guessed 
> better reading the actual documentation 😊 ?

If you're asking me personally, I think it would make a lot of sense
to throw IllegalStateException or a new subclass thereof (e.g.
`ExitInProgressException` or something). However, my opinion doesn't
count. I suspect that those whose opinions *do* count will probably
say that this would be too much of a breaking change.

-- 
- DML • he/him

Re: System.exit deadlock if called within a hook

[David Lloyd]

On Mon, Apr 25, 2022 at 8:09 AM  wrote:
> because it would still run finally blocks, which cannot happen for no return 
> methods like System.exit and Runtime.halt

FWIW I don't think this is correct. `exit` and `halt` etc. cannot
*return* but they're definitely allowed to throw an exception
(`SecurityException` is presently specified) and thus are allowed to
unwind the stack, even through `finally` blocks.

-- 
- DML • he/him

Re: RFR: 8269383: New java.nio.ByteOrder.isBigEndian and isLittleEndian methods

[David Lloyd]

Is this better than the current solution of `nativeOrder() ==
BIG_ENDIAN` other than reducing a few keystrokes?

On Fri, Jun 25, 2021 at 8:45 AM Yi Yang  wrote:
>
> Hi, can I have a review of this change that adds two new utility methods for 
> java.nio.ByteOrder? Looking through the whole JDK codebase, most calls of 
> ByteOrder.nativeOrder() is to check if the underlying platform is 
> little-endian/big-endian. There is no reason to only provide 
> ByteOrder.nativeOrder while leaving big-endian/little-endian checking methods 
> blank.
>
> Thanks!
>
> -
>
> Commit messages:
>  - add new methods
>
> Changes: https://git.openjdk.java.net/jdk/pull/4595/files
>  Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4595&range=00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8269383
>   Stats: 20 lines in 1 file changed: 20 ins; 0 del; 0 mod
>   Patch: https://git.openjdk.java.net/jdk/pull/4595.diff
>   Fetch: git fetch https://git.openjdk.java.net/jdk pull/4595/head:pull/4595
>
> PR: https://git.openjdk.java.net/jdk/pull/4595
>


-- 
- DML • he/him

Re: JEP 411: Deprecation with removal would break most existing Java libraries

[David Lloyd]

On Mon, Jun 14, 2021 at 6:47 PM Peter Firmstone
 wrote:
> SecurityManager depends on Permission, currently there are Permission
> checks throughout the JVM, however Permission implementation classes
> will be removed, although the Permission class itself won't be.

This is incorrect AFAICT.  The relevant JEP text is:

> Permission and subclasses — Other significant classes, such as 
> ProtectionDomain, depend on Permission. Many of the subclasses of Permission, 
> however, are specific to use cases which will likely no longer be relevant 
> after the Security Manager is removed. The maintainers of these subclasses 
> can deprecate and remove them separately, after evaluating the compatibility 
> risk.

> Permission references can be replaced with Guard references (which
> Permissions are instances of).

I guess you've got something fairly complex in mind, could you give
some practical examples of how this would work?

> The Permission implementations of Guard::check call SecurityManager, so
> checks will continue working as expected, but it allows us to intercept
> them and do something different.

What do you envision these checks looking like?  Where would the JDK
find these Guard instances?

> By replacing Permission references with Guard, it allows us to implement
> our own checks in these locations, and OpenJDK doesn't need to maintain
> Permission instances, and or, we don't need to make use of unmaintained
> Permission implementations.

As I said I don't think there is an intention to remove the permission
classes just yet, and I don't think that it is a fair statement to say
that the permission implementations would be unmaintained.  Most of
those classes have not needed to be touched in many years; there's
just not a lot of complexity there for the vast majority of them.

> There are already issues with Permission implementations, take for
> example SocketPermission, it consults DNS and it isn't possible to enter
> a range of IP addresses (such as the local subnet, and a list of public
> IP addresses), for now, every single IP address must be entered and this
> isn't practical.   The proposed API would allow us to re-implement
> SocketPermission functionality, as well as other Permission implementations.

Sure, this would be nice to clean up.  I just don't understand the
proposed mechanism.

> > On Mon, Jun 14, 2021 at 12:57 AM Alan Bateman  
> > wrote:
> >> AccessController::doPriv just runs the action.
> > TBH this should have always been the case.  Implementation-wise, if
> > one were constructing an access control context based on stack
> > walking, one would stop at points where `AccessController` is on the
> > stack (which is easily determinable) to do special work on assembling
> > the access control context based on the method called at that frame.
>
> Yes, one can do that, but these classes will also eventually be removed.

Sure. This was mainly a practical observation about the current
implementation.  And any replacement third-party stack-based
authorization system could (and should) use a similar mechanism
regardless of whether these exact methods stay in the JDK for 5 or 50
more years.

-- 
- DML • he/him

Re: JEP 411: Deprecation with removal would break most existing Java libraries

[David Lloyd]

On Mon, Jun 14, 2021 at 2:38 AM Peter Firmstone
 wrote:
>  1. Develop authorization layer security provider services in OpenJDK,
> back port it to Java 8 and Java 11 (these provide most of the
> utilised functionality of SecurityManager, allowing developers to
> only implement those which they need, without enabling
> SecurityManager and editing policy files).
>  2. Remove SecurityManager, Policy and Policy provider in OpenJDK 19.

The SecurityManager class itself already is *exactly* an authorization
provider.  I don't think it makes sense to consider removing the
security manager class to replace it with something that has basically
exactly the same API (specifically, a single method for each general
authorization check that can be called without constructing any new
objects, if and only if the authorization provider is installed).  See
my other proposal where, post-"removal", SecurityManager (the class)
is retained but made abstract (and sans a few methods).  All of the
existing code which performs authorization checks would be retained
and the problem solved in essentially the way you're describing, just
using existing APIs.

The security manager implementation itself can implement any kind of
authorization behavior whatsoever, based mainly on the Permission
types (which work just fine for this purpose, and anyway are already
retained by the current JEP).  Policy and its supporting classes are
completely unnecessary for implementing a security policy.  In fact,
this is the case today already.

On Mon, Jun 14, 2021 at 12:57 AM Alan Bateman  wrote:
> AccessController::doPriv just runs the action.

TBH this should have always been the case.  Implementation-wise, if
one were constructing an access control context based on stack
walking, one would stop at points where `AccessController` is on the
stack (which is easily determinable) to do special work on assembling
the access control context based on the method called at that frame.

-- 
- DML • he/him

Re: JEP draft: Scope Locals

[David Lloyd]

On Wed, May 12, 2021 at 9:43 AM Andrew Haley  wrote:

> There's been considerable discussion about scope locals on the loom-dev
> list,
> and it's now time to open this to a wider audience. This subject is
> important
> because. although scope locals were motivated by the the needs of Loom,
> they
> have many potential applications outside that project.
>

I didn't get a chance to mention earlier, but I think scope locals are
shaping up really great so far: simple yet powerful.  I know this will be a
really useful addition to the JDK.  There are several projects both within
and without Red Hat that I have authored or have contributed to which use a
very compatible pattern (that is, using a lexically constrained binding
strategy for context association) and are perfect candidates to switch over
to use this feature.

That said, I have one minor comment about the API. :-)

I think it would be really nice if the snapshot class could hold its
run/call method rather than making it a static method of `ScopeLocal`. This
reads more naturally to me (and is nicer to write):

var snap = ScopeLocal.snapshot();
return executor.submit(() -> snap.call(aCallable));

Than this:

var snap = ScopeLocal.snapshot();
return executor.submit(() -> ScopeLocal.callWithSnapshot(aCallable, snap
));

This kind of lexically bound contextual pattern can be found in several of
our existing projects (including those listed below [1] [2]).  In writing
these projects, we took it a step further than just Runnable and Callable
and added variants which accept `Consumer` plus a `T` to pass in, a
`Function` plus a `T` to pass in, and `BiConsumer` and `BiFunction`
variants as well which accept two arguments to pass in.  The practical
reason here is that we could then pass in a method handle with explicit
arguments rather than relying on lambda capture, which can be expensive in
certain circumstances.  The overall benefit vs cost of doing this is
probably debatable, but I thought the idea might be interesting at any rate.

Anyway that's it.  I love this feature and am excited to see it get into
the JDK!

[1] WildFly Elytron "Scoped" API, used by client and server authentication
contexts (WildFly's version of JAAS Subject):
https://github.com/wildfly-security/wildfly-elytron/blob/1.x/auth/server/base/src/main/java/org/wildfly/security/auth/server/Scoped.java
[2] WildFly Common "Contextual" API used for transaction, configuration,
and other context propagation:
https://github.com/wildfly/wildfly-common/blob/master/src/main/java/org/wildfly/common/context/Contextual.java

-- 
- DML • he/him

Re: JEP draft: Scope Locals

[David Lloyd]

On Wed, May 19, 2021 at 4:01 AM Andrew Haley  wrote:

> On 5/15/21 6:50 PM, Peter Levart wrote:
> > What if I wanted to create and start a thread that would be "pristine" -
> > not have any ScopeLocal value bound? Is this intentionally not allowed
> > in order to make sure that inheritable ScopeLocal(s) can't be cleared by
> > code that has no access to ScopeLocal instance(s)?
>
> That one is about to be changed by a revision to the JEP. There clearly
> is a need to control whether a newly-created thread inherits scope locals
> or not. For instance, an Executor might lazily create worker threads, and
> we don't want them to inherit scope locals from the thread that was
> running.
>

Turning this around, I would argue that there are few (or perhaps *no*)
cases where it would ever be desirable to inherit scope locals across
thread creation; in cases where this is explicitly desired, one can always
resume the snapshot from within the thread's Runnable. Was there a
particular use case this was meant to address?

-- 
- DML • he/him

Possible subtle memory model error in ClassValue

[David Lloyd]

I'm helping a colleague debug a weird problem that occurs in
ClassValue on jdk11u (and presumably on upstream as well, though it's
presently impossible to verify).  As a disclaimer, the problem
manifests itself when building native images via GraalVM so it's
possible that something is simply broken there, but it seems at least
feasible that it could be a plain old Java bug so I thought I'd send
up a flare here to see if this makes sense to anyone else.

The bug itself manifests (on jdk11u) as an NPE with the following
exception trace:

java.lang.NullPointerException
at 
java.base/java.lang.ClassValue$ClassValueMap.loadFromCache(ClassValue.java:535)
at 
java.base/java.lang.ClassValue$ClassValueMap.probeHomeLocation(ClassValue.java:541)
at java.base/java.lang.ClassValue.get(ClassValue.java:101)
...

Some basic analysis shows that this should be impossible under
normal/naive circumstances: the initializer of
java.lang.ClassValue.ClassValueMap sets the corresponding field to
non-null during construction.

However, I'm wondering if this isn't a side effect of what appears to
be an incorrect double-checked lock at lines 374-378 of
ClassValue.java [1].  In order for the write to the non-volatile
`cache` field of ClassValueMap, it is my understanding that there must
be some acquire/release edge from where the variable is published to
guarantee that all of the writes are visible to the read site, and I'm
not really sure that the exit-the-constructor edge is going to match
up with with the synchronized block which protects a different
non-volatile field.  And even if my feeling that this is dodgy is
valid, I'm not sure whether this NPE is a possible outcome of that
problem!

Thoughts?

[1] 
https://github.com/openjdk/jdk11u-dev/blob/3789983e89c9de252ef546a1b98a732a7d066650/src/java.base/share/classes/java/lang/ClassValue.java#L374-L378
-- 
- DML • he/him

Math.mutiplyHigh() is signed

[David Lloyd]

In retrospect I don't know why I expected Math.multiplyHigh() to
return the high word of the unsigned product of two 64-bit numbers,
given that long is signed; in my defence however, the docs don't
actually seem to specify.

WDYT about a patch like this to clarify?

diff --git a/src/java.base/share/classes/java/lang/Math.java
b/src/java.base/share/classes/java/lang/Math.java
index 8147b7109e2..552501ca6d7 100644
--- a/src/java.base/share/classes/java/lang/Math.java
+++ b/src/java.base/share/classes/java/lang/Math.java
@@ -1095,8 +1095,8 @@ public final class Math {
 }

 /**
- * Returns as a {@code long} the most significant 64 bits of the 128-bit
- * product of two 64-bit factors.
+ * Returns as a {@code long} the most significant 64 bits of the
signed 128-bit
+ * product of two 64-bit signed factors.
  *
  * @param x the first value
  * @param y the second value


-- 
- DML

Re: New candidate JEP: 371: Hidden Classes

[David Lloyd]

On Mon, Jan 27, 2020 at 12:30 PM Mandy Chung  wrote:
>
>
>
> On 1/27/20 6:31 AM, Remi Forax wrote:
>
> I'm a bit curious about this:
>
> To invoke private nestmate instance methods from code in the hidden class, use
> invokevirtual or invokeinterface instead of invokespecial. Generated bytecode
> that uses invokespecial to invoke a private nestmate instance method will fail
> verification. invokespecial should only be used to invoke private nestmate
> constructors.
>
> This seems pretty unusual.  Don't we normally use invokespecial for
> private method invocations?  What is the reasoning for this?
>
> It's the new usual since 11 and the introduction of nestmates,
> before javac was generating an accessor method (the access$000 methods).
>
> :
>
>
>
> As Remi said, this feature has been there since 11.  You can reference JEP 
> 181 for more details.
>
> Mandy
> [1] https://openjdk.java.net/jeps/181

This is very helpful, thanks!
-- 
- DML

Re: New candidate JEP: 371: Hidden Classes

[David Lloyd]

On Fri, Jan 24, 2020 at 4:56 PM David Holmes  wrote:
>
> FYI.

I'm a bit curious about this:

> To invoke private nestmate instance methods from code in the hidden class, 
> use invokevirtual or invokeinterface instead of invokespecial. Generated 
> bytecode that uses invokespecial to invoke a private nestmate instance method 
> will fail verification. invokespecial should only be used to invoke private 
> nestmate constructors.

This seems pretty unusual.  Don't we normally use invokespecial for
private method invocations?  What is the reasoning for this?

-- 
- DML

Re: New candidate JEP: 370: Foreign-Memory Access API

[David Lloyd]

On Fri, Nov 29, 2019 at 5:34 PM John Rose  wrote:
>
> On Nov 29, 2019, at 4:24 AM, David Lloyd  wrote:
>
>
> Makes sense, it's still early.  But now the seed is planted. :)
>
>
> (It’s better documented.  Most of the points made here were not new.)

Okay, anyway I'm glad that I'm not the first person to have thought of
these use cases.  The API looks good!
-- 
- DML

Re: New candidate JEP: 370: Foreign-Memory Access API

[David Lloyd]

On Thu, Nov 28, 2019 at 11:24 AM Vladimir Ivanov
 wrote:
>
>
> >> Second, what about an API to allocate memory from the stack?  This is
> >> really useful in GraalVM (granted they have a sort-of-Panama-ish way
> >> of calling C functions already, and they're really quite "loose" when
> >> it comes to safety in some regards) and could simplify some things
> >> already, particularly if the pointer could then be passed to a JNI
> >> method.  Given the sensitive nature of stack allocation you'd probably
> >> have to add some extra checks (like checking that the thread matches
> >> on each access and that it is in scope, and you'd probably have to do
> >> the latter via callback like StackWalker does) but it would be pretty
> >> useful nonetheless.
> >
> > I guess it depends on what the goal is here - if the goal is to 'speed
> > up allocation', there are other ways to skin that cat (and we're
> > actually working on what seems like a very promising direction which we
> > should be able to communicate more publicly soon). But, another related
> > goal would be to be able to express stack allocation in Java directly,
> > so that some of the work for e.g. writing bridge code which goes from
> > Java to native can be expressed in Java directly. While something like
> > this would probably be useful, as an implementation tool (e.g. to
> > implement our own programmable invoker [2]), I don't see that being a
> > priority from a public API perspective (assuming we can make allocation
> > fast enough - that is!).
>
> Yes, stack allocation looks attractive, but in practice its usability is
> quite limited. (IMO thread-local arena-style memory allocator is more
> practical.) And it makes some assumptions about how threads and stacks
> work which are aren't specified by JVMS, but are implementation details.
> For example, how will it interact with Project Loom?
>
> Leaving high-level considerations aside, there's already a way to build
> stack allocator without any JVM assistance on top of JNI (though
> definitely not the most efficient and performant one):
>
>- do a native call;
>- allocate memory on stack inside native frame;
>- do an upcall to user-provided callback and pass a pointer to
> allocated memory;
>
> On Java level it can be exposed as:
>
>  native R allocateOnStack(long size, LongFunction task);
>
> What Memory Access API can add to it is safety: instead of passing raw
> pointer to stack memory with all safety concerns associated with it, it
> can be wrapped into a MemorySegment before returning it to the user and
> on a way back closed, thus effectively forbidding any illegal accesses
> (both temporal and spatial) and in completely transparent way:
>
>  R withSegmentOnStack(long size, Function task);
>
> As a performance optimization, JIT can then intrinsify the native method
> and directly allocate memory on stack without the need for auxiliary
> native frame. It'll provide alloca-like behavior while considerably
> simplifying JVM support.

That's exactly what I'm looking for: safety plus speed, for the
primary purpose of native invocation.  As a bonus, having the memory
access API provide this capability could mean that it could, on the
back end, do something other than strictly stack-based allocation
(e.g. allocating from a TLAB for larger objects).

> It definitely looks very interesting in the context of low-level ABI and
> native invokers, but IMO stack allocation is not a prerequisite for
> Memory Access API and the API shouldn't wait for it to be released.
>
> After there's more data available from ABI experiments, we'll be in a
> better position to decide how to proceed (either add it to Memory Access
> API, build a separate API on top, or even drop it).

Makes sense, it's still early.  But now the seed is planted. :)
-- 
- DML

Re: New candidate JEP: 370: Foreign-Memory Access API

[David Lloyd]

On Mon, Nov 25, 2019 at 11:28 AM  wrote:
>
> https://openjdk.java.net/jeps/370

One more question!  Well, two I guess.

First, in the interim before Panama or something like it comes to the
main project, will it be possible to pass MemoryAddress kinds of
things to JNI methods (to replace the current "pattern" of sticking
pointer values into `long` fields)?  Could the JNI API be extended for
this purpose?

Second, what about an API to allocate memory from the stack?  This is
really useful in GraalVM (granted they have a sort-of-Panama-ish way
of calling C functions already, and they're really quite "loose" when
it comes to safety in some regards) and could simplify some things
already, particularly if the pointer could then be passed to a JNI
method.  Given the sensitive nature of stack allocation you'd probably
have to add some extra checks (like checking that the thread matches
on each access and that it is in scope, and you'd probably have to do
the latter via callback like StackWalker does) but it would be pretty
useful nonetheless.
-- 
- DML

Re: New candidate JEP: 370: Foreign-Memory Access API

[David Lloyd]

On Mon, Nov 25, 2019 at 11:28 AM  wrote:
>
> https://openjdk.java.net/jeps/370

Looks pretty interesting!  Why make the API user deal with VarHandles
though, as opposed to (say) putting accessor methods directly onto
MemoryAddress?
-- 
- DML

Re: Class-Path (in jar file) semantics different between Java 11 and 13 (on Windows)?

[David Lloyd]

On Wed, Nov 20, 2019 at 8:59 AM Alan Bateman  wrote:
>
> On 20/11/2019 13:50, David Lloyd wrote:
> > :
> > OK, but this decision violates both the old and updated spec (and
> > makes it difficult to write code that works in both cases: in
> > situations that reject absolute URLs (javac) and in situations that
> > reject drive letters (this code)), so I would request that this be
> > revisited.
> >
> This cannot be rushed as it require detailed security analysis.

Sure, whatever process is necessary should be undertaken.  Hopefully
there is not a lot to do in terms of security analysis for this
particular change though; I imagine it's more of just running through
the process.  But when the just-rewritten code doesn't meet the
just-rewritten spec, I think it's warranted.

> Is there
> any reason why you need to encode absolute file paths as relative URLs?
> I assume the use-case discussed here will work if File::toURI or toURL
> is used to create the file URL as it will have the "file:" scheme.

I'll see where the usages are.  I believe at least one usage is out of
our control though, and I'm pretty sure that Maven uses absolute paths
for surefire and failsafe (test) launching.

-- 
- DML

Re: Class-Path (in jar file) semantics different between Java 11 and 13 (on Windows)?

[David Lloyd]

On Wed, Nov 20, 2019 at 9:26 AM Scott Palmer  wrote:
>
> /C:/blah... is “root relative”. The (old) spec says the URL must be "relative 
> to the code base".  What does "code base" mean when not referring to Applets 
> or RMI?

"code base" is generally the URL of the class path entry itself.  It's
mainly defined in the specification for java.lang.ClassLoader.

> This seems like a bad idea (security hole) that worked by accident.

The specification is pretty clear, and doesn't seem unintentional.
Relative URLs have a very specific normative definition.  It's very
unambiguous; the code just doesn't work the way the specification says
it should.

-- 
- DML

Re: Class-Path (in jar file) semantics different between Java 11 and 13 (on Windows)?

[David Lloyd]

On Wed, Nov 20, 2019 at 2:25 AM Alan Bateman  wrote:
>
> On 19/11/2019 23:25, David Lloyd wrote:
> > :
> > OK, having read the updated specification (thanks Alan!) I'm now quite
> > curious why `/C:/helloworld.jar` is considered invalid.  It is in fact
> > a valid relative URL (colons are allowed in path segments, and the
> > leading `/` unambiguously delineates the URL path), and thus it seems
> > that it should be considered valid.
> This is a awkward area as the parsing here is very security sensitive.
> The current implementation is deliberately limited to make it easy to
> audit. It was a deliberate decision to disallow relative URLs that
> encode a Windows file path containing a drive letter.

OK, but this decision violates both the old and updated spec (and
makes it difficult to write code that works in both cases: in
situations that reject absolute URLs (javac) and in situations that
reject drive letters (this code)), so I would request that this be
revisited.

> You can of course
> use an absolute file URL here and I would expect
> "file:/C:/helloworld.jar" to work. The spec was relaxed to allow
> absolute file URLs for cases like this. I'm not opposed to expanding the
> parsing to allow for more cases but a detailed security review will be
> needed on all changes in this area.

Is there anything I can do to help?

-- 
- DML

Re: Class-Path (in jar file) semantics different between Java 11 and 13 (on Windows)?

[David Lloyd]

On Mon, Nov 18, 2019 at 9:37 AM Alan Bateman  wrote:
>
> On 18/11/2019 15:01, Jaikiran Pai wrote:
> > :
> >
> > So this Class-Path entry is being ignored starting Java 13.
> >
> Yes, bad values are now ignored, bringing an end to an effort on the
> run-time over multiple releases to fix the problems this area.
> JDK-8224253[1] is the JDK 13 release note to announce this change and I
> see you've found the system property that you can use to track down bad
> values. In previous releases you will get the same behavior with
> -Djdk.net.URLClassPath.disableClassPathURLCheck=false as the changes to
> reject bad input have been in place for some time. Brent can summarize
> but I think the only outstanding work is to fix the javac handling.

OK, having read the updated specification (thanks Alan!) I'm now quite
curious why `/C:/helloworld.jar` is considered invalid.  It is in fact
a valid relative URL (colons are allowed in path segments, and the
leading `/` unambiguously delineates the URL path), and thus it seems
that it should be considered valid.

Examining the code in URLClassPath:

static URL tryResolveFile(URL base, String input) throws MalformedURLException {
int index = input.indexOf(':'); // <-- A
boolean isFile;
if (index >= 0) {
String scheme = input.substring(0, index);
isFile = "file".equalsIgnoreCase(scheme);
} else {
isFile = true;
}
return (isFile) ? new URL(base, input) : null;
}

The line marked as `A` seems to incorrectly detect URL scheme.  If a
`/` appears before the `:` then the `:` is legitimately part of the
path, not a scheme delimiter, however this code does not check to see
if the `:` appears after a `/`.
-- 
- DML

Re: Class-Path (in jar file) semantics different between Java 11 and 13 (on Windows)?

[David Lloyd]

On Mon, Nov 18, 2019 at 9:37 AM Alan Bateman  wrote:
>
> On 18/11/2019 15:01, Jaikiran Pai wrote:
> > :
> >
> > So this Class-Path entry is being ignored starting Java 13.
> >
> Yes, bad values are now ignored, bringing an end to an effort on the
> run-time over multiple releases to fix the problems this area.
> JDK-8224253[1] is the JDK 13 release note to announce this change and I
> see you've found the system property that you can use to track down bad
> values. In previous releases you will get the same behavior with
> -Djdk.net.URLClassPath.disableClassPathURLCheck=false as the changes to
> reject bad input have been in place for some time. Brent can summarize
> but I think the only outstanding work is to fix the javac handling.
>
> -Alan
> [1] https://bugs.openjdk.java.net/browse/JDK-8224253

Where can the updated specification be found?  It has in the past been
clearly specified and well understood that class path entries are
interpreted as relative URLs.  If that has changed then this will
definitely break Quarkus and perhaps other applications as well.

A Google search for "JAR file specification" only seems to turn up
older versions (JDK 10 and earlier).
-- 
- DML

Re: RandomAccess Interface and List Heirarchy

[David Lloyd]

On Sat, Sep 28, 2019 at 3:39 AM Peter Levart  wrote:
>
> On 9/25/19 12:15 PM, Remi Forax wrote:
> > that said, i believe we should deprecate LinkedList (and any other List 
> > implementation that doesn't implement RandomAccess) because there are too 
> > much code out there that suppose that list.get() is O(1).
>
> Hi Remi,
>
> Deprecating LinkedList as a whole is maybe to harsh. LinkedList is a
> List, but it is also the only JDK implementation of single-threaded
> linked Deque, which, although a retrofitted feature, is a perfectly
> fitted feature of LinkedList.

Surely ArrayDeque is almost universally superior to LinkedList for
this use, in the same way that ArrayList has been shown to be almost
universally superior to LinkedList for list use cases?

See also 
https://stackoverflow.com/questions/322715/when-to-use-linkedlist-over-arraylist-in-java

The salient point being that in most cases the O(1) of LinkedList is
effectively slower than the O(n) of Array*.

-- 
- DML

Re: RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[David Lloyd]

On Tue, Jul 9, 2019 at 8:19 AM Jaikiran Pai  wrote:
> - In addition, I have sneaked in an unrelated change in this patch, into
> the Deflater.end() method:
>
>  public void end() {
>  synchronized (zsRef) {
> +// check if already closed
> +if (zsRef.address() == 0) {
> +return;
> +}
>  zsRef.clean();
>  input = ZipUtils.defaultBuf;
> +inputArray = null;
> +}
>
> Unlike in the Inflater.end(), the Deflater.end() didn't previously have
> the "inputArray = null". I have included it in this patch, since it
> looks right to clean up that array too. If this isn't the right
> time/patch to do it, please do let me know and I'll take that up separately.

This was probably my mistake.  The fix looks good to me!
-- 
- DML

Re: PING: RFR(s): (new approach) 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[David Lloyd]

https://www.unix.com/man-page/redhat/2/execve/ is from 1997.

On Wed, Jun 5, 2019 at 9:50 AM David Lloyd  wrote:

> If we're talking just Linux though, has this *ever* been an issue?  You've
> been able to call execve on a shell script since at least 2002 as far as I
> can tell from a quick search.  Maybe this should be conditional so that it
> can be excluded on known-good platforms?
>
> On Wed, Jun 5, 2019 at 9:40 AM Martin Buchholz 
> wrote:
>
>> https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
>>
>> """Historically, there have been two ways that implementations can exec
>> shell scripts.
>>
>> One common historical implementation is that the execl(), execv(),
>> execle(), and execve() functions return an [ENOEXEC] error for any file not
>> recognizable as executable, including a shell script. When the execlp() and
>> execvp() functions encounter such a file, they assume the file to be a
>> shell script and invoke a known command interpreter to interpret such
>> files. This is now required by POSIX.1-2017. These implementations of
>> execvp() and execlp() only give the [ENOEXEC] error in the rare case of a
>> problem with the command interpreter's executable file. Because of these
>> implementations, the [ENOEXEC] error is not mentioned for execlp() or
>> execvp(), although implementations can still give it.
>>
>> Another way that some historical implementations handle shell scripts is
>> by recognizing the first two bytes of the file as the character string "#!"
>> and using the remainder of the first line of the file as the name of the
>> command interpreter to execute.
>> """
>>
>> On Tue, Jun 4, 2019 at 11:54 PM Thomas Stüfe 
>> wrote:
>>
>>> Hi David,
>>>
>>> You are right. I am pretty sure there are ancient historical reasons for
>>> doing this by hand instead of letting exec() deal with scripts itself.
>>> Martin or Roger may know more.
>>>
>>> But I think right now this is more of a compatibility concern - changing
>>> this behavior has the risk of breaking lots of user code. Since we have not
>>> the manpower to deal with this - look at how long simple fixes are in
>>> review, we are all busy elsewhere - chances of changing this are small.
>>>
>>> Cheers, Thomas
>>>
>>>
>>> On Tue, Jun 4, 2019 at 10:14 PM David Lloyd 
>>> wrote:
>>>
>>>> In this case, going from what the execve(2) Linux man page says (and
>>>> Mac seems to agree), if the script were executable it would have a '#!'
>>>> interpreter string and the kernel would execute it anyway, would it not?
>>>> What case would be solved by explicitly starting a shell?
>>>>
>>>> On Tue, Jun 4, 2019 at 2:15 PM Roger Riggs 
>>>> wrote:
>>>>
>>>>> Hi Thomas,
>>>>>
>>>>> If the argument is not an .exe, then it may be a command shell script
>>>>> (.sh, etc.)
>>>>> Only the system knows it is not an exe (errno == ENOEXEC), so it then
>>>>> passes it as
>>>>> the first argument to /bin/sh that will handle the shell files.
>>>>>
>>>>> And yes, count me as a Reviewer and reviewed.
>>>>>
>>>>> Roger
>>>>>
>>>>> On 06/04/2019 12:14 PM, Thomas Stüfe wrote:
>>>>>
>>>>> On Tue, Jun 4, 2019 at 5:09 PM Roger Riggs 
>>>>> wrote:
>>>>>
>>>>> ...
>>>>>
>>>>>
>>>>> Then I ran an strace over it and saw this:
>>>>>
>>>>> 5332 [pid  3911] execve("./sleep2", ["./sleep2"], [/* 78 vars */]
>>>>> 
>>>>>   ..
>>>>> 5342 [pid  3911] <... execve resumed> )  = -1 ENOEXEC (Exec format
>>>>> error)
>>>>> 5343 [pid  3911] execve("/bin/sh", ["/bin/sh", "./sleep2"], [/* 78
>>>>> vars */] 
>>>>> 5347 [pid  3911] <... execve resumed> )  = 0
>>>>>
>>>>> So, if the first exec fails for whatever reason, we try again, passing
>>>>> the executable file name as argument to the system shell. This does not
>>>>> feel right? Do you know why we do this?
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks, Roger
>>>>>>
>>>>>>
>>>>> Thank you Roger. Can I consider the patch to be reviewed by you?
>>>>>
>>>>> ..Thomas
>>>>>
>>>>>
>>>>>
>>>>
>>>> --
>>>> - DML
>>>>
>>>
>
> --
> - DML
>


-- 
- DML

Re: PING: RFR(s): (new approach) 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[David Lloyd]

If we're talking just Linux though, has this *ever* been an issue?  You've
been able to call execve on a shell script since at least 2002 as far as I
can tell from a quick search.  Maybe this should be conditional so that it
can be excluded on known-good platforms?

On Wed, Jun 5, 2019 at 9:40 AM Martin Buchholz  wrote:

> https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
>
> """Historically, there have been two ways that implementations can exec
> shell scripts.
>
> One common historical implementation is that the execl(), execv(),
> execle(), and execve() functions return an [ENOEXEC] error for any file not
> recognizable as executable, including a shell script. When the execlp() and
> execvp() functions encounter such a file, they assume the file to be a
> shell script and invoke a known command interpreter to interpret such
> files. This is now required by POSIX.1-2017. These implementations of
> execvp() and execlp() only give the [ENOEXEC] error in the rare case of a
> problem with the command interpreter's executable file. Because of these
> implementations, the [ENOEXEC] error is not mentioned for execlp() or
> execvp(), although implementations can still give it.
>
> Another way that some historical implementations handle shell scripts is
> by recognizing the first two bytes of the file as the character string "#!"
> and using the remainder of the first line of the file as the name of the
> command interpreter to execute.
> """
>
> On Tue, Jun 4, 2019 at 11:54 PM Thomas Stüfe 
> wrote:
>
>> Hi David,
>>
>> You are right. I am pretty sure there are ancient historical reasons for
>> doing this by hand instead of letting exec() deal with scripts itself.
>> Martin or Roger may know more.
>>
>> But I think right now this is more of a compatibility concern - changing
>> this behavior has the risk of breaking lots of user code. Since we have not
>> the manpower to deal with this - look at how long simple fixes are in
>> review, we are all busy elsewhere - chances of changing this are small.
>>
>> Cheers, Thomas
>>
>>
>> On Tue, Jun 4, 2019 at 10:14 PM David Lloyd 
>> wrote:
>>
>>> In this case, going from what the execve(2) Linux man page says (and Mac
>>> seems to agree), if the script were executable it would have a '#!'
>>> interpreter string and the kernel would execute it anyway, would it not?
>>> What case would be solved by explicitly starting a shell?
>>>
>>> On Tue, Jun 4, 2019 at 2:15 PM Roger Riggs 
>>> wrote:
>>>
>>>> Hi Thomas,
>>>>
>>>> If the argument is not an .exe, then it may be a command shell script
>>>> (.sh, etc.)
>>>> Only the system knows it is not an exe (errno == ENOEXEC), so it then
>>>> passes it as
>>>> the first argument to /bin/sh that will handle the shell files.
>>>>
>>>> And yes, count me as a Reviewer and reviewed.
>>>>
>>>> Roger
>>>>
>>>> On 06/04/2019 12:14 PM, Thomas Stüfe wrote:
>>>>
>>>> On Tue, Jun 4, 2019 at 5:09 PM Roger Riggs 
>>>> wrote:
>>>>
>>>> ...
>>>>
>>>>
>>>> Then I ran an strace over it and saw this:
>>>>
>>>> 5332 [pid  3911] execve("./sleep2", ["./sleep2"], [/* 78 vars */]
>>>> 
>>>>   ..
>>>> 5342 [pid  3911] <... execve resumed> )  = -1 ENOEXEC (Exec format
>>>> error)
>>>> 5343 [pid  3911] execve("/bin/sh", ["/bin/sh", "./sleep2"], [/* 78 vars
>>>> */] 
>>>> 5347 [pid  3911] <... execve resumed> )  = 0
>>>>
>>>> So, if the first exec fails for whatever reason, we try again, passing
>>>> the executable file name as argument to the system shell. This does not
>>>> feel right? Do you know why we do this?
>>>>
>>>>
>>>>>
>>>>> Thanks, Roger
>>>>>
>>>>>
>>>> Thank you Roger. Can I consider the patch to be reviewed by you?
>>>>
>>>> ..Thomas
>>>>
>>>>
>>>>
>>>
>>> --
>>> - DML
>>>
>>

-- 
- DML

Re: PING: RFR(s): (new approach) 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[David Lloyd]

In this case, going from what the execve(2) Linux man page says (and Mac
seems to agree), if the script were executable it would have a '#!'
interpreter string and the kernel would execute it anyway, would it not?
What case would be solved by explicitly starting a shell?

On Tue, Jun 4, 2019 at 2:15 PM Roger Riggs  wrote:

> Hi Thomas,
>
> If the argument is not an .exe, then it may be a command shell script
> (.sh, etc.)
> Only the system knows it is not an exe (errno == ENOEXEC), so it then
> passes it as
> the first argument to /bin/sh that will handle the shell files.
>
> And yes, count me as a Reviewer and reviewed.
>
> Roger
>
> On 06/04/2019 12:14 PM, Thomas Stüfe wrote:
>
> On Tue, Jun 4, 2019 at 5:09 PM Roger Riggs  wrote:
>
> ...
>
>
> Then I ran an strace over it and saw this:
>
> 5332 [pid  3911] execve("./sleep2", ["./sleep2"], [/* 78 vars */]
> 
>   ..
> 5342 [pid  3911] <... execve resumed> )  = -1 ENOEXEC (Exec format
> error)
> 5343 [pid  3911] execve("/bin/sh", ["/bin/sh", "./sleep2"], [/* 78 vars
> */] 
> 5347 [pid  3911] <... execve resumed> )  = 0
>
> So, if the first exec fails for whatever reason, we try again, passing the
> executable file name as argument to the system shell. This does not feel
> right? Do you know why we do this?
>
>
>>
>> Thanks, Roger
>>
>>
> Thank you Roger. Can I consider the patch to be reviewed by you?
>
> ..Thomas
>
>
>

-- 
- DML

Re: RFR(s): (new approach) 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[David Lloyd]

I'm in favor of what the change is meant to accomplish.  I haven't had
time to analyze the change in detail, and I may not get time to do so.
But I'm not a reviewer in any case, so maybe that doesn't matter too
much.

On Wed, May 22, 2019 at 1:16 AM Thomas Stüfe  wrote:
>
> Ping...
>
> Guys, I need some feedback on this. If we do not fix this issue, we may want 
> to roll back the use of posix_spawn() as a default and return to vfork for 
> JDK13.
>
> The fix has been tested in our nightlies for two nights in a row and did not 
> show any errors.
>
> Cheers, Thomas
>
>
> On Mon, May 20, 2019 at 4:15 PM Thomas Stüfe  wrote:
>>
>> Hi all,
>>
>> (old mail thread: 
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/060139.html)
>>
>> May I please have your reviews and opinions for the following bug fix:
>>
>> issue: https://bugs.openjdk.java.net/browse/JDK-8223777
>> cr: 
>> http://cr.openjdk.java.net/~stuefe/webrevs/8223777-posix_spawn-no-exec-error-alternate-impl/webrev.00/webrev/
>>
>> ---
>>
>> The fix implements Florians proposal: the jspawnhelper will signal its 
>> aliveness to the parent process the moment it gains control. If the parent 
>> process does not get the signal, it assumes exec'ing the jspawnhelper never 
>> worked.
>>
>> I only do this for posix_spawn mode, out of cautiousness.
>>
>> (Note that I kept the fix as minimal as possible. I found a minor bug and 
>> some improvement possibilities and opened follow up issues to track them: 
>> JDK-8224180 and JDK-8224181).
>>
>> Thanks, Thomas
>>
>>


-- 
- DML

Re: RFR(s): 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[David Lloyd]

Ah I see.  I like the idea, it would mean pretty comprehensive error
reporting and should work across platforms too I think.

On Tue, May 14, 2019 at 7:16 AM Florian Weimer  wrote:
>
> * David Lloyd:
>
> > Pipe communication isn't very costly AFAIK.  The added complexity
> > would be waiting for either a thing to appear on the pipe indicating
> > success or else a waitpid/waitid+WNOHANG for exit code 127.  But
> > writing a thing to the pipe won't help determine if the subsequent
> > exec succeeded, which is really the interesting bit.
>
> Please have a look at the code.  It's already using CLOEXEC to cover
> the execve call (src/java.base/unix/native/libjava/ProcessImpl_md.c):
>
> switch (readFully(fail[0], &errnum, sizeof(errnum))) {
> case 0: break; /* Exec succeeded */
> case sizeof(errnum):
> waitpid(resultPid, NULL, 0);
> throwIOException(env, errnum, "Exec failed");
> goto Catch;
> default:
> throwIOException(env, errno, "Read failed");
> goto Catch;
> }
>
> Instead of 0/4 bytes, the outcome could be 0/4/8 bytes, corresponding to
> jspawnhelper exec failure, complete success, and exec failure in
> jspawnhelper.
>
> The run-time cost would be the additional pipe write in jspawnhelper.
> There shouldn't even be another ping-pong between the processes.
>
> Thanks,
> Florian



-- 
- DML

Re: RFR(s): 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[David Lloyd]

Pipe communication isn't very costly AFAIK.  The added complexity
would be waiting for either a thing to appear on the pipe indicating
success or else a waitpid/waitid+WNOHANG for exit code 127.  But
writing a thing to the pipe won't help determine if the subsequent
exec succeeded, which is really the interesting bit.

You *could* wait for the other end of the pipe to close (and write
jspawnhelper to close the write end on exec) as an indication that the
subsequent exec succeeded, while also reading from the pipe to
determine if the exec has failed (writing the errno or something).
Not sure if that's racy though, as I've only given it a minute of
thought.

On Tue, May 14, 2019 at 4:37 AM Thomas Stüfe  wrote:
>
> Neat idea!
>
> But this incurs costs for this pipe communication on every spawn. What do
> the others think?
>
> Cheers, Thomas
>
> On Tue, May 14, 2019 at 10:22 AM Florian Weimer  wrote:
>
> > * Thomas Stüfe:
> >
> > > This is impossible to fix completely - see Martin's comment about the
> > > impossibility to foresee whether an exec() will succeed without actually
> > > exec()ing. But at least we can test the execute permissions on the
> > > jspawnhelper. Which this fix does. This fixes Ubuntu 16.4 (Now, I get an
> > > IOException if jspawnhelper is not executable).
> >
> > I think jspawnhelper could write something to the pipe to show that it
> > has started.  If you never get that notification, you know that
> > jspawnhelper hasn't run, even if posix_spawn has succeeded.
> >
> > (This fix will also help qemu-user and WSL, which implement vfork as
> > fork and interfere with the internal error reporting from posix_spawn.)
> >
> > Thanks,
> > Florian
> >



-- 
- DML

Re: RFR(s): 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[David Lloyd]

On Mon, May 13, 2019 at 11:01 AM Thomas Stüfe  wrote:
>
> Hi all,
>
> may I have please your opinions about the following change:
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8223777
> webrev:
> http://cr.openjdk.java.net/~stuefe/webrevs/8223777-posix_spawn-no-exec-error/webrev.00/webrev/
> [...]
> But at least we can test the execute permissions on the
> jspawnhelper. Which this fix does. This fixes Ubuntu 16.4 (Now, I get an
> IOException if jspawnhelper is not executable).

I like it.  But I wonder if it's a good idea to test in this way:

+/* Require all bits set since this is how jspawnhelper
+ * is set up in a canonical installation */
+if (s.st_mode & S_IXUSR &&
+s.st_mode & S_IXGRP &&
+s.st_mode & S_IXOTH) {
+return 1;
+} else {

What if the JDK is not world-accessible for some reason?  This would
not seem unreasonable.  I think it would be best to err on the side of
caution: if no `x` bit is set, then it would not be executable and an
exception should be reported, but otherwise it *may* be executable so
either no error should be reported at this point, or a "smarter" check
should be done (e.g. comparing the UIDs and so forth).

-- 
- DML

Re: Process.exec with the linux posix_spawn mode has a bug

[David Lloyd]

On Sat, May 11, 2019 at 9:49 AM Remi Forax  wrote:
>
> I've seen a weird error on my CI [1] since the 15th of February when using 
> the jdk 13,
> i was not able to run jshell* using the programmatic API (java tool) anymore.
>
> Yesterday, i took the time to track that issue and i believe i've found the 
> root cause, trying to execute the java launcher using ProcessBuilder.start() 
> with the jdk 13 sometimes fails with an i/o exception (errno 13) which is 
> weird because you are trying to execute the same process that the one you are 
> executing. Note that this is a spurious bug, I was not able to find the exact 
> condition that triggers that bug.
> So sometime it works, sometime it doesn't.

Assuming you're running on Linux (given that the changeset only
applies to Linux), errno 13 is EACCES which would indicate a
filesystem permission issue.  Are you sure you have permission to
execute `jspawnhelper` in your JVM installation?  Could there be an
SELinux restriction in place?

Here's what the man page says:

```

 [EACCES]   Search permission is denied for a component of
the path prefix.


 [EACCES]   The new process file is not an ordinary file.


 [EACCES]   The new process file mode denies execute permission.


 [EACCES]   The new process file is on a filesystem
mounted with execution

disabled (MNT_NOEXEC in ).
```

> If i pass -Djdk.lang.Process.launchMechanism=fork (or vfork) when starting 
> the VM, the bug disappear meaning that the bug occurs when the VM is trying 
> to use posix_spawn, perhaps spawn is trying to do an optimization when you 
> try to execute the same process as the one you are running under some 
> conditions ?

The posix_spawn approach executes an intermediate support process
(`jspawnhelper`) which in turn executes the target process.  I believe
there's an extensive discussion on the reasons for this in the
`core-libs-dev` archive.


-- 
- DML

Re: [11u]: RFR CSR backport: JDK-8220362: Add "POSIX_SPAWN" as valid value to jdk.lang.Process.launchMechanism on Linux

[David Lloyd]

Is vfork still guaranteed to be functional by the end of the Java 11
support frame, from the perspective of any organization which supports
JDKs of this version?

On Fri, Mar 8, 2019 at 9:22 AM Andrew Haley  wrote:
>
> On 3/8/19 2:39 PM, Langer, Christoph wrote:
> > please review the CSR backport 
> > https://bugs.openjdk.java.net/browse/JDK-8220362.
> >
> > It is the backport of https://bugs.openjdk.java.net/browse/JDK-8214511 to 
> > JDK11.
> >
> > We want to backport 
> > JDK-8212828 to jdk11u and 
> > hence backporting the CSR is a prerequisite. The CSR is, however, more or 
> > less identical for 11u.
>
> I don't see any explanation of why this should go into jdk11.
>
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. 
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671



-- 
- DML

Re: Downport JDK-8212828 (posix_spawn on Linux as non-default) to 11u

[David Lloyd]

I'm in favor FWIW.
On Wed, Nov 28, 2018 at 9:36 AM Thomas Stüfe  wrote:
>
> Hi all,
>
> I would like to have this patch in 11u too, so I just did a 11u
> downport request.
>
> Our agreement was that launchMechanism=POSIX_SPAWN would stay as
> optional non-default value for jdk12, and at the start of JDK13
> development we would switch the default to posix_spawn.
>
> I would like to see this non-default-but-possible-to-set option in 11u
> too, just to broaden the test surface. We'd get more testers that way.
>
> What do you think, does that make sense?
>
> Thanks, Thomas



-- 
- DML

Re: RFR: JDK-8212828 Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[David Lloyd]

t;   34  * @run main/othervm/timeout=300 Basic
> >>   35  * @run main/othervm/timeout=300
> >> -Djdk.lang.Process.launchMechanism=fork Basic
> >>   36  * @test
> >>   37  * @requires (os.family == "liinux")
> >>   38  * @run main/othervm/timeout=300
> >> -Djdk.lang.Process.launchMechanism=posix_spawn Basic
> >>   39  * @author Martin Buchholz
> >>   40  */
> >>
> >> If I have @requires (os.family == "linux") all three variants are executed 
> >> - ok.
> >>
> >> Then I wanted to have a negative test, so I misnamed linux as "liinux"
> >> and would have expected the first @test block to fire, the second to
> >> be ignored. But in fact:
> >>
> >> thomas@mainframe /shared/projects/openjdk/jtreg $
> >> ../jtreg-prebuilt/jtreg/bin/jtreg -jdk
> >> ../jdk-jdk/output-slowdebug/images/jdk/
> >> ../jdk-jdk/source/test/jdk/java/lang/ProcessBuilder/Basic.java
> >> Test results: no tests selected
> >>
> >> So to me it looks like as if the @requires tag is valid for both @test
> >> blocks, not only the one it appears in.
> >>
> >> I'll run this through our build system too.
> >>
> >> To fully test out using posix_spawn in many more different scenarios the
> >> build system should be augmented to be able to use it as the default for
> >> an entire build/CI/CD/test runs.
> >>
> >> Sure! But I think this is out of scope for this patch.
> >>
> >> Thanks, Thomas
> >>
> >> Thanks, Roger
> >>
> >> On 10/24/2018 10:35 AM, Thomas Stüfe wrote:
> >>
> >> Hi all,
> >>
> >> version 2 of Davids patch, with test changes:
> >>
> >> http://cr.openjdk.java.net/~stuefe/webrevs/JDK-8212828-posix_spawn.patch/webrev.01/webrev/
> >>
> >> Executed the test on my local Ubuntu box, works fine. Submit job runs.
> >>
> >> About the test: I added a new line:
> >>
> >>* @run main/othervm/timeout=300 Basic
> >>* @run main/othervm/timeout=300 -Djdk.lang.Process.launchMechanism=fork 
> >> Basic
> >> + * @run main/othervm/timeout=300
> >> -Djdk.lang.Process.launchMechanism=posix_spawn Basic
> >>
> >> The way I understand those tests work is that the first line causes
> >> the test to be run with the default launch mechanism, the second line
> >> with jdk.lang.Process.launchMechanism=fork explicitly. The third line,
> >> added by me, will now explicitly test with posix_spawn. I did not
> >> limit the platforms, since posix_spawn should now be available on all
> >> platforms, so it should work on all platforms. But then, on all other
> >> platforms it has already been the default.
> >>
> >> This is still a bit iffy: On Windows, jdk.lang.Process.launchMechanism
> >> gets ignored, so we just executed the same test twice (now three
> >> times)? And now we execute the posix_spawn variant twice on all
> >> platforms where this is the default, so line 1 and 3 are the same? You
> >> see I am not a jtreg expert :) Can I specify a @run directive for only
> >> one platform? In that case I would limit the explicit posix_spawn test
> >> to Linux.
> >>
> >> Note however that if we really abondon vfork in the future and make
> >> posix_spawn the default, this test becomes simpler too.
> >>
> >> Best, Thomas
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >> On Wed, Oct 24, 2018 at 3:36 PM David Lloyd  wrote:
> >>
> >> On Wed, Oct 24, 2018 at 1:05 AM Thomas Stüfe  
> >> wrote:
> >>
> >> Review:
> >>
> >> - copyright dates need updating on the C-sources
> >>
> >> - I opt for "#if defined(__solaris__) || defined(_ALLBSD_SOURCE) ||
> >> defined(_AIX) || defined(__linux__)" to be removed completely from
> >> unix-specific source files. The ifdef now covers all OpenJDK Unix
> >> platforms.
> >>
> >> Here's a version with these changes.
> >>
> >> --
> >> - DML
> >>
> >>



-- 
- DML

Re: RFR: JDK-8212828 Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[David Lloyd]

On Wed, Oct 24, 2018 at 8:52 AM Thomas Stüfe  wrote:
>
> Hi David,
>
> I think we need some form of test, as Alan indicated.
>
> java/lang/ProcessBuilder/Basic.java should run through. Actually, I
> would like it if we would run this test for all valid enabled launch
> mechanisms, regardless which one is default. Can this be done?

I agree, however, I do not know how this can be accomplished.  Someone
who understands this test infrastructure will have to take over.

-- 
- DML

Re: RFR: JDK-8212828 Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[David Lloyd]

On Wed, Oct 24, 2018 at 1:05 AM Thomas Stüfe  wrote:
> Review:
>
> - copyright dates need updating on the C-sources
>
> - I opt for "#if defined(__solaris__) || defined(_ALLBSD_SOURCE) ||
> defined(_AIX) || defined(__linux__)" to be removed completely from
> unix-specific source files. The ifdef now covers all OpenJDK Unix
> platforms.

Here's a version with these changes.

-- 
- DML
commit c6407df740924753f3393fd9ba3af1d2722cdf1d
Author: David M. Lloyd 
Date:   Thu Oct 18 15:56:37 2018 -0500

[JDK-8212828] Enable POSIX_SPAWN as an option for child process creation on Linux

diff --git a/make/launcher/Launcher-java.base.gmk b/make/launcher/Launcher-java.base.gmk
index 0ce0287d2be..c28fe42d102 100644
--- a/make/launcher/Launcher-java.base.gmk
+++ b/make/launcher/Launcher-java.base.gmk
@@ -84,7 +84,7 @@ endif
 
 
 
-ifneq ($(findstring $(OPENJDK_TARGET_OS), macosx solaris aix), )
+ifneq ($(findstring $(OPENJDK_TARGET_OS), macosx solaris aix linux), )
   $(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \
   NAME := jspawnhelper, \
   SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \
diff --git a/src/java.base/unix/classes/java/lang/ProcessImpl.java b/src/java.base/unix/classes/java/lang/ProcessImpl.java
index 368a4f7380b..adb76ff3ec8 100644
--- a/src/java.base/unix/classes/java/lang/ProcessImpl.java
+++ b/src/java.base/unix/classes/java/lang/ProcessImpl.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -89,7 +89,7 @@ final class ProcessImpl extends Process {
 
 private static enum Platform {
 
-LINUX(LaunchMechanism.VFORK, LaunchMechanism.FORK),
+LINUX(LaunchMechanism.VFORK, LaunchMechanism.POSIX_SPAWN, LaunchMechanism.FORK),
 
 BSD(LaunchMechanism.POSIX_SPAWN, LaunchMechanism.FORK),
 
diff --git a/src/java.base/unix/native/libjava/ProcessImpl_md.c b/src/java.base/unix/native/libjava/ProcessImpl_md.c
index 533584fdb7a..c9812e2e221 100644
--- a/src/java.base/unix/native/libjava/ProcessImpl_md.c
+++ b/src/java.base/unix/native/libjava/ProcessImpl_md.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1995, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1995, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -44,9 +44,7 @@
 #include 
 #include 
 
-#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
 #include 
-#endif
 
 #include "childproc.h"
 
@@ -390,7 +388,6 @@ forkChild(ChildStuff *c) {
 return resultPid;
 }
 
-#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
 static pid_t
 spawnChild(JNIEnv *env, jobject process, ChildStuff *c, const char *helperpath) {
 pid_t resultPid;
@@ -473,7 +470,6 @@ spawnChild(JNIEnv *env, jobject process, ChildStuff *c, const char *helperpath)
  * via the statement below */
 return resultPid;
 }
-#endif
 
 /*
  * Start a child process running function childProcess.
@@ -489,10 +485,8 @@ startChild(JNIEnv *env, jobject process, ChildStuff *c, const char *helperpath)
 #endif
   case MODE_FORK:
 return forkChild(c);
-#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
   case MODE_POSIX_SPAWN:
 return spawnChild(env, process, c, helperpath);
-#endif
   default:
 return -1;
 }

Re: RFR: JDK-8212828 Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[David Lloyd]

I'm not really sure TBH.  I did some manual testing; beyond that, in a
way, the point of this change is to *get* more real-world testing.  I
haven't yet found any existing tests which try out the various process
creation options for a given platform, however.

Maybe an idea would be to have a variation on
test/jdk/java/lang/ProcessBuilder/BigFork.java which tries all of the
LaunchMechanism options (other than FORK, which I believe is known to
likely fail with this test) which are available on the current arch?
Also there's test/jdk/java/lang/ProcessBuilder/Basic.java which could
possibly be adapted to run multiple times as well (and maybe also to
cover FORK which is supported for many archs but AFAICT not tested on
archs where there are other options, i.e. all of them).  But I'm not
really sure how to make that happen with the OpenJDK test
infrastructure.

As for build changes, no I do not believe any are needed, though I am
not able to build on non-Linux platforms to be sure; I was hoping to
get a run through jdk-submit to verify that but I don't seem to have
access to that either, having no particular status in the OpenJDK
project other than sometime contributor.  Martin did suggest that
maybe the spawn native code could be modified to use config-based
macros (e.g. HAS_SPAWN_H) instead of the current platform-based
detection but I feel that's out of scope for this little change.
On Tue, Oct 23, 2018 at 2:53 PM Roger Riggs  wrote:
>
> Hi David, et.al.
>
> What would be the rest of the plan for testing?
> Usually, changes come with tests and a plan.
> What build parameters are needed to run a full set of tests with the change?
>
> Are there build changes needed?
>
> Thanks, Roger
>
>
> On 10/23/2018 03:26 PM, David Lloyd wrote:
> > My plans to try jdk/submit have fallen through unfortunately, as I
> > cannot seem to gain direct or indirect access to that system.  So I
> > guess I'm looking for any reviews on this patch now.  Thomas has
> > volunteered to sponsor.
> >
> > Thanks.
> >
> > On Tue, Oct 23, 2018 at 10:49 AM Thomas Stüfe  
> > wrote:
> >> Here you go:
> >>
> >> https://bugs.openjdk.java.net/browse/JDK-8212828
> >>
> >> If noone else steps in, I can sponsor the change for you.
> >>
> >> Cheers, Thomas
> >> On Tue, Oct 23, 2018 at 4:19 PM David Lloyd  wrote:
> >>> Sure.  I don't have any tracking information on the bugreport one I
> >>> submitted, but if you can track that down and promote it, it would
> >>> save you some typing.  Otherwise whatever you can do would be great,
> >>> thanks.
> >>> On Tue, Oct 23, 2018 at 9:02 AM Thomas Stüfe  
> >>> wrote:
> >>>> Oh, I can open a bug report on JBS for you. Should I?
> >>>>
> >>>> (Now I understand the "reuse bug id").
> >>>>
> >>>>
> >>>> On Tue, Oct 23, 2018 at 3:18 PM David Lloyd  
> >>>> wrote:
> >>>>> I've submitted a bug report via bugreport.java.com.  If/when it gets
> >>>>> promoted to a proper JIRA with an issue number, I'll see if I can put
> >>>>> the patch up on jdk/submit.
> >>>>> On Thu, Oct 18, 2018 at 4:42 PM David Lloyd  
> >>>>> wrote:
> >>>>>> The issue 6850720 isn't _exactly_ to use POSIX_SPAWN for process
> >>>>>> launching on Linux, but it's the closest I could find out of what are
> >>>>>> really a surprisingly large number of issues that refer to posix_spawn
> >>>>>> in one way or another relating to ProcessImpl.  There's a different
> >>>>>> issue to move from vfork to posix_spawn on Solaris, but I wasn't sure
> >>>>>> if that one was quite right to hang this off of.  Maybe it should be
> >>>>>> yet another issue of its own.
> >>>>>>
> >>>>>> Anyway: this is a follow-up to the email thread entitled "Runtime.exec
> >>>>>> : vfork() concerns and a fix proposal", where it was casually
> >>>>>> mentioned that maybe posix_spawn could become an option on Linux,
> >>>>>> whereafter it could be thoroughly tested by brave individuals and
> >>>>>> eventually maybe become the default on that platform, obsoleting the
> >>>>>> vfork support for good.
> >>>>>>
> >>>>>> The following patch does just that.  I've tested it launching a
> >>>>>> multi-process WildFly instance a bunch of times, in conjunction with
> >>>>>> the conveniently existent "jdk.lang.Process.launchMechanism" property,
> >>>>>> and nothing exploded so here it is.  The usual deal with git patches:
> >>>>>> apply directly through "patch -p1".
> >
> >
>


-- 
- DML

RFR: JDK-8212828 Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[David Lloyd]

My plans to try jdk/submit have fallen through unfortunately, as I
cannot seem to gain direct or indirect access to that system.  So I
guess I'm looking for any reviews on this patch now.  Thomas has
volunteered to sponsor.

Thanks.

On Tue, Oct 23, 2018 at 10:49 AM Thomas Stüfe  wrote:
>
> Here you go:
>
> https://bugs.openjdk.java.net/browse/JDK-8212828
>
> If noone else steps in, I can sponsor the change for you.
>
> Cheers, Thomas
> On Tue, Oct 23, 2018 at 4:19 PM David Lloyd  wrote:
> >
> > Sure.  I don't have any tracking information on the bugreport one I
> > submitted, but if you can track that down and promote it, it would
> > save you some typing.  Otherwise whatever you can do would be great,
> > thanks.
> > On Tue, Oct 23, 2018 at 9:02 AM Thomas Stüfe  
> > wrote:
> > >
> > > Oh, I can open a bug report on JBS for you. Should I?
> > >
> > > (Now I understand the "reuse bug id").
> > >
> > >
> > > On Tue, Oct 23, 2018 at 3:18 PM David Lloyd  
> > > wrote:
> > > >
> > > > I've submitted a bug report via bugreport.java.com.  If/when it gets
> > > > promoted to a proper JIRA with an issue number, I'll see if I can put
> > > > the patch up on jdk/submit.
> > > > On Thu, Oct 18, 2018 at 4:42 PM David Lloyd  
> > > > wrote:
> > > > >
> > > > > The issue 6850720 isn't _exactly_ to use POSIX_SPAWN for process
> > > > > launching on Linux, but it's the closest I could find out of what are
> > > > > really a surprisingly large number of issues that refer to posix_spawn
> > > > > in one way or another relating to ProcessImpl.  There's a different
> > > > > issue to move from vfork to posix_spawn on Solaris, but I wasn't sure
> > > > > if that one was quite right to hang this off of.  Maybe it should be
> > > > > yet another issue of its own.
> > > > >
> > > > > Anyway: this is a follow-up to the email thread entitled "Runtime.exec
> > > > > : vfork() concerns and a fix proposal", where it was casually
> > > > > mentioned that maybe posix_spawn could become an option on Linux,
> > > > > whereafter it could be thoroughly tested by brave individuals and
> > > > > eventually maybe become the default on that platform, obsoleting the
> > > > > vfork support for good.
> > > > >
> > > > > The following patch does just that.  I've tested it launching a
> > > > > multi-process WildFly instance a bunch of times, in conjunction with
> > > > > the conveniently existent "jdk.lang.Process.launchMechanism" property,
> > > > > and nothing exploded so here it is.  The usual deal with git patches:
> > > > > apply directly through "patch -p1".



-- 
- DML
commit f958702002c3ff7c84fbc067fa86118c4ef07cc6
Author: David M. Lloyd 
Date:   Thu Oct 18 15:56:37 2018 -0500

[JDK-8212828] Enable POSIX_SPAWN as an option for child process creation on Linux

diff --git a/make/launcher/Launcher-java.base.gmk b/make/launcher/Launcher-java.base.gmk
index 0ce0287d2be..c28fe42d102 100644
--- a/make/launcher/Launcher-java.base.gmk
+++ b/make/launcher/Launcher-java.base.gmk
@@ -84,7 +84,7 @@ endif
 
 
 
-ifneq ($(findstring $(OPENJDK_TARGET_OS), macosx solaris aix), )
+ifneq ($(findstring $(OPENJDK_TARGET_OS), macosx solaris aix linux), )
   $(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \
   NAME := jspawnhelper, \
   SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \
diff --git a/src/java.base/unix/classes/java/lang/ProcessImpl.java b/src/java.base/unix/classes/java/lang/ProcessImpl.java
index 368a4f7380b..959e50dfecd 100644
--- a/src/java.base/unix/classes/java/lang/ProcessImpl.java
+++ b/src/java.base/unix/classes/java/lang/ProcessImpl.java
@@ -89,7 +89,7 @@ final class ProcessImpl extends Process {
 
 private static enum Platform {
 
-LINUX(LaunchMechanism.VFORK, LaunchMechanism.FORK),
+LINUX(LaunchMechanism.VFORK, LaunchMechanism.POSIX_SPAWN, LaunchMechanism.FORK),
 
 BSD(LaunchMechanism.POSIX_SPAWN, LaunchMechanism.FORK),
 
diff --git a/src/java.base/unix/native/libjava/ProcessImpl_md.c b/src/java.base/unix/native/libjava/ProcessImpl_md.c
index 533584fdb7a..6869a64f2cc 100644
--- a/src/java.base/unix/native/libjava/ProcessImpl_md.c
+++ b/src/java.base/unix/native/libjava/ProcessImpl_md.c
@@ -44,7 +44,7 @@
 #include 
 #include 
 
-#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
+#

Re: 6850720: Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[David Lloyd]

Sure.  I don't have any tracking information on the bugreport one I
submitted, but if you can track that down and promote it, it would
save you some typing.  Otherwise whatever you can do would be great,
thanks.
On Tue, Oct 23, 2018 at 9:02 AM Thomas Stüfe  wrote:
>
> Oh, I can open a bug report on JBS for you. Should I?
>
> (Now I understand the "reuse bug id").
>
>
> On Tue, Oct 23, 2018 at 3:18 PM David Lloyd  wrote:
> >
> > I've submitted a bug report via bugreport.java.com.  If/when it gets
> > promoted to a proper JIRA with an issue number, I'll see if I can put
> > the patch up on jdk/submit.
> > On Thu, Oct 18, 2018 at 4:42 PM David Lloyd  wrote:
> > >
> > > The issue 6850720 isn't _exactly_ to use POSIX_SPAWN for process
> > > launching on Linux, but it's the closest I could find out of what are
> > > really a surprisingly large number of issues that refer to posix_spawn
> > > in one way or another relating to ProcessImpl.  There's a different
> > > issue to move from vfork to posix_spawn on Solaris, but I wasn't sure
> > > if that one was quite right to hang this off of.  Maybe it should be
> > > yet another issue of its own.
> > >
> > > Anyway: this is a follow-up to the email thread entitled "Runtime.exec
> > > : vfork() concerns and a fix proposal", where it was casually
> > > mentioned that maybe posix_spawn could become an option on Linux,
> > > whereafter it could be thoroughly tested by brave individuals and
> > > eventually maybe become the default on that platform, obsoleting the
> > > vfork support for good.
> > >
> > > The following patch does just that.  I've tested it launching a
> > > multi-process WildFly instance a bunch of times, in conjunction with
> > > the conveniently existent "jdk.lang.Process.launchMechanism" property,
> > > and nothing exploded so here it is.  The usual deal with git patches:
> > > apply directly through "patch -p1".
> > >
> > > commit f0eb9ff7c46dff76f91160491fcca0eb25d0ab95
> > > Author: David M. Lloyd 
> > > Date:   Thu Oct 18 15:56:37 2018 -0500
> > >
> > > [JDK-6850720] Enable POSIX_SPAWN as an option for child process
> > > creation on Linux
> > >
> > > diff --git a/make/launcher/Launcher-java.base.gmk
> > > b/make/launcher/Launcher-java.base.gmk
> > > index 0ce0287d2be..c28fe42d102 100644
> > > --- a/make/launcher/Launcher-java.base.gmk
> > > +++ b/make/launcher/Launcher-java.base.gmk
> > > @@ -84,7 +84,7 @@ endif
> > >
> > >  
> > > 
> > >
> > > -ifneq ($(findstring $(OPENJDK_TARGET_OS), macosx solaris aix), )
> > > +ifneq ($(findstring $(OPENJDK_TARGET_OS), macosx solaris aix linux), )
> > >$(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \
> > >NAME := jspawnhelper, \
> > >SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \
> > > diff --git a/src/java.base/unix/classes/java/lang/ProcessImpl.java
> > > b/src/java.base/unix/classes/java/lang/ProcessImpl.java
> > > index 368a4f7380b..959e50dfecd 100644
> > > --- a/src/java.base/unix/classes/java/lang/ProcessImpl.java
> > > +++ b/src/java.base/unix/classes/java/lang/ProcessImpl.java
> > > @@ -89,7 +89,7 @@ final class ProcessImpl extends Process {
> > >
> > >  private static enum Platform {
> > >
> > > -LINUX(LaunchMechanism.VFORK, LaunchMechanism.FORK),
> > > +LINUX(LaunchMechanism.VFORK, LaunchMechanism.POSIX_SPAWN,
> > > LaunchMechanism.FORK),
> > >
> > >  BSD(LaunchMechanism.POSIX_SPAWN, LaunchMechanism.FORK),
> > >
> > > diff --git a/src/java.base/unix/native/libjava/ProcessImpl_md.c
> > > b/src/java.base/unix/native/libjava/ProcessImpl_md.c
> > > index 533584fdb7a..6869a64f2cc 100644
> > > --- a/src/java.base/unix/native/libjava/ProcessImpl_md.c
> > > +++ b/src/java.base/unix/native/libjava/ProcessImpl_md.c
> > > @@ -44,7 +44,7 @@
> > >  #include 
> > >  #include 
> > >
> > > -#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
> > > +#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
> > > || defined(__linux__)
> > >  #include 
> > >  #endif
> > >
> > > @@ -390,7 +390,7 @@ forkChild(ChildStuff *c) {
> > >  

Re: 6850720: Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[David Lloyd]

I've submitted a bug report via bugreport.java.com.  If/when it gets
promoted to a proper JIRA with an issue number, I'll see if I can put
the patch up on jdk/submit.
On Thu, Oct 18, 2018 at 4:42 PM David Lloyd  wrote:
>
> The issue 6850720 isn't _exactly_ to use POSIX_SPAWN for process
> launching on Linux, but it's the closest I could find out of what are
> really a surprisingly large number of issues that refer to posix_spawn
> in one way or another relating to ProcessImpl.  There's a different
> issue to move from vfork to posix_spawn on Solaris, but I wasn't sure
> if that one was quite right to hang this off of.  Maybe it should be
> yet another issue of its own.
>
> Anyway: this is a follow-up to the email thread entitled "Runtime.exec
> : vfork() concerns and a fix proposal", where it was casually
> mentioned that maybe posix_spawn could become an option on Linux,
> whereafter it could be thoroughly tested by brave individuals and
> eventually maybe become the default on that platform, obsoleting the
> vfork support for good.
>
> The following patch does just that.  I've tested it launching a
> multi-process WildFly instance a bunch of times, in conjunction with
> the conveniently existent "jdk.lang.Process.launchMechanism" property,
> and nothing exploded so here it is.  The usual deal with git patches:
> apply directly through "patch -p1".
>
> commit f0eb9ff7c46dff76f91160491fcca0eb25d0ab95
> Author: David M. Lloyd 
> Date:   Thu Oct 18 15:56:37 2018 -0500
>
> [JDK-6850720] Enable POSIX_SPAWN as an option for child process
> creation on Linux
>
> diff --git a/make/launcher/Launcher-java.base.gmk
> b/make/launcher/Launcher-java.base.gmk
> index 0ce0287d2be..c28fe42d102 100644
> --- a/make/launcher/Launcher-java.base.gmk
> +++ b/make/launcher/Launcher-java.base.gmk
> @@ -84,7 +84,7 @@ endif
>
>  
> 
>
> -ifneq ($(findstring $(OPENJDK_TARGET_OS), macosx solaris aix), )
> +ifneq ($(findstring $(OPENJDK_TARGET_OS), macosx solaris aix linux), )
>$(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \
>NAME := jspawnhelper, \
>SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \
> diff --git a/src/java.base/unix/classes/java/lang/ProcessImpl.java
> b/src/java.base/unix/classes/java/lang/ProcessImpl.java
> index 368a4f7380b..959e50dfecd 100644
> --- a/src/java.base/unix/classes/java/lang/ProcessImpl.java
> +++ b/src/java.base/unix/classes/java/lang/ProcessImpl.java
> @@ -89,7 +89,7 @@ final class ProcessImpl extends Process {
>
>  private static enum Platform {
>
> -LINUX(LaunchMechanism.VFORK, LaunchMechanism.FORK),
> +LINUX(LaunchMechanism.VFORK, LaunchMechanism.POSIX_SPAWN,
> LaunchMechanism.FORK),
>
>  BSD(LaunchMechanism.POSIX_SPAWN, LaunchMechanism.FORK),
>
> diff --git a/src/java.base/unix/native/libjava/ProcessImpl_md.c
> b/src/java.base/unix/native/libjava/ProcessImpl_md.c
> index 533584fdb7a..6869a64f2cc 100644
> --- a/src/java.base/unix/native/libjava/ProcessImpl_md.c
> +++ b/src/java.base/unix/native/libjava/ProcessImpl_md.c
> @@ -44,7 +44,7 @@
>  #include 
>  #include 
>
> -#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
> +#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
> || defined(__linux__)
>  #include 
>  #endif
>
> @@ -390,7 +390,7 @@ forkChild(ChildStuff *c) {
>  return resultPid;
>  }
>
> -#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
> +#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
> || defined(__linux__)
>  static pid_t
>  spawnChild(JNIEnv *env, jobject process, ChildStuff *c, const char
> *helperpath) {
>  pid_t resultPid;
> @@ -489,7 +489,7 @@ startChild(JNIEnv *env, jobject process,
> ChildStuff *c, const char *helperpath)
>  #endif
>case MODE_FORK:
>  return forkChild(c);
> -#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
> +#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
> || defined(__linux__)
>case MODE_POSIX_SPAWN:
>  return spawnChild(env, process, c, helperpath);
>  #endif



-- 
- DML

Re: 6850720: Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[David Lloyd]

On Mon, Oct 22, 2018 at 1:23 PM Thomas Stüfe  wrote:
> Hi all,
[...]
> So far I have not read a single technical reason in this thread why
> vfork needs to be abandoned now

Note that my patch does not abandon vfork.  It does not even change
the default exec strategy away from vfork, nor does it cause any
obstacle to your proposed change.  It only adds the ability for users
to opt in to the existing posix_spawn implementation on Linux.  It's
just a first step, in whatever direction we might be going, which is
far from decided.  As you and Martin have both implied, it's a good
idea to be very cautious in this area.

> The current posix_spawn() implementation was added to glibc with glibc 2.24. 
> So, what was the state of posix_spawn() before that version? Is it safe to 
> use, does it do the right thing, or will we encounter regressions?

I certainly have not suggested making posix_spawn be the default on
Linux at this time.  But if we don't make it at least a possible
option, it cannot be tested *at all*, by anyone, unless they're
adventurous enough to hack OpenJDK themselves.  The questions you ask
can not be answered without at least making it a choice.

There are many reasonable paths that can be taken after this patch is
merged (if it is):

• It could stop here and keep the status quo; users who have trouble
with the current implementation can try out posix_spawn
• The Linux port could be modified similarly to your proposal to use
the existing jspawnhelper infrastructure with vfork on Linux, while
still allowing posix_spawn to be manually selected
• The Linux port could be modified to use posix_spawn if
gnu_get_libc_version(3) and/or other run-time probes return an
acceptable value, using vfork (either the original or your modified
approach) otherwise, still allowing posix_spawn to be manually
selected

Other possibilities exist as well.  Nothing in this email thread
(AFAICT) invalidates your basic proposal, other than perhaps the
observation that your proposal has similarities to the existing
jspawnhelper path, so that could possibly be utilized.  This patch
only *increases* our possible future choices; it does not impose any
restrictions.

-- 
- DML

Re: 6850720: Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[David Lloyd]

On Sun, Oct 21, 2018 at 7:25 PM Martin Buchholz  wrote:
> As author of the vfork strategy ...  I'm supportive of the directions in this 
> thread.

Great.

> vfork is even (!) less in favor than it used to be, so migrating off of it 
> seems good.  Just make sure we don't bring back the OOM problems or 
> unacceptable overhead from e.g. an second exec.  Create a 100GB heap, then 
> run /bin/true 1000 times.

Tried it and it succeeded without issues.  I didn't use a 100GB heap
though because I don't have that kind of RAM!  But, I created a long
chain of object arrays totalling about 8GB and forced it to stay alive
until after the 1000 execs of /bin/true and it worked fine.

To verify that the program works correctly, I compared the results
with VFORK and FORK.  As expected, VFORK also worked correctly, but
FORK failed with errno=ENOMEM.

-- 
- DML

Re: 6850720: Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[David Lloyd]

On Fri, Oct 19, 2018 at 1:56 AM Thomas Stüfe  wrote:
>
> Hi David,
>
> (for convenience, here the old thread:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055333.html)
>
> I would rather not expose a third way to spawn to the end user. As a
> test switch for developers, this seems fine.
>
> AFAIK (and someone please correct me if I am wrong) posix_spawn is not
> the magical third way beside fork and vfork, it is just a wrapper
> around fork/vfork and exec. Especially posix_spawn(), if using fork(),
> will still have the same pitfalls raw fork() has - quasi-random ENOMEM
> if we hit the overcommit heuristic, and inferior performance compared
> to vfork. So you have nothing gained, you have just relegated the
> fork/vfork decision down to a different layer, one which you have
> little control over. And since that fork/vfork decision is kind of
> important, you need that contol.

We've gained the same thing that was gained in [1] and for the same
reason: vfork is deprecated and it is an error to continue to rely
directly on it.  The only non-deprecated means of using vfork is via
posix_spawn.

And, posix_spawn is not the game of roulette that you make it out to
be.  To quote the manual page:

"The child process is created using vfork(2) instead of fork(2) when
either of the following is true:

*  the spawn-flags element of the attributes object pointed to by
attrp contains the GNU-specific flag POSIX_SPAWN_USEVFORK; or

*  file_actions is NULL and the spawn-flags element of the attributes
object  pointed  to  by  attrp  does  not  contain
POSIX_SPAWN_SETSIGMASK,  POSIX_SPAWN_SETSIGDEF,
POSIX_SPAWN_SETSCHEDPARAM, POSIX_SPAWN_SETSCHEDULER,
POSIX_SPAWN_SETPGROUP, or POSIX_SPAWN_RESETIDS.

In other words, vfork(2) is used if the caller requests it, or if
there is no cleanup expected in the child before it exec(3)s the
requested file."

It is true that we aren't explicitly requesting it, however, in our
case file_actions is NULL and attrp is also NULL, so posix_spawn will
use vfork* on Linux.

* Actually neither fork nor vfork use anything but the clone()
syscall.  To quote the sources, "The Linux implementation of
posix_spawn{p} uses the clone syscall directly with CLONE_VM and
CLONE_VFORK flags and an allocated stack."

> Note that glibc has a flag to force always vfork behavior:
> POSIX_SPAWN_USEVFORK.

Sure, but I don't really see this as necessary if glibc is already
following the vfork-like path.  Another thing to know is that at least
in the 2.26 sources I have checked out, the POSIX_SPAWN_USEVFORK flag
is completely ignored.  See also [2].

[1] https://bugs.openjdk.java.net/browse/JDK-8161360
[2] 
https://github.com/bminor/glibc/commit/ccfb2964726512f6669fea99a43afa714e2e6a80

--
- DML

6850720: Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[David Lloyd]

The issue 6850720 isn't _exactly_ to use POSIX_SPAWN for process
launching on Linux, but it's the closest I could find out of what are
really a surprisingly large number of issues that refer to posix_spawn
in one way or another relating to ProcessImpl.  There's a different
issue to move from vfork to posix_spawn on Solaris, but I wasn't sure
if that one was quite right to hang this off of.  Maybe it should be
yet another issue of its own.

Anyway: this is a follow-up to the email thread entitled "Runtime.exec
: vfork() concerns and a fix proposal", where it was casually
mentioned that maybe posix_spawn could become an option on Linux,
whereafter it could be thoroughly tested by brave individuals and
eventually maybe become the default on that platform, obsoleting the
vfork support for good.

The following patch does just that.  I've tested it launching a
multi-process WildFly instance a bunch of times, in conjunction with
the conveniently existent "jdk.lang.Process.launchMechanism" property,
and nothing exploded so here it is.  The usual deal with git patches:
apply directly through "patch -p1".

commit f0eb9ff7c46dff76f91160491fcca0eb25d0ab95
Author: David M. Lloyd 
Date:   Thu Oct 18 15:56:37 2018 -0500

[JDK-6850720] Enable POSIX_SPAWN as an option for child process
creation on Linux

diff --git a/make/launcher/Launcher-java.base.gmk
b/make/launcher/Launcher-java.base.gmk
index 0ce0287d2be..c28fe42d102 100644
--- a/make/launcher/Launcher-java.base.gmk
+++ b/make/launcher/Launcher-java.base.gmk
@@ -84,7 +84,7 @@ endif

 


-ifneq ($(findstring $(OPENJDK_TARGET_OS), macosx solaris aix), )
+ifneq ($(findstring $(OPENJDK_TARGET_OS), macosx solaris aix linux), )
   $(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \
   NAME := jspawnhelper, \
   SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \
diff --git a/src/java.base/unix/classes/java/lang/ProcessImpl.java
b/src/java.base/unix/classes/java/lang/ProcessImpl.java
index 368a4f7380b..959e50dfecd 100644
--- a/src/java.base/unix/classes/java/lang/ProcessImpl.java
+++ b/src/java.base/unix/classes/java/lang/ProcessImpl.java
@@ -89,7 +89,7 @@ final class ProcessImpl extends Process {

 private static enum Platform {

-LINUX(LaunchMechanism.VFORK, LaunchMechanism.FORK),
+LINUX(LaunchMechanism.VFORK, LaunchMechanism.POSIX_SPAWN,
LaunchMechanism.FORK),

 BSD(LaunchMechanism.POSIX_SPAWN, LaunchMechanism.FORK),

diff --git a/src/java.base/unix/native/libjava/ProcessImpl_md.c
b/src/java.base/unix/native/libjava/ProcessImpl_md.c
index 533584fdb7a..6869a64f2cc 100644
--- a/src/java.base/unix/native/libjava/ProcessImpl_md.c
+++ b/src/java.base/unix/native/libjava/ProcessImpl_md.c
@@ -44,7 +44,7 @@
 #include 
 #include 

-#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
+#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
|| defined(__linux__)
 #include 
 #endif

@@ -390,7 +390,7 @@ forkChild(ChildStuff *c) {
 return resultPid;
 }

-#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
+#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
|| defined(__linux__)
 static pid_t
 spawnChild(JNIEnv *env, jobject process, ChildStuff *c, const char
*helperpath) {
 pid_t resultPid;
@@ -489,7 +489,7 @@ startChild(JNIEnv *env, jobject process,
ChildStuff *c, const char *helperpath)
 #endif
   case MODE_FORK:
 return forkChild(c);
-#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
+#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX)
|| defined(__linux__)
   case MODE_POSIX_SPAWN:
 return spawnChild(env, process, c, helperpath);
 #endif

Re: Fwd: Re: RFR (12): 8191053: Provide a mechanism to make system's security manager immutable

[David Lloyd]

On Wed, Oct 3, 2018 at 7:53 PM Sergey Bylokhov
 wrote:
> Hi, Sean.
> One question related to SecurityManager and performance, is it possible
> to provide a special version of AccessController.doPrivileged which will
> be noop if SecurityManager is not present?

TBH that method (at least, the no-arg variant) should *always* have
been a no-op.  All it really accomplishes, in practice, is to place a
mark on the stack where the stack crawl to build the access control
context should stop (plus one frame), and this effect is something
that is already a natural consequence of a method being called in the
JVM.

The doPrivileged variant that accepts an ACC parameter should likewise
always have been no-op other than stashing the nested ACC into some
sort of thread-local (or better, a field on Thread) which can be
referred to by the aforementioned stack crawl.

The pure-java AccessController I prototyped late last year relies on
these ideas, among other things.
-- 
- DML

Re: Runtime.exec : vfork() concerns and a fix proposal

[David Lloyd]

On Wed, Sep 12, 2018 at 9:58 AM Roger Riggs  wrote:
>
> Hi David,
>
> How does your proposal differ from the current posix_spawn and
> jspawnhelper MODE_POSIX_SPAWN
> implementation available on Solaris and AIX?

Reading through the code, it ends up being pretty much identical
AFAICT; if I understand correctly, Linux would be using vfork (or its
equivalent) internally in this case.

> This area is sensitive enough that it would be prudent to implement it
> as a new mode in ProcessImpl/ProcessImpl; retaining the existing options.
> Changing the default could be a build option and would require extensive
> testing and a long stability period.

Seems worthwhile though, given vfork's now-10-year-old obsolescence.
It looks like Linux is the only platform still using vfork for
ProcessImpl in OpenJDK.

-- 
- DML

Re: Runtime.exec : vfork() concerns and a fix proposal

[David Lloyd]

On Wed, Sep 12, 2018 at 9:21 AM Martin Buchholz  wrote:
>
> I agree that your proposal makes the implementation more robust, so if
> you are willing to implement it and it doesn't cost too much, then I
> would support it.  The current implementation doesn't seem to cause
> trouble in practice, or at least we don't see any bug reports.
>
> When you benchmark with open file descriptors, try with and without
> FD_CLOEXEC flag set.  Almost all fds should be created with the
> FD_CLOEXEC flag set - openjdk is moving in that direction.  When there
> are many open files, your implementation may be a weird sort of
> optimization.
>
> On Tue, Sep 11, 2018 at 10:51 AM, Thomas Stüfe  
> wrote:
>
> > The point of this technique is that we minimize the window in the
> > child between vfork and the first exec. In fact, we are now fully
> > POSIX compliant.
>
> 2004-POSIX compliant ... but in the mean time they removed vfork().
> But vfork is critical for Google (and probably others)  due to the
> momentary overcommit problem on Linux.  Or has that changed somehow?

If the lack of POSIX support is a problem, posix_spawn could possibly
be used instead in this case:

> The child process is created using vfork(2) instead of fork(2) when [...] 
> file_actions is NULL and the spawn-flags element of the attributes object 
> pointed to by attrp does not contain POSIX_SPAWN_SETSIGMASK, 
> POSIX_SPAWN_SETSIGDEF, POSIX_SPAWN_SETSCHEDPARAM, POSIX_SPAWN_SETSCHEDULER, 
> POSIX_SPAWN_SETPGROUP, or POSIX_SPAWN_RESETIDS.

This would only work if the helper program is run directly though.

-- 
- DML

Re: Runtime.exec : vfork() concerns and a fix proposal

[David Lloyd]

On Wed, Sep 12, 2018 at 2:04 AM Thomas Stüfe  wrote:
> Hi David,
>
> On Tue, Sep 11, 2018 at 8:29 PM, David Lloyd  wrote:
> > I think this is a cool idea.
>
> Thanks. I think I did not come up with it though, I think the
> technique was known already.
>
> > Do you have any performance numbers?
>
> Sure:
>
> small program, just spawning off /bin/true a 1000 times, measured on
> my t450s running Ubuntu 16.4:
>
> Number open files:  1000   10
>
> openjdk8:  305ms 1.5s115s
> sapjvm8:   721ms  2.3s   142s
>
> factor 2.4   1.531.23

This doesn't seem too bad, even for a first implementation.  I wonder
if it could be improved further using e.g. shared memory to reduce
syscalls?

-- 
- DML

Re: Runtime.exec : vfork() concerns and a fix proposal

[David Lloyd]

I think this is a cool idea.  Do you have any performance numbers?
On Tue, Sep 11, 2018 at 12:52 PM Thomas Stüfe  wrote:
>
> Hi all,
>
> I wanted to gauge opinions on the following issue:
>
> Runtime.exec, on Linux, uses vfork(2) by default. It gives us better
> performance compared with fork() and robustness in constrained memory
> situations.
>
> But as we know vfork() can be dangerous if used incorrectly. In the
> child process before exec'ing, we live in the memory of the parent
> process. If we are not very careful we can influence or crash the
> parent process.
>
> According to POSIX pretty much the only thing the child process is
> allowed to do after vfork(2) is to exec(3) immediately; if that fails,
> you must call _exit(2).
>
> http://pubs.opengroup.org/onlinepubs/009604599/functions/vfork.html
>
> However, in the openjdk we do a number of things beyond that:
>
> - stdin,out,err pipe handling business
> - closing all file descriptors
> - we change the working directory
> - we may actually modify errno manually
> - in case exec fails, we communicate the error back to the parent using pipe.
>
> This involves calling a number of libc functions beyond exec(), namely
> read, close, dup2, opendir/readdir, write, chdir... It also needs a
> bit of stack, since we assemble path names.
>
> --
>
> I was curious whether there were any real issues, so I tested (on
> Ubuntu 16.4) and found:
>
> 1) A crash - any crash - in the child process before exec() will kill
> the parent jvm dead. Weirdly enough, we do not even enter our error
> handling, but seem to die instantly with the default "Segmentation
> Fault".
>
> 2) Signals received by the child process before exec() influence the
> parent process. For example:
>  - SIGINT set to the child ends both parent and child, immediately
>  - SIGABRT aborts both child and parent
>  - any error signal sent to the child lead to the behavior described at (1)
>
> 3) A stack overflow in the child before exec() also kills the parent.
> Unsurprising, since guard page hit -> segfault -> see (1).
>
> 4) more amusing, setting errno in the child before exec() changes the
> errno in the parent process. propagates to the parent process.
> But since errno is thread local and the thread in the parent process
> is waiting in vfork() and will, upon return, not look at errno (after
> all, vfork succeeded) this causes no trouble.
>
> There may be more issues, but these were the ones I tested.
>
> In all cases I counter-tested with fork() instead of vfork() and as
> expected  with fork() the parent process stays unaffected as it should
> be.
>
> -
>
> Whether you think these issues are worth solving is an open question.
>
> All these cases may happen in the wild (well, apart from
> crash-by-programming-error if one assumes the program to be really bug
> free) albeit with a very small probability. But once these bugs occur,
> they can be very difficult to analyse. So fixing this may be
> worthwhile.
>
> At SAP, we opted for robustness, so we changed the Runtime.exec()
> implementation to deal with vfork() issues. Basically, we employ the
> exec-twice technique:
>
> - in the child, after the vfork(), we immediately exec() into a little
> bootstrap binary ("forkhelper").
> - Now we are safe in the sense that we do not share memory with the
> parent process anymore
> - Then, parent process communicates with the child via pipes and gives
> it all information needed to do the "real" exec: environ, current dir,
> arguments... .
> - Now the child exec's a second time, this time into the real target binary.
>
> The point of this technique is that we minimize the window in the
> child between vfork and the first exec. In fact, we are now fully
> POSIX compliant. This solves the described pathological cases.
>
> It has some other advantages too, e.g. allowing for better error
> handling and tracing in the Runtime.exec() area. Performance-wise it
> depends: we exec twice, so we pay twice. However, since the parent
> continues execution after the first exec, it spends less time waiting
> on the child process, which can make a difference if there are many
> file descriptors open.
>
> ---
>
> Checking opinions here. Do you think we are okay with our current
> implementation or would a change as described above be welcome in the
> OpenJDK too?
>
> Thanks, and Best Regards, Thomas



-- 
- DML

Re: JEP 330 class loader getResourceAsStream

[David Lloyd]

On Mon, Aug 27, 2018 at 9:41 AM Alan Bateman  wrote:
>
> On 24/08/2018 18:27, David Lloyd wrote:
> > Why not go ahead and implement getResource as well?  It's not *that*
> > big of a deal to add a URL handler, and it would be a fairly trivial
> > one.
> Right, it wouldn't be too hard but it would require a bit of plumbing to
> have it backed by the Memory* classes in the source file launcher. That
> said, a newbie starting out with "java HelloWorld.java" is unlikely to
> be writing code that needs the class bytes so it's more likely the more
> advanced cases where the code is using a library that needs the bytes.

AFAIK any code would expect that resources available as streams would
generally also be available as URLs.  I'm not sure that distinguishing
between basic and advanced code really clarifies anything in terms of
the question.

-- 
- DML

Re: JEP 330 class loader getResourceAsStream

[David Lloyd]

Why not go ahead and implement getResource as well?  It's not *that*
big of a deal to add a URL handler, and it would be a fairly trivial
one.
On Fri, Aug 24, 2018 at 12:21 PM seth lytle  wrote:
>
> JEP 330 (launch single-file source programs) uses a MemoryClassLoader. the
> compiled bytecode is stored in byte arrays in memory and is never written
> to file. currently, the bytecode is not exposed as resources and it would
> be advantageous to do so (enabling the use of eg ASM to modify the classes
> to implement continuations). this class loader has no resources other than
> the bytecode
>
> getResourceAsStream would be easy to implement. getResource would be
> significantly harder - a new url scheme and associated plumbing would be
> needed
>
> jonathon gibbons wrote on compiler-dev:
> > I would defer to ClassLoader experts as to whether it is reasonable
> > to provide getResourceAsStream but not getResource.
> >
> > It might be reasonable the jdk.compiler module to provide a
> URLStreamHandlerProvider for these URLs;
> > that would require some consideration.
>
>
>
> is it reasonable to expose a resource via getResourceAsStream that's not
> available via getResource ?
>
>
> the javadocs for getResource state "returns ... null if ... a URL could not
> be constructed to locate the resource". the javadocs for
> getResourceAsStream do not include such verbage, which appears to be
> precedent for such an arrangement



-- 
- DML

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

[David Lloyd]

It may be confusing (to some, I guess) but it is consistent: the
ThreadLocal subclass author has absolute control over how the value is
presented to the user.  Adding compute() or many of the other
suggested variants will break that guarantee, which seems like kind of
a big deal to me.

What about introducing a different thread local API that has more
modern behavior?  Maybe a new subclass of ThreadLocal?

On Mon, Jun 18, 2018 at 5:28 PM Martin Buchholz  wrote:
>
> 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
>
>


-- 
- DML

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: Initial JDK 11 RFR of JDK-8202385: Annotation to mark serial-related fields and methods

[David Lloyd]

On Thu, May 10, 2018 at 7:52 PM, joe darcy  wrote:
> Hi David,
>
> On 5/10/2018 1:39 PM, David Lloyd wrote:
>>
>> Would it be allowed to enable the serial lint without using the
>> @Serial annotation?
>>
>>
>
> Certainly.
>
> A limited serial lint checker already exists in javac and we enable that
> check it in the JDK build. Basically it checks that a serializable class
> defines a static final serialVersionUID field.
>
> The expanded checks I have planned for JDK-8202056: "Expand serial warning
> to check for bad overloads of serial-related methods" for do various
> additional checks even without @Serial annotation being used. Without the
> annotation, the lint checks could verify that if a field or method with one
> of the designated names is present in a serializable class, it is declared
> properly, catching bad method overloads, etc. In addition, it could catch
> ineffective serial-related fields and methods in an enum, etc.

Great.  Then it's a +1 from me, FWIW.

-- 
- DML

Re: Initial JDK 11 RFR of JDK-8202385: Annotation to mark serial-related fields and methods

[David Lloyd]

Would it be allowed to enable the serial lint without using the
@Serial annotation?

On Thu, May 10, 2018 at 10:55 AM, joe darcy  wrote:
> Hello,
>
> Please review the webrev (code below) to address
>
> JDK-8202385: Annotation to mark serial-related fields and methods
> http://cr.openjdk.java.net/~darcy/8202385.0/
>
> The proposed java.io.Serial annotation type is intended to be used along
> with an augmented implementation of javac's "serial" lint check; that work
> will be done separately as part of JDK-8202056: "Expand serial warning to
> check for bad overloads of serial-related methods".
>
> Currently, javac's serial lint check looks for serialVersionUID fields to be
> declared in Serializable classes. However, there are various other
> structural properties of the declaration of serialization-related fields and
> methods that could be checked at compile-time. The proposed java.io.Serial
> annotation type (name subject to change [*]) explicitly marks fields and
> methods that are intended to be called as part of the Serialization
> machinery. This marking allows using the wrong name for a method or field to
> be easily caught; the serialization mechanism will generally silently ignore
> mis-declared serialization-related fields and methods.
>
> Since the annotation is intended for compile-time checking, the annotation
> type has has source retention, like the Override annotation type.
>
> Cheers,
>
> -Joe
>
> [*] Unleash the bikes from the bikeshed! On the name and package of the
> annotation type, since the other serialization types are in the java.io
> package and the checks are not proposed to be part of the language, this
> annotation type more appropriately lives in java.io package rather than
> java.lang. The name "Serial" is consistent with the "@serial" javadoc tag.
> Other possible names include "Serialize" and "SerialRelated". Another
> possibility would be to have the annotation type be a nested type in
> java.io.Serializable.
>
> -=-=-=-=-=-=-
>
> // GPLv2 elided.
>
> package java.io;
>
> import java.lang.annotation.*;
>
> /**
>  * Indicates that a field or method is related to the {@linkplain
>  * Serializable serialization mechanism}. This annotation type is
>  * intended to allow compile-time checking of serialization-related
>  * declarations, analogous to the checking enabled by the {@link
>  * java.lang.Override} annotation type to validate method overriding.
>  *
>  * Specifically, annotations of this type are intended to be
>  * applied to serialization-related methods and fields in classes
>  * declared to be {@code Serializable}. The five serialization-related
>  * methods are:
>  *
>  * 
>  * {@code private void writeObject(java.io.ObjectOutputStream stream)
> throws IOException}
>  * {@code private void readObject(java.io.ObjectInputStream stream)
> throws IOException, ClassNotFoundException}
>  * {@code private void readObjectNoData() throws ObjectStreamException}
>  * ANY-ACCESS-MODIFIER {@code Object writeReplace() throws
> ObjectStreamException}
>  * ANY-ACCESS-MODIFIER {@code Object readResolve() throws
> ObjectStreamException}
>  * 
>  *
>  * The two serialization-related fields are:
>  *
>  * 
>  * {@code private static final ObjectStreamField[]
> serialPersistentFields}
>  * {@code private static final long serialVersionUID}
>  * 
>  *
>  * A compiler can validate that a method or field marked with a
>  * @Serial annotation is one of the defined
> serialization-related
>  * methods declared in a meaningful context.
>  *
>  * It is a semantic error to apply this annotation to other fields or
> methods, including:
>  * 
>  * fields or methods in a class that is not {@code Serializable}
>  *
>  * fields or methods of the proper structural declaration, but in
>  * a type where they are ineffectual. For example, {@code enum} types
>  * are defined to have a {@code serialVersionUID} of {@code 0L} so a
>  * {@code serialVersionUID} field declared in an {@code enum} type is
>  * ignored. The five serialization-related methods identified above
>  * are likewise ignored for an {@code enum} type.
>  *
>  * 
>  */
> @Target({ElementType.METHOD, ElementType.FIELD})
> @Retention(RetentionPolicy.SOURCE)
> public @interface Serial {
> }
>



-- 
- DML

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

[David Lloyd]

I'm not a reviewer, but I would ask: how sure are we that it's OK to
use lambdas from here?  Is there a chance that the magic bootstrap
stuff won't yet be initialized at the time when an early thread could
exit for whatever reason?

Anyway I think using a lambda with forEach is probably overkill in
atThreadExit.  A simple for loop would suffice IMO.

Also, I would be quite surprised if there wasn't a way to get a system
property from system code without having to use doPrivileged; that
might bear some researching.

Could this all be simplified to drop the necessity for passing around
the thread local map and thread local instances?  The sun.nio.ch.Util
has static access to its ThreadLocal, so really all it needs is to be
able to give a Runnable to run at thread exit and it can take care of
the rest.  It could be a simple as keeping an ArrayList of Runnables
on Thread or in another ThreadLocal.

On Tue, May 8, 2018 at 10:07 AM, Tony Printezis  wrote:
> Hi all,
>
> Following the discussion on this a few weeks ago, here’s the first version
> of the change:
>
> http://cr.openjdk.java.net/~tonyp/8202788/webrev.0/
>
> I think the consensus was that it’d be easier if the exit hooks were only
> available within java.base. Is it enough that I added the functionality to
> the jdk.internal.misc package? (And is jdk.internal.misc the best place for
> this?)
>
> I’ll also add a test for the new functionality. But let’s first come up
> with an approach that everyone is happy with. :-)
>
> One additional question: I put the new functionality conditional on a
> property (jdk.nio.freeBuffersAtThreadExit) and it’s off by default. Should
> I make it on by default? Or just not add the property all-together?
>
> Tony
>
>
> —
> Tony Printezis | @TonyPrintezis | tprinte...@twitter.com



-- 
- DML

Re: Charsets for hex/base64

[David Lloyd]

I feel like what you're looking for is really a general-purpose data
transformation API.  I think bending charsets into this shape would be
a bad move, but I like the idea of a more generalized solution.
Google Guava has such an abstraction, and I know of a couple of others
a well.

On Thu, May 3, 2018 at 4:07 AM, Jonas Konrad  wrote:
> Well technically there is some sort of precedent to this, since
> CharsetEncoder/Decoder operate on CharBuffers which are just utf-16 encoded
> strings. So charsets already may produce a single output code *unit* for
> multiple input code units (UTF-32, which may output 1 code unit for 2 UTF-16
> input code units / chars). Of course, consuming multiple code points would
> be new but code points aren't really part of the CharBuffer api.
>
> - Jonas
>
>
> On 05/02/2018 05:29 PM, Weijun Wang wrote:
>>
>>
>>
>>> On May 2, 2018, at 4:35 PM, Jonas Konrad  wrote:
>>>
>>> "0a0b0c".getBytes(HexCharset.getInstance()) = new byte[] { 0x0a, 0x0b,
>>> 0x0c }
>>> new String(new byte[] { 0x0a, 0x0b, 0x0c }, HexCharset.getInstance()) =
>>> "0a0b0c"
>>
>>
>> Normally a charset is to encode a string to byte[], but here you can
>> actually decoding a string to byte[]. This would lead to quite some concept
>> differences.
>>
>> For example, we can say if a char is encodable for a charset, but for the
>> HEX "charset", you will have to say what combination of chars is
>> "encodable".
>>
>> --Max
>>
>



-- 
- DML

Re: [PATCH] minor regex cleanup: use switch for enum

[David Lloyd]

On Mon, Apr 23, 2018 at 7:42 PM, Isaac Levy  wrote:
> On Mon, Apr 23, 2018 at 5:18 PM David Lloyd  wrote:
>>
>> FWIW I strongly doubt this will improve performance; probably the
>> opposite in fact, as IIRC an enum switch generates an extra class
>> (though perhaps this has changed).  The original code was quite
>> compact and utilized identity comparisons, and given there are only
>> three alternatives it probably was able to exploit branch prediction
>> as well (if such a thing even matters in this context).
>
>
> Well, there are enum switches on the same enum elsewhere in the class, so
> should those instead be replaced by if checks?

I think that any change that's made for performance should be tested
against performance regression, generally speaking.  My personal
understanding is that, when there are a small number of alternatives,
an identity "if" tree can perform slightly better in some cases
because HotSpot C2 gathers and utilizes statistics about branches
taken in these cases; for switch, it (still IIRC) does not do so.
Given that this is regex, it might be worth testing.

> A larger change could remove this branch entirely, with different classes for 
> each of the types, which
> are known during compile.

If that is beneficial.  However every new class adds metaspace usage
and (in this case) some kind of polymorphic dispatch, so you'd want to
be fairly confident that in a typical application, only one of the two
would be loaded, or that there would be some other significant gain,
as there is definitely a cost.  Everything has a cost, especially in
hot perf-sensitive code.

Anyway I'm not a reviewer but these are considerations that I would
have were I contributing the change.  JMH might be of use.

-- 
- DML

Re: [PATCH] minor regex cleanup: use switch for enum

[David Lloyd]

FWIW I strongly doubt this will improve performance; probably the
opposite in fact, as IIRC an enum switch generates an extra class
(though perhaps this has changed).  The original code was quite
compact and utilized identity comparisons, and given there are only
three alternatives it probably was able to exploit branch prediction
as well (if such a thing even matters in this context).

I'm not a reviewer but this change seems pointless without supporting perf data.

On Mon, Apr 23, 2018 at 4:05 PM, Xueming Shen  wrote:
>
> I would assume in case of an exception thrown from
> appendExpandedReplacement() we don't
> want "text" to be pushed into the "sb".
>
> -sherman
>
> On 4/23/18, 1:49 PM, Isaac Levy wrote:
>>
>> Thanks. Another small perf patch below -- maybe we can combine. Avoids a
>> StringBuilder allocation:
>>
>> --- a/src/java.base/share/classes/java/util/regex/Matcher.java
>> +++ b/src/java.base/share/classes/java/util/regex/Matcher.java
>> @@ -993,13 +993,11 @@
>> public Matcher appendReplacement(StringBuilder sb, String replacement)
>> {
>>  // If no match, return error
>>  if (first < 0)
>>  throw new IllegalStateException("No match available");
>> -StringBuilder result = new StringBuilder();
>> -appendExpandedReplacement(replacement, result);
>>  // Append the intervening text
>>  sb.append(text, lastAppendPosition, first);
>>  // Append the match substitution
>> +appendExpandedReplacement(replacement, sb);
>> -sb.append(result);
>>
>>
>> On Mon, Apr 23, 2018 at 4:31 PM, Xueming Shen > > wrote:
>> > this looks fine.
>> >
>> > -sherman
>
>



-- 
- DML

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

[David Lloyd]

OK thanks for the update.  It seems odd to me - C99 has been around
for a pretty long time - but, as long as the change goes in, I'm
happy.  Thanks!

On Thu, Apr 19, 2018 at 1:13 AM, Xueming Shen  wrote:
> David,
>
> webrev has been updated to address the compiler error on solaris-sparcv9.
> The C comipler on
> solaris-sparcv9 platform does not like those "declaration follow a
> statement" new code in
> Deflater.c and Inflater.c
>
> http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev/
>
> Thanks,
> Sherman
>
>
> On 4/18/18, 1:16 PM, David Lloyd wrote:
>
> +1 from me, thanks!
>
> On Wed, Apr 18, 2018 at 3:09 PM, Xueming Shen 
> wrote:
>
> Alan, David,
>
> Any more update/comment/suggestion on this one? I have updated
> DeInflate.java with
> some new test cases to cover the newly added methods. Good to go?
>
> -Sherman
>
> On 04/10/2018 08:49 PM, Xueming Shen wrote:
>
>
> http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev/
>
> Thanks,
> Sherman
>
>
>
>



-- 
- DML

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

[David Lloyd]

+1 from me, thanks!

On Wed, Apr 18, 2018 at 3:09 PM, Xueming Shen  wrote:
> Alan, David,
>
> Any more update/comment/suggestion on this one? I have updated
> DeInflate.java with
> some new test cases to cover the newly added methods. Good to go?
>
> -Sherman
>
> On 04/10/2018 08:49 PM, Xueming Shen wrote:
>>
>>
>>
>> http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev/
>>
>> Thanks,
>> Sherman
>>
>



-- 
- DML

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

[David Lloyd]

That's great news, thanks!  I'll pull down your updates just so my
local copy does not get out of date in the meantime.

On Tue, Apr 10, 2018 at 10:49 PM, Xueming Shen  wrote:
> Hi David,
>
> The CSR has been approved
> https://bugs.openjdk.java.net/browse/JDK-8200527
>
> API docs have been updated slightly based on the review suggestion.
>
> (1) added some words in the class spec for both Inflater and Deflater.
>
> * 
>  * This class deflates sequences of bytes into ZLIB compressed data format.
>  * The input byte sequence is provided in either byte array or byte buffer,
>  * via one of the {@code setInput()} methods. The output byte sequence is
>  * written to the output byte array or byte buffer passed to the
>  * {@code deflate()} methods.
>  * 
>
> * 
>  * This class inflates sequences of ZLIB compressed bytes. The input byte
>  * sequence is provided in either byte array or byte buffer, via one of the
>  * {@code setInput()} methods. The output byte sequence is written to the
>  * output byte array or byte buffer passed to the {@code deflate()} methods.
>  * 
>
>
> (2) adjusted the workding a little for those setInput() methods
>
>  * 
>  * One of the {@code setInput()} methods should be called whenever
>  * {@code needsInput()} returns true indicating that more input data
>  * is required.
>  * 
>
>
> Two issues have been noticed when running tier1/2/3 tests
>
> (1) there is a error at ln#Inflater.c#243, the "input" is being released
> instead of "output"
>
> http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev.00/src/java.base/share/native/libzip/Inflater.c.sdiff.html
>
> which triggered crash for some tests. fixed.
>
> (2) sun/nio/ch/TestMaxCachedBufferSize.java failed "because of" the
> "defaultBuf"
>  uses direct ByteBuffer. This is probably the issue of the test but I
> simply update
>  the "defaultBuf" to be the heap buffer/0, instead of touch the failed
> test case.
>  I don't have problem if you prefer to "fix" the test and keep the
> "defaultBuf" as
>  direct buffer instead.
>
>// static final ByteBuffer defaultBuf = ByteBuffer.allocateDirect(0);
> static final ByteBuffer defaultBuf = ByteBuffer.allocate(0);
>
> (3) I also updated test/jdk/java/util/zip/DeInflate.java with more tests for
> the new APIs.
>  More tests might be desired though.
>
> The latest webrev is at
>
> http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev/
>
> Thanks,
> Sherman
>



-- 
- DML

Re: Promptly freeing the per-thread cached direct buffers when a thread exits

[David Lloyd]

On Fri, Apr 6, 2018 at 8:57 AM, Tony Printezis  wrote:
>> ThreadLocal clearing
>
> Could you clarify what you mean by ThreadLocal clearing?

I mean calling ThreadLocal#remove().

> I like the suggestion to add an overridable exit() method to ThreadLocal. If
> you want to avoid calling user code by Thread::exit, would adding
> ThreadLocals (which are tagged appropriately) to a queue for later
> processing a better approach (similar to the mechanism used for References /
> ReferencesQueues)? The user can of course create a memory leak by not
> polling the queue frequently enough. But, that’s also the case for
> References. And at least user code cannot block Thread::exit.

It's more complexity, and at some point you have to ask: is it better
to block thread A or thread B?  At least blocking thread A is somewhat
expected.

-- 
- DML

Re: Promptly freeing the per-thread cached direct buffers when a thread exits

[David Lloyd]

On Thu, Apr 5, 2018 at 4:45 PM, Tony Printezis  wrote:
> Would proposing to introduce thread exit hooks be reasonable? Or is
> everyone going to freak out? :-) The hooks can be either per-Thread or even
> per-ThreadLocal. And it's OK if the hooks can only be used within java.base.

User-accessible thread exit hooks would be nice, from a user
perspective.  From a JDK perspective - it adds new opportunities for
user code to jam things up, so it's a tradeoff.

ThreadLocal clearing on thread exit would have been nice back in the
beginning, but now I think it would be a fairly substantial behavior
change.  Adding a new exit() method on ThreadLocal would be better
(but not perfect) compatibility-wise, and see prior note about users
jamming things up...

I think that at a minimum, explicitly releasing thread-local NIO
direct buffers on thread exit (without introducing a user facing API)
is probably safe.  Some kind of user-accessible hook would be nice,
but I can't imagine it would take anything less than a massive
discussion to get there.

-- 
- DML

Re: Patch fixing JDK-8176553

[David Lloyd]

Actually I've talked to Jan and we're going to try again with the more
correct subject line & RFR.  Thanks.

On Wed, Apr 4, 2018 at 8:51 AM, David Lloyd  wrote:
> Is there anyone who would be willing to sponsor this change?
>
> On Tue, Jan 23, 2018 at 4:58 PM, Jan Kalina  wrote:
>> Hi,
>> I has prepared trivial patch for bug JDK-8176553, which I has reported.
>>
>> I will welcome if it could be merged into JDK.
>> (The bug is present in JDK9 and JDK8 too.)
>> I am covered by Red Hat OCA.
>>
>> The patch is attached, bug reproducer is already in JIRA:
>> https://bugs.openjdk.java.net/browse/JDK-8176553
>>
>> Thanks,
>> Jan Kalina
>
>
>
> --
> - DML



-- 
- DML

Re: Patch fixing JDK-8176553

[David Lloyd]

Is there anyone who would be willing to sponsor this change?

On Tue, Jan 23, 2018 at 4:58 PM, Jan Kalina  wrote:
> Hi,
> I has prepared trivial patch for bug JDK-8176553, which I has reported.
>
> I will welcome if it could be merged into JDK.
> (The bug is present in JDK9 and JDK8 too.)
> I am covered by Red Hat OCA.
>
> The patch is attached, bug reproducer is already in JIRA:
> https://bugs.openjdk.java.net/browse/JDK-8176553
>
> Thanks,
> Jan Kalina



-- 
- DML

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

[David Lloyd]

On Fri, Mar 30, 2018 at 12:05 PM, Xueming Shen  wrote:
>
> Hi David,
>
> (1) Deflater.setDictionary(ByteBuffer) missed a "@since 11"

Oops, easy fix.

> (2) infalte(...)  may not need to catch DFE twice, better (?) to just update
> the inputPos/input
>  at outsider catch as
>
>  if (input == null)
>  this.inputPos = inputPos + inputConsumed;
>  else
>  input.position(inputPos + inputConsumed;
>
> the if/else might be repetitive, but better than two layers of same DFE
> catch?

I'm okay with anything at this point, but this does introduce a
"inputPos possibly not initialized" problem which doesn't seem to have
a clean solution.  WDYT?

-- 
- DML

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

[David Lloyd]

Thanks!

On Fri, Mar 30, 2018 at 11:52 AM, Xueming Shen  wrote:
> On 3/30/18, 7:07 AM, Alan Bateman wrote:
>>
>> On 29/03/2018 13:18, David Lloyd wrote:
>>>
>>> :
>>> OK great.  In that case, I think all feedback has been accounted for,
>>> and this should be ready to go AFAIK.
>>>
>> I skimmed through the patch attached to your last mail. I also saw
>> Sherman's mail offering to look at the existing wording about the flush
>> marker. So I think this the API is good and we should get the CSR submitted.
>>
>> I'm less sure about the tests. The patch modifies FlaterTest but it's not
>> clear that is covers all the scenarios, ReadOnlyBufferException just one
>> that comes to mind. Maybe we could identify additional tests while the CSR
>> is in progress.
>>
>> -Alan
>
> CSR: https://bugs.openjdk.java.net/browse/JDK-8200527



-- 
- DML

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

[David Lloyd]

On Wed, Mar 28, 2018 at 11:38 PM, Xueming Shen  wrote:
> On 3/28/18, 6:14 AM, David Lloyd wrote:
>> I've implemented all the other changes except for this one.  Latest
>> version is attached, online version is here [1].
>
> "flush marker" was copy/pasted from the original zlib.h api doc when I added 
> the
> support for different flush options. It might be better to put it is a 
> apiNote. A
> "flush marker" in this case is a 5-byte empty block, which will be output if 
> the
> deflater does not have enough space to output the bytes and it is in 
> full_flush or
> sync_flush mode. I think you can leave it as is for now and I will try to see 
> if I
> can move it around or add some understandable wording, if I can.

OK great.  In that case, I think all feedback has been accounted for,
and this should be ready to go AFAIK.

Thanks!
-- 
- DML

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

[David Lloyd]

On Tue, Mar 27, 2018 at 11:59 AM, David Lloyd  wrote:
> On Tue, Mar 27, 2018 at 10:10 AM, Alan Bateman  
> wrote:
>> On 27/03/2018 00:09, David Lloyd wrote:
>>> I was going for some kind of consistency with the array variants (that
>>> is, name the parameter what it is), though they simply use "b" for
>>> their parameter name so that interpretation might be a stretch.
>>> Should I update them all?
>>
>> I think this would be good, if you don't mind doing it.
>
> OK.
>
>>> I'm not 100% familiar with the new JavaDoc categories (ok I'm 0%
>>> familiar), but the JEP for these says "This category consists of
>>> commentary, rationale, or examples pertaining to the API.".  But this
>>> feels more like specification to me since it is "specifications that
>>> apply to all valid implementations, including preconditions,
>>> postconditions, etc.".  Which is to say, if you don't provide enough
>>> remaining space in the output buffer, you will have incorrect
>>> operation as a result.  WDYT?
>>
>> The current wording (which pre-dates your changes of course) reads more like
>> API advice. I think it's a bit confusing too as it doesn't define what a
>> "flush marker" is. I think we will need to re-word that sentence to make it
>> clearer.
>
> OK.  I am at a loss for a better way to explain it though; any suggestions?

I've implemented all the other changes except for this one.  Latest
version is attached, online version is here [1].

[1] https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v18


-- 
- DML
commit 55e4df2ab3c7ccb68426bc90f3cc5d2d1a3de96e
Author: David M. Lloyd 
Date:   Fri Feb 16 11:00:10 2018 -0600

[JDK-6341887] Update Inflater/Deflater to handle ByteBuffer

diff --git a/make/mapfiles/libzip/mapfile-vers 
b/make/mapfiles/libzip/mapfile-vers
index d711d8e17f4..11ccc2d6ecb 100644
--- a/make/mapfiles/libzip/mapfile-vers
+++ b/make/mapfiles/libzip/mapfile-vers
@@ -33,20 +33,28 @@ SUNWprivate_1.1 {
Java_java_util_zip_CRC32_update;
Java_java_util_zip_CRC32_updateBytes0;
Java_java_util_zip_CRC32_updateByteBuffer0;
-   Java_java_util_zip_Deflater_deflateBytes;
+   Java_java_util_zip_Deflater_deflateBytesBytes;
+   Java_java_util_zip_Deflater_deflateBytesBuffer;
+   Java_java_util_zip_Deflater_deflateBufferBytes;
+   Java_java_util_zip_Deflater_deflateBufferBuffer;
Java_java_util_zip_Deflater_end;
Java_java_util_zip_Deflater_getAdler;
Java_java_util_zip_Deflater_init;
Java_java_util_zip_Deflater_initIDs;
Java_java_util_zip_Deflater_reset;
Java_java_util_zip_Deflater_setDictionary;
+   Java_java_util_zip_Deflater_setDictionaryBuffer;
Java_java_util_zip_Inflater_end;
Java_java_util_zip_Inflater_getAdler;
-   Java_java_util_zip_Inflater_inflateBytes;
+   Java_java_util_zip_Inflater_inflateBytesBytes;
+   Java_java_util_zip_Inflater_inflateBytesBuffer;
+   Java_java_util_zip_Inflater_inflateBufferBytes;
+   Java_java_util_zip_Inflater_inflateBufferBuffer;
Java_java_util_zip_Inflater_init;
Java_java_util_zip_Inflater_initIDs;
Java_java_util_zip_Inflater_reset;
Java_java_util_zip_Inflater_setDictionary;
+   Java_java_util_zip_Inflater_setDictionaryBuffer;
ZIP_Close;
ZIP_CRC32;
ZIP_FreeEntry;
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java 
b/src/java.base/share/classes/java/util/zip/Deflater.java
index c75dd4a33f0..85e3debb393 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -26,7 +26,13 @@
 package java.util.zip;
 
 import java.lang.ref.Cleaner.Cleanable;
+import java.lang.ref.Reference;
+import java.nio.ByteBuffer;
+import java.nio.ReadOnlyBufferException;
+import java.util.Objects;
+
 import jdk.internal.ref.CleanerFactory;
+import sun.nio.ch.DirectBuffer;
 
 /**
  * This class provides support for general purpose compression using the
@@ -92,8 +98,9 @@ import jdk.internal.ref.CleanerFactory;
 public class Deflater {
 
 private final DeflaterZStreamRef zsRef;
-private byte[] buf = new byte[0];
-private int off, len;
+private ByteBuffer input = ZipUtils.defaultBuf;
+private byte[] inputArray;
+private int inputPos, inputLim;
 private int level, strategy;
 private boolean setParams;
 private boolean finish, finished;
@@ -170,9 +177,14 @@ public class Deflater {
  */
 public static 

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

[David Lloyd]

On Tue, Mar 27, 2018 at 10:10 AM, Alan Bateman  wrote:
> On 27/03/2018 00:09, David Lloyd wrote:
>> I was going for some kind of consistency with the array variants (that
>> is, name the parameter what it is), though they simply use "b" for
>> their parameter name so that interpretation might be a stretch.
>> Should I update them all?
>
> I think this would be good, if you don't mind doing it.

OK.

>> I'm not 100% familiar with the new JavaDoc categories (ok I'm 0%
>> familiar), but the JEP for these says "This category consists of
>> commentary, rationale, or examples pertaining to the API.".  But this
>> feels more like specification to me since it is "specifications that
>> apply to all valid implementations, including preconditions,
>> postconditions, etc.".  Which is to say, if you don't provide enough
>> remaining space in the output buffer, you will have incorrect
>> operation as a result.  WDYT?
>
> The current wording (which pre-dates your changes of course) reads more like
> API advice. I think it's a bit confusing too as it doesn't define what a
> "flush marker" is. I think we will need to re-word that sentence to make it
> clearer.

OK.  I am at a loss for a better way to explain it though; any suggestions?

>>> I don't have cycles just now to go through all the implementation but I
>>> think Sherman is doing that. It will need careful review to avoid being
>>> abused to attack memory outside of the buffer. I did check the use of
>>> position() and limit() to calculate the remaining and these need correct.
>>
>> I hope this was a typo of "these seem correct" and not a typo of
>> "these need correcting"?
>
> Oops, this was indeed a typo in my mail.

OK great.

-- 
- DML

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

[David Lloyd]

On Mon, Mar 26, 2018 at 9:53 AM, Alan Bateman  wrote:
> On 23/03/2018 19:17, David Lloyd wrote:
>>
>> All fixed.  You're right, the catch blocks consolidated fairly easily
>> and it looks better.  I've attached the latest version of the patch.
>>
> I went through the APIs and javadoc changes attached to your last mail.
>
> This version addresses most of the points I brought up a few weeks ago and
> the API generally looks good. The only methods that I'm not sure about is
> the setDictionary variants as I don't see a bit need for these.

I think they may have use in some cases e.g. reading a dictionary from
a file, and generally in code which prefers byte buffers over arrays
(completely outside of the direct/heap buffer question), and they were
easy enough to implement.  But I'm okay dropping these methods if that
is what is desired.

> Deflater.setInput currently has "The given buffer's position will be updated
> ...". I think could be clearer by saying that the buffer position s
> advanced by the deflate operations up to its limit. This will make it more
> consistent with the wording in the deflate methods.

OK.

> I also wonder if the parameter should be renamed to "input" or "source".

I was going for some kind of consistency with the array variants (that
is, name the parameter what it is), though they simply use "b" for
their parameter name so that interpretation might be a stretch.
Should I update them all?

> The deflate methods talk about "remaining space" which is a bit inconsistent
> the buffer APIs where it uses "bytes remaining". I think we should try to
> keep this as consistent as possible.

OK.

> Also the usage advice, "Make sure the buffer's remaining space ..." should
> probably be moved to an @apiNote (this goes for the existing deflate methods
> too).

I'm not 100% familiar with the new JavaDoc categories (ok I'm 0%
familiar), but the JEP for these says "This category consists of
commentary, rationale, or examples pertaining to the API.".  But this
feels more like specification to me since it is "specifications that
apply to all valid implementations, including preconditions,
postconditions, etc.".  Which is to say, if you don't provide enough
remaining space in the output buffer, you will have incorrect
operation as a result.  WDYT?

> I don't have cycles just now to go through all the implementation but I
> think Sherman is doing that. It will need careful review to avoid being
> abused to attack memory outside of the buffer. I did check the use of
> position() and limit() to calculate the remaining and these need correct.

I hope this was a typo of "these seem correct" and not a typo of
"these need correcting"?

> Style wide then we should try to keep thing consistent with the existing
> code as possible (most of the "final" usages are a distraction and aren't
> needed for example).

OK I can remove them; it's been my personal convention for so many
years that I don't see them when they're there (only when they're
missing).  But removing them here is OK with me.


-- 
- DML

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

[David Lloyd]

On Fri, Mar 23, 2018 at 1:06 PM, Alan Bateman  wrote:
> Is there an up to date webrev on cr.openjdk.java.net that we can review?

I'm an outside contributor so I don't have any easy way to prepare
one; however, if you wish, you can look at the diff on my GitHub
mirror here [1].  In case you're curious about the "version", it's the
16th local iteration for me, even though the updated patch I've just
sent is only the 6th version sent to the list.

[1] https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v16

-- 
- DML

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

[David Lloyd]

On Fri, Mar 23, 2018 at 12:51 PM, Xueming Shen  wrote:
>
> Hi David,
>
> Sorry for the late response. I will sponsor this change. Will prepare for
> the CSR for
> the new APIs.

Thanks!

> Couple more comments
>
> (1) Deflater.java/deflate(ByteBuffer, int);
> "In the case of FULL_FLUSH or SYNC_FLUSH ..."
> [...]
> (2) Inflater.java/setDictionary(ByteBuffer);
> "Sets the preset dictionary to the given array of bytes"
> --> bytes in the specified byte buffer?
>
> (3) Infalter.java.inflater(...)
> It appears there is opportunity to "consolidate" some of the repetitive code
> block,
> [...]
> for example the "catch (DFE e) { ...} the only difference is the

All fixed.  You're right, the catch blocks consolidated fairly easily
and it looks better.  I've attached the latest version of the patch.

-- 
- DML
commit 2c798586b3451ba9cfe3069e5ee2699caf2c38fb
Author: David M. Lloyd 
Date:   Fri Feb 16 11:00:10 2018 -0600

[JDK-6341887] Update Inflater/Deflater to handle ByteBuffer

diff --git a/make/mapfiles/libzip/mapfile-vers 
b/make/mapfiles/libzip/mapfile-vers
index d711d8e17f4..11ccc2d6ecb 100644
--- a/make/mapfiles/libzip/mapfile-vers
+++ b/make/mapfiles/libzip/mapfile-vers
@@ -33,20 +33,28 @@ SUNWprivate_1.1 {
Java_java_util_zip_CRC32_update;
Java_java_util_zip_CRC32_updateBytes0;
Java_java_util_zip_CRC32_updateByteBuffer0;
-   Java_java_util_zip_Deflater_deflateBytes;
+   Java_java_util_zip_Deflater_deflateBytesBytes;
+   Java_java_util_zip_Deflater_deflateBytesBuffer;
+   Java_java_util_zip_Deflater_deflateBufferBytes;
+   Java_java_util_zip_Deflater_deflateBufferBuffer;
Java_java_util_zip_Deflater_end;
Java_java_util_zip_Deflater_getAdler;
Java_java_util_zip_Deflater_init;
Java_java_util_zip_Deflater_initIDs;
Java_java_util_zip_Deflater_reset;
Java_java_util_zip_Deflater_setDictionary;
+   Java_java_util_zip_Deflater_setDictionaryBuffer;
Java_java_util_zip_Inflater_end;
Java_java_util_zip_Inflater_getAdler;
-   Java_java_util_zip_Inflater_inflateBytes;
+   Java_java_util_zip_Inflater_inflateBytesBytes;
+   Java_java_util_zip_Inflater_inflateBytesBuffer;
+   Java_java_util_zip_Inflater_inflateBufferBytes;
+   Java_java_util_zip_Inflater_inflateBufferBuffer;
Java_java_util_zip_Inflater_init;
Java_java_util_zip_Inflater_initIDs;
Java_java_util_zip_Inflater_reset;
Java_java_util_zip_Inflater_setDictionary;
+   Java_java_util_zip_Inflater_setDictionaryBuffer;
ZIP_Close;
ZIP_CRC32;
ZIP_FreeEntry;
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java 
b/src/java.base/share/classes/java/util/zip/Deflater.java
index c75dd4a33f0..a1720a78986 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -26,7 +26,13 @@
 package java.util.zip;
 
 import java.lang.ref.Cleaner.Cleanable;
+import java.lang.ref.Reference;
+import java.nio.ByteBuffer;
+import java.nio.ReadOnlyBufferException;
+import java.util.Objects;
+
 import jdk.internal.ref.CleanerFactory;
+import sun.nio.ch.DirectBuffer;
 
 /**
  * This class provides support for general purpose compression using the
@@ -92,8 +98,9 @@ import jdk.internal.ref.CleanerFactory;
 public class Deflater {
 
 private final DeflaterZStreamRef zsRef;
-private byte[] buf = new byte[0];
-private int off, len;
+private ByteBuffer input = ZipUtils.defaultBuf;
+private byte[] inputArray;
+private int inputPos, inputLim;
 private int level, strategy;
 private boolean setParams;
 private boolean finish, finished;
@@ -170,9 +177,14 @@ public class Deflater {
  */
 public static final int FULL_FLUSH = 3;
 
+/**
+ * Flush mode to use at the end of output.  Can only be provided by the
+ * user by way of {@link #finish()}.
+ */
+private static final int FINISH = 4;
+
 static {
 ZipUtils.loadLibrary();
-initIDs();
 }
 
 /**
@@ -216,16 +228,14 @@ public class Deflater {
  * @see Deflater#needsInput
  */
 public void setInput(byte[] b, int off, int len) {
-if (b== null) {
-throw new NullPointerException();
-}
 if (off < 0 || len < 0 || off > b.length - len) {
 throw new ArrayIndexOutOfBoundsException();
 }
 synchronized (zsRef) {
-this.buf = b;
-this.off = off;
-this.len = len;
+this.input = null;
+this.inputArray = b;
+this.inputPos = off;
+this.inputLim = off + len;
 }
 }
 
@@ -239,6 +249,31 @@ public class Deflater

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

[David Lloyd]

Are there any further comments on this?  If not, would it be possible
to get a sponsor for this change?

Sorry again for the detached email threads; I've learned my GMail lesson well...

Thanks.

On Fri, Mar 16, 2018 at 8:25 AM, David Lloyd  wrote:
> Sorry, that was an error on my part, caused by too much context
> switching.  I've posted an update at
> https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v13 which is
> also attached.
>
> On Wed, Mar 14, 2018 at 8:53 PM, Xueming Shen  wrote:
>> Hi David,
>>
>> https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v12
>>
>> Should we start to review the changes included in above link, or we should
>> wait ? It appears
>> the API is being updated but some implementation have not been updated to
>> follow the spec
>> yet, especially the piece that deals with the output buffer/byteWritten when
>> DataFormatException
>> is raised, for example
>>
>> (1) the "outputConsumedID" is defined but never used to update the
>> corresponding java field
>>  in Inflater.c and
>>
>> (2) the "outputConsumed" is used to update the output ByteBuffer when DFE
>> raised (in Java), but
>>  the corresponding "byteWritten" is not being updated before the
>> exception is thrown.
>>
>> -Sherman
>>
>>
>>
>>
>> On 3/13/18, 10:46 AM, David Lloyd wrote:
>>>
>>> Sorry all, it looks like GMail doesn't know how to keep replies with
>>> the thread when you change the subject line.  The follow-up to this
>>> thread is
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-March/051960.html
>>> with only a few small changes as discussed above.
>>>
>>> On Fri, Mar 2, 2018 at 2:36 PM, David Lloyd
>>> wrote:
>>>>
>>>> On Fri, Mar 2, 2018 at 2:34 PM, David Lloyd
>>>> wrote:
>>>>>
>>>>> On Fri, Mar 2, 2018 at 12:49 PM, Xueming Shen
>>>>> wrote:
>>>>>>
>>>>>> Hi David,
>>>>>>
>>>>>> (1) Deflater.deflate(Bytebuffer)
>>>>>>   the api doc regarding "no_flush" appears to be the copy/paste of
>>>>>> the
>>>>>> byte[] version
>>>>>>   without being updated to the corresponding ByteBuffer?
>>>>>
>>>>> You're right, I missed that one.  I've incorporated this fix locally:
>>>>
>>>> Oops, this should have been:
>>>>
>>>> --- 8<  --- cut here --- 8<  ---
>>>>
>>>> diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
>>>> b/src/java.base/share/classes/java/util/zip/Deflater.java
>>>> index 524125787a8..40f0d9736e2 100644
>>>> --- a/src/java.base/share/classes/java/util/zip/Deflater.java
>>>> +++ b/src/java.base/share/classes/java/util/zip/Deflater.java
>>>> @@ -481,9 +481,9 @@ public class Deflater {
>>>>* in order to determine if more input data is required.
>>>>*
>>>>*This method uses {@link #NO_FLUSH} as its compression flush
>>>> mode.
>>>> - * An invocation of this method of the form {@code
>>>> deflater.deflate(b)}
>>>> + * An invocation of this method of the form {@code
>>>> deflater.deflate(output)}
>>>>* yields the same result as the invocation of
>>>> - * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
>>>> + * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
>>>>*
>>>>* @param output the buffer for the compressed data
>>>>* @return the actual number of bytes of compressed data written to
>>>> the
>>>>
>>>> --- 8<  --- cut here --- 8<  ---
>>>>
>>>> --
>>>> - DML
>>>
>>>
>>>
>>
>
>
>
> --
> - DML



-- 
- DML

Re: [11] [PATCH] 9053056: Anchor name conflict in ClassLoader JavaDoc

[David Lloyd]

On Wed, Mar 21, 2018 at 3:39 PM, mandy chung  wrote:
> On 3/21/18 10:26 AM, David Lloyd wrote:
>
> I see it with IntelliJ IDEA when I pop up "quick JavaDoc" on, say,
> defineClass and then click on "binary name"; it's actually using the
> sources, rather than generated JavaDoc.
>
>
> I don't object to do this renaming as I favor this more explicit id.
> However you will hit the same issue if another class names an anchor
> conflicts with its private member.   Can you configure IntelliJ to generate
> public/protected elements to avoid this issue?

My impression is that it is generating its indexes and rendering on
the fly (or at least in some opaque manner) directly from the sources.
There does not seem to be any setting which governs what is produced
or displayed; I think it is generally a feature (not a bug) that one
can browse into dependency and JDK sources and get quick JavaDoc on
any element, even non-public elements.

-- 
- DML

Re: RFR of 8180451: ByteArrayInputStream should override readAllBytes, readNBytes, and transferTo

[David Lloyd]

Sorry.  This looks OK to me (non-reviewer) now.

On Wed, Mar 21, 2018 at 1:01 PM, Brian Burkhalter
 wrote:
> This is still in need of final approval, assuming it is OK.
>
> Thanks,
>
> Brian
>
> On Mar 14, 2018, at 10:50 AM, Brian Burkhalter 
> wrote:
>
> On Mar 14, 2018, at 9:27 AM, David Lloyd  wrote:
>
> @@ -196,14 +194,32 @@
> return len;
> }
>
> +public synchronized byte[] readAllBytes() {
> +byte[] result = Arrays.copyOfRange(buf, pos, count);
> +pos = count;
> +return result;
> +}
> +
> +public synchronized int readNBytes(byte[] b, int off, int len) {
> +int n = read(b, off, len);
> +return n == -1 ? 0 : n;
> +}
>
> This probably doesn't need to be synchronized, though I imagine the
> difference would be minimal.
>
>
> You are correct, it does not.
>
> +public synchronized long transferTo(OutputStream out) throws
> IOException {
> +int len = count - pos
> +out.write(but, pos, len);
>
> s/but/buf/ I guess?
>
>
> Webrevs corrected in place:
>
> http://cr.openjdk.java.net/~bpb/8180451/webrev.00-01/
> http://cr.openjdk.java.net/~bpb/8180451/webrev.01/
>
>



-- 
- DML

Re: [11] [PATCH] 9053056: Anchor name conflict in ClassLoader JavaDoc

[David Lloyd]

On Wed, Mar 21, 2018 at 12:04 PM, mandy chung  wrote:
>
>
> On 3/21/18 8:58 AM, David Lloyd wrote:
>
> Since adding a field called "name" to java.lang.ClassLoader, the
> "name" anchor which previously referred to the section entitled
> "binary names" has been broken.
>
> The attached doc-only patch changes the name of the anchor to
> "binary-name".  It applies with "patch -p1".
>
> The links are not broken in the published javadoc as it only generates
> javadoc for the public elements.
>
> Are you seeing the broken links from your own javadoc build including
> private elements?

I see it with IntelliJ IDEA when I pop up "quick JavaDoc" on, say,
defineClass and then click on "binary name"; it's actually using the
sources, rather than generated JavaDoc.

-- 
- DML

Re: [11] [PATCH] 9053056: Anchor name conflict in ClassLoader JavaDoc

[David Lloyd]

Ah, so the submitted bug ID doesn't match the final bug ID.  Excellent.

On Wed, Mar 21, 2018 at 11:24 AM, Martin Buchholz  wrote:
> Found it.
> https://bugs.openjdk.java.net/browse/JDK-8199947
>



-- 
- DML

Re: [11] [PATCH] 9053056: Anchor name conflict in ClassLoader JavaDoc

[David Lloyd]

On Wed, Mar 21, 2018 at 11:10 AM, Martin Buchholz  wrote:
> Hi David.
>
>  I'll take this one.

Thanks.

> - * Returns a {@code Package} of the given name that
> + * Returns a {@code Package} of the given  href="#binary-name">name that
>
> Do Packages (I should really learn about Packages) actually have binary
> names?

Maybe but AFAICT they would be identical to the Java language name.
Rather than try to solve this distinction, in the spirit of solving
one bug at a time (and also laziness), I opted to just replace all
existing references and otherwise change nothing.

> I'm confused by the subject.  There is no such bug

I submitted it via bugreport.java.com; it hasn't been reviewed yet.  I
expect it might show up soon.

> On Wed, Mar 21, 2018 at 9:03 AM David Lloyd  wrote:
>>
>> I discovered there are a couple of references outside of ClassLoader
>> as well.  Here's the updated patch...
>>
>> On Wed, Mar 21, 2018 at 10:58 AM, David Lloyd 
>> wrote:
>> > Since adding a field called "name" to java.lang.ClassLoader, the
>> > "name" anchor which previously referred to the section entitled
>> > "binary names" has been broken.
>> >
>> > The attached doc-only patch changes the name of the anchor to
>> > "binary-name".  It applies with "patch -p1".
>> >
>> > --
>> > - DML
>>
>>
>>
>> --
>> - DML



-- 
- DML

Re: [11] [PATCH] 9053056: Anchor name conflict in ClassLoader JavaDoc

[David Lloyd]

I discovered there are a couple of references outside of ClassLoader
as well.  Here's the updated patch...

On Wed, Mar 21, 2018 at 10:58 AM, David Lloyd  wrote:
> Since adding a field called "name" to java.lang.ClassLoader, the
> "name" anchor which previously referred to the section entitled
> "binary names" has been broken.
>
> The attached doc-only patch changes the name of the anchor to
> "binary-name".  It applies with "patch -p1".
>
> --
> - DML



-- 
- DML
diff --git a/src/java.base/share/classes/java/lang/Class.java 
b/src/java.base/share/classes/java/lang/Class.java
index cf34ff2d86e..adb61e69d35 100644
--- a/src/java.base/share/classes/java/lang/Class.java
+++ b/src/java.base/share/classes/java/lang/Class.java
@@ -382,7 +382,7 @@ public final class Class implements java.io.Serializable,
 
 
 /**
- * Returns the {@code Class} with the given 
+ * Returns the {@code Class} with the given 
  * binary name in the given module.
  *
  *  This method attempts to locate, load, and link the class or 
interface.
@@ -404,7 +404,7 @@ public final class Class implements java.io.Serializable,
  * loads a class in another module.
  *
  * @param  module   A module
- * @param  name The binary name
+ * @param  name The binary 
name
  *  of the class
  * @return {@code Class} object of the given name defined in the given 
module;
  * {@code null} if not found.
diff --git a/src/java.base/share/classes/java/lang/ClassLoader.java 
b/src/java.base/share/classes/java/lang/ClassLoader.java
index f0d41f425dd..cb6193f2b1f 100644
--- a/src/java.base/share/classes/java/lang/ClassLoader.java
+++ b/src/java.base/share/classes/java/lang/ClassLoader.java
@@ -73,7 +73,7 @@ import sun.security.util.SecurityConstants;
 /**
  * A class loader is an object that is responsible for loading classes. The
  * class {@code ClassLoader} is an abstract class.  Given the binary name of a class, a class loader should attempt to
+ * href="#binary-name">binary name of a class, a class loader should 
attempt to
  * locate or generate data that constitutes a definition for the class.  A
  * typical strategy is to transform the name into a file name and then read a
  * "class file" of that name from a file system.
@@ -202,7 +202,7 @@ import sun.security.util.SecurityConstants;
  * }
  * 
  *
- *  Binary names 
+ *  Binary names 
  *
  *  Any class name provided as a {@code String} parameter to methods in
  * {@code ClassLoader} must be a binary name as defined by
@@ -480,7 +480,7 @@ public abstract class ClassLoader {
 // -- Class --
 
 /**
- * Loads the class with the specified binary name.
+ * Loads the class with the specified binary 
name.
  * This method searches for classes in the same manner as the {@link
  * #loadClass(String, boolean)} method.  It is invoked by the Java virtual
  * machine to resolve class references.  Invoking this method is equivalent
@@ -488,7 +488,7 @@ public abstract class ClassLoader {
  * false)}.
  *
  * @param  name
- * The binary name of the class
+ * The binary name of the class
  *
  * @return  The resulting {@code Class} object
  *
@@ -500,7 +500,7 @@ public abstract class ClassLoader {
 }
 
 /**
- * Loads the class with the specified binary name.  The
+ * Loads the class with the specified binary 
name.  The
  * default implementation of this method searches for classes in the
  * following order:
  *
@@ -530,7 +530,7 @@ public abstract class ClassLoader {
  * during the entire class loading process.
  *
  * @param  name
- * The binary name of the class
+ * The binary name of the class
  *
  * @param  resolve
  * If {@code true} then resolve the class
@@ -579,7 +579,7 @@ public abstract class ClassLoader {
 }
 
 /**
- * Loads the class with the specified binary name
+ * Loads the class with the specified binary 
name
  * in a module defined to this class loader.  This method returns {@code 
null}
  * if the class could not be found.
  *
@@ -598,7 +598,7 @@ public abstract class ClassLoader {
  * @param  module
  * The module
  * @param  name
- * The binary name of the class
+ * The binary name of the class
  *
  * @return The resulting {@code Class} object in a module defined by
  * this class loader, or {@code null} if the class could not be 
found.
@@ -674,7 +674,7 @@ public abstract class ClassLoader {
 }
 
 /**
- * Finds the class with the specified binary name.
+ * Finds the class with the specified binary 
name.
  * This method should be overridden by class loader implementations that
  * follow the delegation model for loading cla

[11] [PATCH] 9053056: Anchor name conflict in ClassLoader JavaDoc

[David Lloyd]

Since adding a field called "name" to java.lang.ClassLoader, the
"name" anchor which previously referred to the section entitled
"binary names" has been broken.

The attached doc-only patch changes the name of the anchor to
"binary-name".  It applies with "patch -p1".

-- 
- DML
diff --git a/src/java.base/share/classes/java/lang/ClassLoader.java 
b/src/java.base/share/classes/java/lang/ClassLoader.java
index f0d41f425dd..cb6193f2b1f 100644
--- a/src/java.base/share/classes/java/lang/ClassLoader.java
+++ b/src/java.base/share/classes/java/lang/ClassLoader.java
@@ -73,7 +73,7 @@ import sun.security.util.SecurityConstants;
 /**
  * A class loader is an object that is responsible for loading classes. The
  * class {@code ClassLoader} is an abstract class.  Given the binary name of a class, a class loader should attempt to
+ * href="#binary-name">binary name of a class, a class loader should 
attempt to
  * locate or generate data that constitutes a definition for the class.  A
  * typical strategy is to transform the name into a file name and then read a
  * "class file" of that name from a file system.
@@ -202,7 +202,7 @@ import sun.security.util.SecurityConstants;
  * }
  * 
  *
- *  Binary names 
+ *  Binary names 
  *
  *  Any class name provided as a {@code String} parameter to methods in
  * {@code ClassLoader} must be a binary name as defined by
@@ -480,7 +480,7 @@ public abstract class ClassLoader {
 // -- Class --
 
 /**
- * Loads the class with the specified binary name.
+ * Loads the class with the specified binary 
name.
  * This method searches for classes in the same manner as the {@link
  * #loadClass(String, boolean)} method.  It is invoked by the Java virtual
  * machine to resolve class references.  Invoking this method is equivalent
@@ -488,7 +488,7 @@ public abstract class ClassLoader {
  * false)}.
  *
  * @param  name
- * The binary name of the class
+ * The binary name of the class
  *
  * @return  The resulting {@code Class} object
  *
@@ -500,7 +500,7 @@ public abstract class ClassLoader {
 }
 
 /**
- * Loads the class with the specified binary name.  The
+ * Loads the class with the specified binary 
name.  The
  * default implementation of this method searches for classes in the
  * following order:
  *
@@ -530,7 +530,7 @@ public abstract class ClassLoader {
  * during the entire class loading process.
  *
  * @param  name
- * The binary name of the class
+ * The binary name of the class
  *
  * @param  resolve
  * If {@code true} then resolve the class
@@ -579,7 +579,7 @@ public abstract class ClassLoader {
 }
 
 /**
- * Loads the class with the specified binary name
+ * Loads the class with the specified binary 
name
  * in a module defined to this class loader.  This method returns {@code 
null}
  * if the class could not be found.
  *
@@ -598,7 +598,7 @@ public abstract class ClassLoader {
  * @param  module
  * The module
  * @param  name
- * The binary name of the class
+ * The binary name of the class
  *
  * @return The resulting {@code Class} object in a module defined by
  * this class loader, or {@code null} if the class could not be 
found.
@@ -674,7 +674,7 @@ public abstract class ClassLoader {
 }
 
 /**
- * Finds the class with the specified binary name.
+ * Finds the class with the specified binary 
name.
  * This method should be overridden by class loader implementations that
  * follow the delegation model for loading classes, and will be invoked by
  * the {@link #loadClass loadClass} method after checking the
@@ -683,7 +683,7 @@ public abstract class ClassLoader {
  * @implSpec The default implementation throws {@code 
ClassNotFoundException}.
  *
  * @param  name
- * The binary name of the class
+ * The binary name of the class
  *
  * @return  The resulting {@code Class} object
  *
@@ -697,7 +697,7 @@ public abstract class ClassLoader {
 }
 
 /**
- * Finds the class with the given binary name
+ * Finds the class with the given binary name
  * in a module defined to this class loader.
  * Class loader implementations that support the loading from modules
  * should override this method.
@@ -715,7 +715,7 @@ public abstract class ClassLoader {
  * class loader
 
  * @param  name
- * The binary name of the class
+ * The binary name of the class
  *
  * @return The resulting {@code Class} object, or {@code null}
  * if the class could not be found.
@@ -737,7 +737,7 @@ public abstract class ClassLoader {
  * Converts an array of bytes into an instance of class {@code Class}.
  * Before the {@code Class} can be used it must be resolved.  This method

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

[David Lloyd]

Sorry, that was an error on my part, caused by too much context
switching.  I've posted an update at
https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v13 which is
also attached.

On Wed, Mar 14, 2018 at 8:53 PM, Xueming Shen  wrote:
> Hi David,
>
> https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v12
>
> Should we start to review the changes included in above link, or we should
> wait ? It appears
> the API is being updated but some implementation have not been updated to
> follow the spec
> yet, especially the piece that deals with the output buffer/byteWritten when
> DataFormatException
> is raised, for example
>
> (1) the "outputConsumedID" is defined but never used to update the
> corresponding java field
>  in Inflater.c and
>
> (2) the "outputConsumed" is used to update the output ByteBuffer when DFE
> raised (in Java), but
>  the corresponding "byteWritten" is not being updated before the
> exception is thrown.
>
> -Sherman
>
>
>
>
> On 3/13/18, 10:46 AM, David Lloyd wrote:
>>
>> Sorry all, it looks like GMail doesn't know how to keep replies with
>> the thread when you change the subject line.  The follow-up to this
>> thread is
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-March/051960.html
>> with only a few small changes as discussed above.
>>
>> On Fri, Mar 2, 2018 at 2:36 PM, David Lloyd
>> wrote:
>>>
>>> On Fri, Mar 2, 2018 at 2:34 PM, David Lloyd
>>> wrote:
>>>>
>>>> On Fri, Mar 2, 2018 at 12:49 PM, Xueming Shen
>>>> wrote:
>>>>>
>>>>> Hi David,
>>>>>
>>>>> (1) Deflater.deflate(Bytebuffer)
>>>>>   the api doc regarding "no_flush" appears to be the copy/paste of
>>>>> the
>>>>> byte[] version
>>>>>   without being updated to the corresponding ByteBuffer?
>>>>
>>>> You're right, I missed that one.  I've incorporated this fix locally:
>>>
>>> Oops, this should have been:
>>>
>>> --- 8<  --- cut here --- 8<  ---
>>>
>>> diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
>>> b/src/java.base/share/classes/java/util/zip/Deflater.java
>>> index 524125787a8..40f0d9736e2 100644
>>> --- a/src/java.base/share/classes/java/util/zip/Deflater.java
>>> +++ b/src/java.base/share/classes/java/util/zip/Deflater.java
>>> @@ -481,9 +481,9 @@ public class Deflater {
>>>* in order to determine if more input data is required.
>>>*
>>>*This method uses {@link #NO_FLUSH} as its compression flush
>>> mode.
>>> - * An invocation of this method of the form {@code
>>> deflater.deflate(b)}
>>> + * An invocation of this method of the form {@code
>>> deflater.deflate(output)}
>>>* yields the same result as the invocation of
>>> - * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
>>> + * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
>>>*
>>>* @param output the buffer for the compressed data
>>>* @return the actual number of bytes of compressed data written to
>>> the
>>>
>>> --- 8<  --- cut here --- 8<  ---
>>>
>>> --
>>> - DML
>>
>>
>>
>



-- 
- DML
commit ee5f766eeb64d5afa1f3ec1153b3184de35ff198
Author: David M. Lloyd 
Date:   Fri Feb 16 11:00:10 2018 -0600

[JDK-6341887] Update Inflater/Deflater to handle ByteBuffer

diff --git a/make/mapfiles/libzip/mapfile-vers 
b/make/mapfiles/libzip/mapfile-vers
index d711d8e17f4..11ccc2d6ecb 100644
--- a/make/mapfiles/libzip/mapfile-vers
+++ b/make/mapfiles/libzip/mapfile-vers
@@ -33,20 +33,28 @@ SUNWprivate_1.1 {
Java_java_util_zip_CRC32_update;
Java_java_util_zip_CRC32_updateBytes0;
Java_java_util_zip_CRC32_updateByteBuffer0;
-   Java_java_util_zip_Deflater_deflateBytes;
+   Java_java_util_zip_Deflater_deflateBytesBytes;
+   Java_java_util_zip_Deflater_deflateBytesBuffer;
+   Java_java_util_zip_Deflater_deflateBufferBytes;
+   Java_java_util_zip_Deflater_deflateBufferBuffer;
Java_java_util_zip_Deflater_end;
Java_java_util_zip_Deflater_getAdler;
Java_java_util_zip_Deflater_init;
Java_java_util_zip_Deflater_initIDs;
Java_java_util_zip_Deflater_reset;
Java_java_util_zip_Def

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

[David Lloyd]

On Wed, Mar 14, 2018 at 4:31 PM, Aleksey Shipilev  wrote:
> On 03/14/2018 07:09 PM, David Lloyd wrote:
>> On Wed, Mar 14, 2018 at 1:00 PM, mandy chung  wrote:
>>> Thanks for adding the new test.   Looks okay and some minor comment.
>>>
>>> +try {
>>>:
>>> +} catch (Throwable e) {
>>> +System.err.println("\nTEST FAILED:");
>>> +e.printStackTrace();
>>> +throw new RuntimeException("TEST FAILED: " + e.toString());
>>> +}
>>>
>>>
>>> You can take out this try-catch (Basic1.java isn't be the best example to
>>> reference that is old and needs update).
>>
>> Fixed & attached.
>
> Have you tried to run the test?
>
> Because it fails:
>
> $ make images run-test TEST=jdk/java/lang/reflect/Proxy/ProxyClashTest.java
>
> Dynamic proxy API static method clash test
>
> java.lang.IllegalArgumentException: ProxyClashTest$ClashWithRunnable 
> referenced from a method is not
> visible from class loader
> at 
> java.base/java.lang.reflect.Proxy$ProxyBuilder.ensureVisible(Proxy.java:851)
> at 
> java.base/java.lang.reflect.Proxy$ProxyBuilder.validateProxyInterfaces(Proxy.java:682)
> at 
> java.base/java.lang.reflect.Proxy$ProxyBuilder.(Proxy.java:628)
> at 
> java.base/java.lang.reflect.Proxy.lambda$getProxyConstructor$1(Proxy.java:426)
> at
> java.base/jdk.internal.loader.AbstractClassLoaderValue$Memoizer.get(AbstractClassLoaderValue.java:329)
> at
> java.base/jdk.internal.loader.AbstractClassLoaderValue.computeIfAbsent(AbstractClassLoaderValue.java:205)
> at 
> java.base/java.lang.reflect.Proxy.getProxyConstructor(Proxy.java:424)
> at java.base/java.lang.reflect.Proxy.getProxyClass(Proxy.java:384)
> at ProxyClashTest.main(ProxyClashTest.java:56)
> at 
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at 
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:564)
> at 
> com.sun.javatest.regtest.agent.MainActionHelper$SameVMRunnable.run(MainActionHelper.java:229)
> at java.base/java.lang.Thread.run(Thread.java:841)
>
> JavaTest Message: Test threw exception: java.lang.IllegalArgumentException
> JavaTest Message: shutting down test

I did not run it with jtreg, just on the class path after compiling by
hand.  I guess it is failing because the nested class is not visible
from the system class loader when running within JTReg; maybe the fix
would be to use the class loader of the test class or the nested
class?

-- 
- DML

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

[David Lloyd]

On Wed, Mar 14, 2018 at 1:00 PM, mandy chung  wrote:
> Thanks for adding the new test.   Looks okay and some minor comment.
>
> +try {
>:
> +} catch (Throwable e) {
> +System.err.println("\nTEST FAILED:");
> +e.printStackTrace();
> +throw new RuntimeException("TEST FAILED: " + e.toString());
> +}
>
>
> You can take out this try-catch (Basic1.java isn't be the best example to
> reference that is old and needs update).

Fixed & attached.

> I assume your colleague at Red Hat will sponsor it for you?

I will find out.

-- 
- DML
diff --git a/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java 
b/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
index cc8dbbfffab..465a5b938e3 100644
--- a/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
+++ b/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
@@ -449,7 +449,9 @@ class ProxyGenerator {
  */
 for (Class intf : interfaces) {
 for (Method m : intf.getMethods()) {
-addProxyMethod(m, intf);
+if (! Modifier.isStatic(m.getModifiers())) {
+addProxyMethod(m, intf);
+}
 }
 }
 
diff --git a/test/jdk/java/lang/reflect/Proxy/ProxyClashTest.java 
b/test/jdk/java/lang/reflect/Proxy/ProxyClashTest.java
new file mode 100644
index 000..604ddf6a930
--- /dev/null
+++ b/test/jdk/java/lang/reflect/Proxy/ProxyClashTest.java
@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/* @test
+ * @bug 8188240
+ * @summary This is a test to ensure that proxies do not inherit static 
methods.
+ *
+ * @build ProxyClashTest
+ * @run main ProxyClashTest
+ */
+
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+import java.util.Observer;
+
+public class ProxyClashTest {
+
+public interface ClashWithRunnable {
+static int run() { return 123; }
+
+static void foo() {}
+}
+
+public static void main(String[] args) {
+System.err.println(
+"\nDynamic proxy API static method clash test\n");
+
+Class[] interfaces =
+new Class[] { ClashWithRunnable.class, Runnable.class, 
Observer.class };
+
+ClassLoader loader = ClassLoader.getSystemClassLoader();
+
+/*
+ * Generate a proxy class.
+ */
+Class proxyClass = Proxy.getProxyClass(loader, interfaces);
+System.err.println("+ generated proxy class: " + proxyClass);
+
+for (Method method : proxyClass.getDeclaredMethods()) {
+if (method.getName().equals("run") && method.getReturnType() == 
int.class) {
+throw new RuntimeException("proxy inherited a static method");
+}
+if (method.getName().equals("foo")) {
+throw new RuntimeException("proxy inherited a static method");
+}
+}
+
+System.err.println("\nTEST PASSED");
+}
+}

Re: RFR of 8180451: ByteArrayInputStream should override readAllBytes, readNBytes, and transferTo

[David Lloyd]

In:

@@ -196,14 +194,32 @@
 return len;
 }

+public synchronized byte[] readAllBytes() {
+byte[] result = Arrays.copyOfRange(buf, pos, count);
+pos = count;
+return result;
+}
+
+public synchronized int readNBytes(byte[] b, int off, int len) {
+int n = read(b, off, len);
+return n == -1 ? 0 : n;
+}

This probably doesn't need to be synchronized, though I imagine the
difference would be minimal.

+public synchronized long transferTo(OutputStream out) throws IOException {
+int len = count - pos
+out.write(but, pos, len);

s/but/buf/ I guess?

+pos = count;
+return len;
+}
+

On Wed, Mar 14, 2018 at 10:31 AM, Brian Burkhalter
 wrote:
> Reprising this thread from three months ago [1].
>
> A patch including the changes suggested below is at
>
> http://cr.openjdk.java.net/~bpb/8180451/webrev.01/
>
> with the differences between this and the prior version at
>
> http://cr.openjdk.java.net/~bpb/8180451/webrev.00-01/
>
> Thanks,
>
> Brian
>
> [1] 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050550.html
>
> On Dec 12, 2017, at 11:42 AM, Paul Sandoz  wrote:
>
>> 208 public synchronized long transferTo(OutputStream out) throws 
>> IOException {
>> 209 int pos0 = pos;
>> 210 out.write(buf, pos, count - pos);
>> 211 pos = count;
>> 212 return count - pos0;
>> 213 }
>>
>>  int len = count - pos
>>  out.write(but, pos, len);
>>  pos = count;
>>  return len;
>
> On Dec 12, 2017, at 12:23 PM, Brent Christian  
> wrote:
>
>> The changes look fine to me.
>> I would have found the test case a little easier to follow if "off"/"len" 
>> weren't named so similarly to "offset"/"length", but it's not a big deal.
>
>



-- 
- DML

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

[David Lloyd]

On Tue, Mar 13, 2018 at 7:16 PM, David Lloyd  wrote:
> On Tue, Mar 13, 2018 at 6:31 PM, mandy chung  wrote:
>> I prefer to keep the original test case i.e. create a proxy class from
>> Runnable and Observer.   Instead add a new test case to create a proxy class
>> with ClashWithRunnable, Runnable and Observer and verify that it does not
>> include static methods.  I would add another static method
>> ClashWithRunnable::foo (no name clash) and verify
>> proxyClass::getDeclaredMethods does not contain foo.
>
> OK, done.  It's a little bigger now so I'm attaching it.

Apologies, my IDE put it in the wrong directory.  Here's a better one.
I ran it by hand as jtreg and I are not presently on speaking terms.
-- 
- DML
diff --git a/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java 
b/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
index cc8dbbfffab..465a5b938e3 100644
--- a/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
+++ b/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
@@ -449,7 +449,9 @@ class ProxyGenerator {
  */
 for (Class intf : interfaces) {
 for (Method m : intf.getMethods()) {
-addProxyMethod(m, intf);
+if (! Modifier.isStatic(m.getModifiers())) {
+addProxyMethod(m, intf);
+}
 }
 }
 
diff --git a/test/jdk/java/lang/reflect/Proxy/ProxyClashTest.java 
b/test/jdk/java/lang/reflect/Proxy/ProxyClashTest.java
new file mode 100644
index 000..5ad752880b3
--- /dev/null
+++ b/test/jdk/java/lang/reflect/Proxy/ProxyClashTest.java
@@ -0,0 +1,78 @@
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/* @test
+ * @bug 8188240
+ * @summary This is a test to ensure that proxies do not inherit static 
methods.
+ *
+ * @build ProxyClashTest
+ * @run main ProxyClashTest
+ */
+
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+import java.util.Observer;
+
+public class ProxyClashTest {
+
+public interface ClashWithRunnable {
+static int run() { return 123; }
+
+static void foo() {}
+}
+
+public static void main(String[] args) {
+
+System.err.println(
+"\nDynamic proxy API static method clash test\n");
+
+try {
+Class[] interfaces =
+new Class[] { ClashWithRunnable.class, Runnable.class, 
Observer.class };
+
+ClassLoader loader = ClassLoader.getSystemClassLoader();
+
+/*
+ * Generate a proxy class.
+ */
+Class proxyClass = Proxy.getProxyClass(loader, interfaces);
+System.err.println("+ generated proxy class: " + proxyClass);
+
+for (Method method : proxyClass.getDeclaredMethods()) {
+if (method.getName().equals("run") && method.getReturnType() 
== int.class) {
+throw new RuntimeException("proxy inherited a static 
method");
+}
+if (method.getName().equals("foo")) {
+throw new RuntimeException("proxy inherited a static 
method");
+}
+}
+
+System.err.println("\nTEST PASSED");
+
+} catch (Throwable e) {
+System.err.println("\nTEST FAILED:");
+e.printStackTrace();
+throw new RuntimeException("TEST FAILED: " + e.toString());
+}
+}
+}

Re: [PATCH] 8188240: Reflection Proxy should skip static methods

[David Lloyd]

On Tue, Mar 13, 2018 at 6:31 PM, mandy chung  wrote:
> I prefer to keep the original test case i.e. create a proxy class from
> Runnable and Observer.   Instead add a new test case to create a proxy class
> with ClashWithRunnable, Runnable and Observer and verify that it does not
> include static methods.  I would add another static method
> ClashWithRunnable::foo (no name clash) and verify
> proxyClass::getDeclaredMethods does not contain foo.

OK, done.  It's a little bigger now so I'm attaching it.


-- 
- DML
diff --git a/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java 
b/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
index cc8dbbfffab..465a5b938e3 100644
--- a/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
+++ b/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
@@ -449,7 +449,9 @@ class ProxyGenerator {
  */
 for (Class intf : interfaces) {
 for (Method m : intf.getMethods()) {
-addProxyMethod(m, intf);
+if (! Modifier.isStatic(m.getModifiers())) {
+addProxyMethod(m, intf);
+}
 }
 }
 
diff --git a/test/jdk/java/lang/reflect/Proxy/src/test/ProxyClashTest.java 
b/test/jdk/java/lang/reflect/Proxy/src/test/ProxyClashTest.java
new file mode 100644
index 000..5ad752880b3
--- /dev/null
+++ b/test/jdk/java/lang/reflect/Proxy/src/test/ProxyClashTest.java
@@ -0,0 +1,78 @@
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/* @test
+ * @bug 8188240
+ * @summary This is a test to ensure that proxies do not inherit static 
methods.
+ *
+ * @build ProxyClashTest
+ * @run main ProxyClashTest
+ */
+
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+import java.util.Observer;
+
+public class ProxyClashTest {
+
+public interface ClashWithRunnable {
+static int run() { return 123; }
+
+static void foo() {}
+}
+
+public static void main(String[] args) {
+
+System.err.println(
+"\nDynamic proxy API static method clash test\n");
+
+try {
+Class[] interfaces =
+new Class[] { ClashWithRunnable.class, Runnable.class, 
Observer.class };
+
+ClassLoader loader = ClassLoader.getSystemClassLoader();
+
+/*
+ * Generate a proxy class.
+ */
+Class proxyClass = Proxy.getProxyClass(loader, interfaces);
+System.err.println("+ generated proxy class: " + proxyClass);
+
+for (Method method : proxyClass.getDeclaredMethods()) {
+if (method.getName().equals("run") && method.getReturnType() 
== int.class) {
+throw new RuntimeException("proxy inherited a static 
method");
+}
+if (method.getName().equals("foo")) {
+throw new RuntimeException("proxy inherited a static 
method");
+}
+}
+
+System.err.println("\nTEST PASSED");
+
+} catch (Throwable e) {
+System.err.println("\nTEST FAILED:");
+e.printStackTrace();
+throw new RuntimeException("TEST FAILED: " + e.toString());
+}
+}
+}

[PATCH] 8188240: Reflection Proxy should skip static methods

[David Lloyd]

I worked up a little patch for 8188240.  I was able to co-opt an
existing test which now fails before the patch and passes after.  It's
a tiny patch so I'm including it inline.  I've CC'd Mandy because she
filed the original bug.

Here's the patch (use patch -p1 to apply):

 cut --- 8< --- cut 
diff --git a/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
b/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
index cc8dbbfffab..465a5b938e3 100644
--- a/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
+++ b/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
@@ -449,7 +449,9 @@ class ProxyGenerator {
  */
 for (Class intf : interfaces) {
 for (Method m : intf.getMethods()) {
-addProxyMethod(m, intf);
+if (! Modifier.isStatic(m.getModifiers())) {
+addProxyMethod(m, intf);
+}
 }
 }

diff --git a/test/jdk/java/lang/reflect/Proxy/Basic1.java
b/test/jdk/java/lang/reflect/Proxy/Basic1.java
index b2440ccd9f9..4a21470b1be 100644
--- a/test/jdk/java/lang/reflect/Proxy/Basic1.java
+++ b/test/jdk/java/lang/reflect/Proxy/Basic1.java
@@ -36,6 +36,10 @@ import java.util.*;

 public class Basic1 {

+public interface ClashWithRunnable {
+static int run() { return 123; }
+}
+
 public static void main(String[] args) {

 System.err.println(
@@ -43,7 +47,7 @@ public class Basic1 {

 try {
 Class[] interfaces =
-new Class[] { Runnable.class, Observer.class };
+new Class[] { ClashWithRunnable.class,
Runnable.class, Observer.class };

 ClassLoader loader = ClassLoader.getSystemClassLoader();

 cut --- 8< --- cut 

-- 
- DML

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

[David Lloyd]

Sorry all, it looks like GMail doesn't know how to keep replies with
the thread when you change the subject line.  The follow-up to this
thread is 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-March/051960.html
with only a few small changes as discussed above.

On Fri, Mar 2, 2018 at 2:36 PM, David Lloyd  wrote:
> On Fri, Mar 2, 2018 at 2:34 PM, David Lloyd  wrote:
>> On Fri, Mar 2, 2018 at 12:49 PM, Xueming Shen  
>> wrote:
>>> Hi David,
>>>
>>> (1) Deflater.deflate(Bytebuffer)
>>>  the api doc regarding "no_flush" appears to be the copy/paste of the
>>> byte[] version
>>>  without being updated to the corresponding ByteBuffer?
>>
>> You're right, I missed that one.  I've incorporated this fix locally:
>
> Oops, this should have been:
>
> --- 8< --- cut here --- 8< ---
>
> diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
> b/src/java.base/share/classes/java/util/zip/Deflater.java
> index 524125787a8..40f0d9736e2 100644
> --- a/src/java.base/share/classes/java/util/zip/Deflater.java
> +++ b/src/java.base/share/classes/java/util/zip/Deflater.java
> @@ -481,9 +481,9 @@ public class Deflater {
>   * in order to determine if more input data is required.
>   *
>   * This method uses {@link #NO_FLUSH} as its compression flush mode.
> - * An invocation of this method of the form {@code deflater.deflate(b)}
> + * An invocation of this method of the form {@code
> deflater.deflate(output)}
>   * yields the same result as the invocation of
> - * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
> + * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
>   *
>   * @param output the buffer for the compressed data
>   * @return the actual number of bytes of compressed data written to the
>
> --- 8< --- cut here --- 8< ---
>
> --
> - DML



-- 
- DML

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

[David Lloyd]

The fourth version of this patch is attached.  There is one functional
change; the rest are documentation changes.

The functional change is that Inflater now updates both the input and
output buffer position, as well as the produced/consumed byte count
and remaining count, in the event of a DataFormatException.  The
documentation changes reflect this change.

An online version of this patch can be found here [1].

[1] https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v12

-- 
- DML
commit 48f02037c6958828c41579748827134f1d6b58e9
Author: David M. Lloyd 
Date:   Fri Feb 16 11:00:10 2018 -0600

[JDK-6341887] Update Inflater/Deflater to handle ByteBuffer

diff --git a/make/mapfiles/libzip/mapfile-vers 
b/make/mapfiles/libzip/mapfile-vers
index d711d8e17f4..11ccc2d6ecb 100644
--- a/make/mapfiles/libzip/mapfile-vers
+++ b/make/mapfiles/libzip/mapfile-vers
@@ -33,20 +33,28 @@ SUNWprivate_1.1 {
Java_java_util_zip_CRC32_update;
Java_java_util_zip_CRC32_updateBytes0;
Java_java_util_zip_CRC32_updateByteBuffer0;
-   Java_java_util_zip_Deflater_deflateBytes;
+   Java_java_util_zip_Deflater_deflateBytesBytes;
+   Java_java_util_zip_Deflater_deflateBytesBuffer;
+   Java_java_util_zip_Deflater_deflateBufferBytes;
+   Java_java_util_zip_Deflater_deflateBufferBuffer;
Java_java_util_zip_Deflater_end;
Java_java_util_zip_Deflater_getAdler;
Java_java_util_zip_Deflater_init;
Java_java_util_zip_Deflater_initIDs;
Java_java_util_zip_Deflater_reset;
Java_java_util_zip_Deflater_setDictionary;
+   Java_java_util_zip_Deflater_setDictionaryBuffer;
Java_java_util_zip_Inflater_end;
Java_java_util_zip_Inflater_getAdler;
-   Java_java_util_zip_Inflater_inflateBytes;
+   Java_java_util_zip_Inflater_inflateBytesBytes;
+   Java_java_util_zip_Inflater_inflateBytesBuffer;
+   Java_java_util_zip_Inflater_inflateBufferBytes;
+   Java_java_util_zip_Inflater_inflateBufferBuffer;
Java_java_util_zip_Inflater_init;
Java_java_util_zip_Inflater_initIDs;
Java_java_util_zip_Inflater_reset;
Java_java_util_zip_Inflater_setDictionary;
+   Java_java_util_zip_Inflater_setDictionaryBuffer;
ZIP_Close;
ZIP_CRC32;
ZIP_FreeEntry;
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java 
b/src/java.base/share/classes/java/util/zip/Deflater.java
index c75dd4a33f0..40f0d9736e2 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -26,7 +26,13 @@
 package java.util.zip;
 
 import java.lang.ref.Cleaner.Cleanable;
+import java.lang.ref.Reference;
+import java.nio.ByteBuffer;
+import java.nio.ReadOnlyBufferException;
+import java.util.Objects;
+
 import jdk.internal.ref.CleanerFactory;
+import sun.nio.ch.DirectBuffer;
 
 /**
  * This class provides support for general purpose compression using the
@@ -92,8 +98,9 @@ import jdk.internal.ref.CleanerFactory;
 public class Deflater {
 
 private final DeflaterZStreamRef zsRef;
-private byte[] buf = new byte[0];
-private int off, len;
+private ByteBuffer input = ZipUtils.defaultBuf;
+private byte[] inputArray;
+private int inputPos, inputLim;
 private int level, strategy;
 private boolean setParams;
 private boolean finish, finished;
@@ -170,9 +177,14 @@ public class Deflater {
  */
 public static final int FULL_FLUSH = 3;
 
+/**
+ * Flush mode to use at the end of output.  Can only be provided by the
+ * user by way of {@link #finish()}.
+ */
+private static final int FINISH = 4;
+
 static {
 ZipUtils.loadLibrary();
-initIDs();
 }
 
 /**
@@ -216,16 +228,14 @@ public class Deflater {
  * @see Deflater#needsInput
  */
 public void setInput(byte[] b, int off, int len) {
-if (b== null) {
-throw new NullPointerException();
-}
 if (off < 0 || len < 0 || off > b.length - len) {
 throw new ArrayIndexOutOfBoundsException();
 }
 synchronized (zsRef) {
-this.buf = b;
-this.off = off;
-this.len = len;
+this.input = null;
+this.inputArray = b;
+this.inputPos = off;
+this.inputLim = off + len;
 }
 }
 
@@ -239,6 +249,31 @@ public class Deflater {
 setInput(b, 0, b.length);
 }
 
+/**
+ * Sets input data for compression. This should be called whenever
+ * needsInput() returns true indicating that more input data is required.
+ * 
+ * The given buffer's position will be updated as deflate operations are
+ * performed.  The in

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

[David Lloyd]

On Fri, Mar 2, 2018 at 2:34 PM, David Lloyd  wrote:
> On Fri, Mar 2, 2018 at 12:49 PM, Xueming Shen  wrote:
>> Hi David,
>>
>> (1) Deflater.deflate(Bytebuffer)
>>  the api doc regarding "no_flush" appears to be the copy/paste of the
>> byte[] version
>>  without being updated to the corresponding ByteBuffer?
>
> You're right, I missed that one.  I've incorporated this fix locally:

Oops, this should have been:

--- 8< --- cut here --- 8< ---

diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
b/src/java.base/share/classes/java/util/zip/Deflater.java
index 524125787a8..40f0d9736e2 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -481,9 +481,9 @@ public class Deflater {
  * in order to determine if more input data is required.
  *
  * This method uses {@link #NO_FLUSH} as its compression flush mode.
- * An invocation of this method of the form {@code deflater.deflate(b)}
+ * An invocation of this method of the form {@code
deflater.deflate(output)}
  * yields the same result as the invocation of
- * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
+ * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
  *
  * @param output the buffer for the compressed data
  * @return the actual number of bytes of compressed data written to the

--- 8< --- cut here --- 8< ---

-- 
- DML

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

[David Lloyd]

On Fri, Mar 2, 2018 at 12:49 PM, Xueming Shen  wrote:
> Hi David,
>
> (1) Deflater.deflate(Bytebuffer)
>  the api doc regarding "no_flush" appears to be the copy/paste of the
> byte[] version
>  without being updated to the corresponding ByteBuffer?

You're right, I missed that one.  I've incorporated this fix locally:

--- 8< --- cut here --- 8< ---

diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
b/src/java.base/share/classes/java/util/zip/Deflater.java
index 524125787a8..9cf735a9477 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -483,7 +483,7 @@ public class Deflater {
  * This method uses {@link #NO_FLUSH} as its compression flush mode.
  * An invocation of this method of the form {@code deflater.deflate(b)}
  * yields the same result as the invocation of
- * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
+ * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
  *
  * @param output the buffer for the compressed data
  * @return the actual number of bytes of compressed data written to the

--- 8< --- cut here --- 8< ---

I'll post an amended patch once we work out the other points below
(and any other feedback that comes in the meantime).

> (2) Inflater.inflate()
>  a "inputConsumed" field is added to handle the "bytes-read" if a DFE is
> being thrown.
>
>  While the API doc specifies that "if the setInput(bytebuffer) method
> was call "
>  it appears the inputPos/bytesRead are being updated for the
> "setInput(byte[]) case
>  as well. This might be a desirable change, but is an "incompatible"
> behavior change
>  as well. I doubt if there is any existing app depending on this
> existing behavior though,
>  it's really hard, if not impossible, to try to recover from this kind
> of error

I agree; this was implemented based on Alan's points but I could go
either way with it.

I had a thought that I could update the position only in the buffer
input case, not the array input case, but this would still have the
effect of changing the behavior of Inflater.getRemaining() when the
input is a buffer, which is still technically an incompatibility.  So
the three options are:

* Always update the input position on exception
* Only update the input position on exception when the input is a ByteBuffer
* Never update the input position on exception

AFAICT there are no inherent technical issues with any of these
approaches beyond the obscure compatibility change.  Since the
behavior was previously unspecified, I think that decreases the
compatibility risk even further, so I'm inclined to think that #1 is
the best overall choice.

>  There might also bring in a little inconsistency, as we are updating
> the "input" in this
>  case,  but why not the "output" buffer? It's true there is no way to do
> that with the
>  inflate(byte[] ...), it's kinda doable for the inflate(ByteBuffer) and
> in fact there might
>  be bytes that have been written into the output ByteBuffer in this
> case.

Since there is no way to get the output bytes consumed in the array
case, this is not an incompatible change, so I am happy to include it
in my next revision.  However I think it only makes sense to do this
if it is consistent with input; i.e. if option #3 above is chosen then
it would be inconsistent to update the output position on exception
and we should not do it.

> (3) there are usages like
>  Math.max(buf.limit() - outputPos, 0);
>  just wonder if there is any reason not using existing api method.
>  Math.max(buf.remaining(), 0);

This is because the buffer's position can change between the call to
buf.position() and buf.remaining(), which can have the ultimate effect
of allowing invalid memory accesses.  By acquiring the position and
limit each one time, I can guarantee some consistent view of these
properties.  This is consistent with (for example) sun.nio.ch.IOUtil.

Also just as a general principle, buf.remaining() reads both the limit
and position, and since I already have the position, it makes sense to
just read the limit which is the only other piece of data that must be
read in order to correctly calculate these values.

Thanks for the feedback.


-- 
- DML

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

[David Lloyd]

The third... and hopefully final... version of this patch is attached.
It is the same as V2, however it also uses
Reference.reachabilityFence() defensively on buffers.  This may be
necessary because there are code paths which may allow such buffers to
be GCed after their address is acquired but before the native code
successfully is able to read it.

The online version of this patch is visible at [1].

[1] https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v8

-- 
- DML
commit 09313d25a1362039806a0c1b2d443874eb101843
Author: David M. Lloyd 
Date:   Fri Feb 16 11:00:10 2018 -0600

[JDK-6341887] Update Inflater/Deflater to handle ByteBuffer

diff --git a/make/mapfiles/libzip/mapfile-vers 
b/make/mapfiles/libzip/mapfile-vers
index d711d8e17f4..11ccc2d6ecb 100644
--- a/make/mapfiles/libzip/mapfile-vers
+++ b/make/mapfiles/libzip/mapfile-vers
@@ -33,20 +33,28 @@ SUNWprivate_1.1 {
Java_java_util_zip_CRC32_update;
Java_java_util_zip_CRC32_updateBytes0;
Java_java_util_zip_CRC32_updateByteBuffer0;
-   Java_java_util_zip_Deflater_deflateBytes;
+   Java_java_util_zip_Deflater_deflateBytesBytes;
+   Java_java_util_zip_Deflater_deflateBytesBuffer;
+   Java_java_util_zip_Deflater_deflateBufferBytes;
+   Java_java_util_zip_Deflater_deflateBufferBuffer;
Java_java_util_zip_Deflater_end;
Java_java_util_zip_Deflater_getAdler;
Java_java_util_zip_Deflater_init;
Java_java_util_zip_Deflater_initIDs;
Java_java_util_zip_Deflater_reset;
Java_java_util_zip_Deflater_setDictionary;
+   Java_java_util_zip_Deflater_setDictionaryBuffer;
Java_java_util_zip_Inflater_end;
Java_java_util_zip_Inflater_getAdler;
-   Java_java_util_zip_Inflater_inflateBytes;
+   Java_java_util_zip_Inflater_inflateBytesBytes;
+   Java_java_util_zip_Inflater_inflateBytesBuffer;
+   Java_java_util_zip_Inflater_inflateBufferBytes;
+   Java_java_util_zip_Inflater_inflateBufferBuffer;
Java_java_util_zip_Inflater_init;
Java_java_util_zip_Inflater_initIDs;
Java_java_util_zip_Inflater_reset;
Java_java_util_zip_Inflater_setDictionary;
+   Java_java_util_zip_Inflater_setDictionaryBuffer;
ZIP_Close;
ZIP_CRC32;
ZIP_FreeEntry;
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java 
b/src/java.base/share/classes/java/util/zip/Deflater.java
index c75dd4a33f0..524125787a8 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -26,7 +26,13 @@
 package java.util.zip;
 
 import java.lang.ref.Cleaner.Cleanable;
+import java.lang.ref.Reference;
+import java.nio.ByteBuffer;
+import java.nio.ReadOnlyBufferException;
+import java.util.Objects;
+
 import jdk.internal.ref.CleanerFactory;
+import sun.nio.ch.DirectBuffer;
 
 /**
  * This class provides support for general purpose compression using the
@@ -92,8 +98,9 @@ import jdk.internal.ref.CleanerFactory;
 public class Deflater {
 
 private final DeflaterZStreamRef zsRef;
-private byte[] buf = new byte[0];
-private int off, len;
+private ByteBuffer input = ZipUtils.defaultBuf;
+private byte[] inputArray;
+private int inputPos, inputLim;
 private int level, strategy;
 private boolean setParams;
 private boolean finish, finished;
@@ -170,9 +177,14 @@ public class Deflater {
  */
 public static final int FULL_FLUSH = 3;
 
+/**
+ * Flush mode to use at the end of output.  Can only be provided by the
+ * user by way of {@link #finish()}.
+ */
+private static final int FINISH = 4;
+
 static {
 ZipUtils.loadLibrary();
-initIDs();
 }
 
 /**
@@ -216,16 +228,14 @@ public class Deflater {
  * @see Deflater#needsInput
  */
 public void setInput(byte[] b, int off, int len) {
-if (b== null) {
-throw new NullPointerException();
-}
 if (off < 0 || len < 0 || off > b.length - len) {
 throw new ArrayIndexOutOfBoundsException();
 }
 synchronized (zsRef) {
-this.buf = b;
-this.off = off;
-this.len = len;
+this.input = null;
+this.inputArray = b;
+this.inputPos = off;
+this.inputLim = off + len;
 }
 }
 
@@ -239,6 +249,31 @@ public class Deflater {
 setInput(b, 0, b.length);
 }
 
+/**
+ * Sets input data for compression. This should be called whenever
+ * needsInput() returns true indicating that more input data is required.
+ * 
+ * The given buffer's position will be updated as deflate operations are
+ * performed.  The input buffer may

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

[David Lloyd]

This is the second version of the patch first discussed here [1] to
add the ability to inflate/deflate to/from heap/direct byte buffers.

The patch is attached, as well as benchmark results with the original
and new code, and the benchmark source.  The patch is also visible
online at [2].

Some notable features of this version:

• Complete JavaDoc per Alan's reminder
• Misc. small bug fixes
• Removed nearly all JNI field accesses, replaced with simple
bit-fields on input and output parameters to inflate* and deflate*
native methods
• Fixed a problem Alan alluded to where an inflate operation can throw
DataFormatException yet still have consumed some input data; this is
done by way of a single field-based output parameter which is only
used when this exception is thrown
• Cleaned up "finish" handling on the deflate side
• Implemented Sherman's suggestion to support both array-based and
buffer-based input; no ByteBuffer wrapping is used in this version
• Updated FlaterTest to test all combinations of inflate/deflate
array/heap/direct
• Per-call performance seems generally improved overall; from the JMH results:
◦ JDK version completed 95% of 16-byte inflate ops within 1262
ns/op (array in, array out)
◦ New version completes 95% of 16-byte inflate ops within 1124
ns/op (array in, array out)
◦ In addition, the new direct buffer modes show slightly better
results at 1114 ns/op for 16-byte ops for direct buffer in & out - not
accounting for possible secondary improvements resulting from avoiding
Get*Critical JNI functions (as well as the obvious: avoiding the need
to allocate & copy between byte arrays and direct buffers)

The JMH test methods are named based on the type of input and output
done for each test, where "A" signifies "array", "H" signifies "heap
buffer", and "D" signifies "direct buffer".

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-February/051562.html
[2] https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v6

-- 
- DML
commit 04a83977c5b70d1a9d5850b05fc8b585356b6c2a
Author: David M. Lloyd 
Date:   Fri Feb 16 11:00:10 2018 -0600

[JDK-6341887] Update Inflater/Deflater to handle ByteBuffer

diff --git a/make/mapfiles/libzip/mapfile-vers 
b/make/mapfiles/libzip/mapfile-vers
index d711d8e17f4..11ccc2d6ecb 100644
--- a/make/mapfiles/libzip/mapfile-vers
+++ b/make/mapfiles/libzip/mapfile-vers
@@ -33,20 +33,28 @@ SUNWprivate_1.1 {
Java_java_util_zip_CRC32_update;
Java_java_util_zip_CRC32_updateBytes0;
Java_java_util_zip_CRC32_updateByteBuffer0;
-   Java_java_util_zip_Deflater_deflateBytes;
+   Java_java_util_zip_Deflater_deflateBytesBytes;
+   Java_java_util_zip_Deflater_deflateBytesBuffer;
+   Java_java_util_zip_Deflater_deflateBufferBytes;
+   Java_java_util_zip_Deflater_deflateBufferBuffer;
Java_java_util_zip_Deflater_end;
Java_java_util_zip_Deflater_getAdler;
Java_java_util_zip_Deflater_init;
Java_java_util_zip_Deflater_initIDs;
Java_java_util_zip_Deflater_reset;
Java_java_util_zip_Deflater_setDictionary;
+   Java_java_util_zip_Deflater_setDictionaryBuffer;
Java_java_util_zip_Inflater_end;
Java_java_util_zip_Inflater_getAdler;
-   Java_java_util_zip_Inflater_inflateBytes;
+   Java_java_util_zip_Inflater_inflateBytesBytes;
+   Java_java_util_zip_Inflater_inflateBytesBuffer;
+   Java_java_util_zip_Inflater_inflateBufferBytes;
+   Java_java_util_zip_Inflater_inflateBufferBuffer;
Java_java_util_zip_Inflater_init;
Java_java_util_zip_Inflater_initIDs;
Java_java_util_zip_Inflater_reset;
Java_java_util_zip_Inflater_setDictionary;
+   Java_java_util_zip_Inflater_setDictionaryBuffer;
ZIP_Close;
ZIP_CRC32;
ZIP_FreeEntry;
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java 
b/src/java.base/share/classes/java/util/zip/Deflater.java
index c75dd4a33f0..36e08a1dfc7 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -26,7 +26,12 @@
 package java.util.zip;
 
 import java.lang.ref.Cleaner.Cleanable;
+import java.nio.ByteBuffer;
+import java.nio.ReadOnlyBufferException;
+import java.util.Objects;
+
 import jdk.internal.ref.CleanerFactory;
+import sun.nio.ch.DirectBuffer;
 
 /**
  * This class provides support for general purpose compression using the
@@ -92,8 +97,9 @@ import jdk.internal.ref.CleanerFactory;
 public class Deflater {
 
 private final DeflaterZStreamRef zsRef;
-private byte[] buf = new byte[0];
-private int off, len;
+private ByteBuffer input = ZipUtils.defaultBuf;
+private byte[] inputArray;
+private int inputPos, inpu

Re: Draft JEP: To use UTF-8 as the default charset for the Java virtual machine.

[David Lloyd]

I agree with Uwe and Remi; if the default is still changeable, the
problem doesn't go away, it simply becomes slightly more insidious.

On Wed, Feb 21, 2018 at 12:31 AM, Xueming Shen  wrote:
> This draft JEP contains a proposal to use UTF-8 as the default charset for
> the JVM, so that
> APIs that depend on the default charset behave consistently cross all
> platforms.
>
> For more details, please see:
> https://bugs.openjdk.java.net/browse/JDK-8187041
>
> Sherman



-- 
- DML

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

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: [JDK-6341887] Patch: java.util.zip: Add ByteBuffer methods to Inflater/Deflater

[David Lloyd]

Also available in more readable form at:

  
https://github.com/dmlloyd/openjdk/commit/becd36e852e55a29a4685577453944552c817b66

On Fri, Feb 16, 2018 at 4:13 PM, David Lloyd  wrote:
> It would be convenient to be able to inflate/deflate a direct or heap
> byte buffer without having to copy it through an array first.  For my
> Friday mini-project this week, I've decided to take a crack at this.
> The attached patch is the result.  Would anyone be interested in
> reviewing and maybe sponsoring this change?  It would be really great
> to get it in to JDK 11.
>
> The patch includes a modification to FlaterTest to run through
> permutations of using direct and heap byte buffers.  Though I couldn't
> get jtreg working, I did compile and run the test by hand and it seems
> to pass; the build also works fine with the new code.
>
> Extra thanks to Martin Balao for pointing me towards mapfile-vers when
> I couldn't figure out why I was getting UnsatisfiedLinkError.  That
> one was not obvious.
> --
> - DML



-- 
- DML

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

[David Lloyd]

It would be convenient to be able to inflate/deflate a direct or heap
byte buffer without having to copy it through an array first.  For my
Friday mini-project this week, I've decided to take a crack at this.
The attached patch is the result.  Would anyone be interested in
reviewing and maybe sponsoring this change?  It would be really great
to get it in to JDK 11.

The patch includes a modification to FlaterTest to run through
permutations of using direct and heap byte buffers.  Though I couldn't
get jtreg working, I did compile and run the test by hand and it seems
to pass; the build also works fine with the new code.

Extra thanks to Martin Balao for pointing me towards mapfile-vers when
I couldn't figure out why I was getting UnsatisfiedLinkError.  That
one was not obvious.
-- 
- DML
commit becd36e852e55a29a4685577453944552c817b66
Author: David M. Lloyd 
Date:   Fri Feb 16 11:00:10 2018 -0600

[JDK-6341887] Update Inflater/Deflator to handle ByteBuffer

diff --git a/make/mapfiles/libzip/mapfile-vers 
b/make/mapfiles/libzip/mapfile-vers
index d711d8e17f4..11ccc2d6ecb 100644
--- a/make/mapfiles/libzip/mapfile-vers
+++ b/make/mapfiles/libzip/mapfile-vers
@@ -33,20 +33,28 @@ SUNWprivate_1.1 {
Java_java_util_zip_CRC32_update;
Java_java_util_zip_CRC32_updateBytes0;
Java_java_util_zip_CRC32_updateByteBuffer0;
-   Java_java_util_zip_Deflater_deflateBytes;
+   Java_java_util_zip_Deflater_deflateBytesBytes;
+   Java_java_util_zip_Deflater_deflateBytesBuffer;
+   Java_java_util_zip_Deflater_deflateBufferBytes;
+   Java_java_util_zip_Deflater_deflateBufferBuffer;
Java_java_util_zip_Deflater_end;
Java_java_util_zip_Deflater_getAdler;
Java_java_util_zip_Deflater_init;
Java_java_util_zip_Deflater_initIDs;
Java_java_util_zip_Deflater_reset;
Java_java_util_zip_Deflater_setDictionary;
+   Java_java_util_zip_Deflater_setDictionaryBuffer;
Java_java_util_zip_Inflater_end;
Java_java_util_zip_Inflater_getAdler;
-   Java_java_util_zip_Inflater_inflateBytes;
+   Java_java_util_zip_Inflater_inflateBytesBytes;
+   Java_java_util_zip_Inflater_inflateBytesBuffer;
+   Java_java_util_zip_Inflater_inflateBufferBytes;
+   Java_java_util_zip_Inflater_inflateBufferBuffer;
Java_java_util_zip_Inflater_init;
Java_java_util_zip_Inflater_initIDs;
Java_java_util_zip_Inflater_reset;
Java_java_util_zip_Inflater_setDictionary;
+   Java_java_util_zip_Inflater_setDictionaryBuffer;
ZIP_Close;
ZIP_CRC32;
ZIP_FreeEntry;
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java 
b/src/java.base/share/classes/java/util/zip/Deflater.java
index c75dd4a33f0..5ededeb56ca 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -26,6 +26,9 @@
 package java.util.zip;
 
 import java.lang.ref.Cleaner.Cleanable;
+import java.nio.ByteBuffer;
+import java.util.Objects;
+
 import jdk.internal.ref.CleanerFactory;
 
 /**
@@ -92,8 +95,7 @@ import jdk.internal.ref.CleanerFactory;
 public class Deflater {
 
 private final DeflaterZStreamRef zsRef;
-private byte[] buf = new byte[0];
-private int off, len;
+private ByteBuffer input;
 private int level, strategy;
 private boolean setParams;
 private boolean finish, finished;
@@ -216,17 +218,11 @@ public class Deflater {
  * @see Deflater#needsInput
  */
 public void setInput(byte[] b, int off, int len) {
-if (b== null) {
-throw new NullPointerException();
-}
+Objects.requireNonNull(b);
 if (off < 0 || len < 0 || off > b.length - len) {
 throw new ArrayIndexOutOfBoundsException();
 }
-synchronized (zsRef) {
-this.buf = b;
-this.off = off;
-this.len = len;
-}
+setInput(ByteBuffer.wrap(b, off, len));
 }
 
 /**
@@ -236,7 +232,22 @@ public class Deflater {
  * @see Deflater#needsInput
  */
 public void setInput(byte[] b) {
-setInput(b, 0, b.length);
+// freeload the NPE off of wrap()
+setInput(ByteBuffer.wrap(b));
+}
+
+/**
+ * Sets input data for compression. This should be called whenever
+ * needsInput() returns true indicating that more input data is required.
+ * @param byteBuffer the input data bytes
+ * @see Deflater#needsInput
+ * @since 11
+ */
+public void setInput(ByteBuffer byteBuffer) {
+Objects.requireNonNull(byteBuffer);
+synchronized (zsRef) {
+this.input = byteBuffer;
+}
 }
 
 /**
@@ -252,16 +263,11 @@ public clas

Re: RFR: JDK-8021560,(str) String constructors that take ByteBuffer

[David Lloyd]

On Tue, Feb 13, 2018 at 2:41 AM, Alan Bateman  wrote:
> On 13/02/2018 06:24, Xueming Shen wrote:
>>
>> Hi,
>>
>> Please help review the proposal to add following constructors and methods
>> in String
>> class to take ByteBuffer as the input and output data buffer.
>>
>> public String(ByteBuffer bytes, Charset cs);
>> public String(ByteBuffer bytes, String csname);
>
> These constructors looks good (for the parameter names then I assume you
> meant "src" rather than "bytes" here).
>
>> public int getBytes(byte[] dst, int offset, Charset cs);
>> public int getBytes(byte[] dst, int offset, String csname);
>> public int getBytes(ByteBuffer bytes, Charset cs);
>> public int getBytes(ByteBuffer bytes, Charset csn);
>
> These four methods encode as many characters as possible into the
> destination byte[] or buffer but don't give any indication that the
> destination didn't have enough space to encode the entire string. I thus
> worry they could be a hazard and result in buggy code. If there is
> insufficient space then the user of the API doesn't know how many characters
> were encoded so it's not easy to substring and call getBytes again to encode
> the remaining characters. There is also the issue of how to size the
> destination. What would you think about having them fail when there is
> insufficient space? If they do fail then there is a side effect that they
> will have written to the destination so that would need to be documented
> too.

The ones that output to a ByteBuffer have more flexibility in that the
buffer position can be moved according to the number of bytes written,
but the method _could_ return the number of _chars_ actually written.
But this is not particularly useful without variants which accept an
offset into the string, unless it can be shown that
s.substring(coffs).getBytes(xxx) is reasonably efficient.

It might be better to shuffle this around a little and instead have a
Charset[Encoder].getBytes(int codePoint, byte[] b, int offs, int
len)/.getBytes(int codePoint, ByteBuffer buf) kind of thing which
returns the number of bytes or e.g. -1 or -count if there isn't enough
space in the target.  Then it would be less onerous for users to write
simple for-each-codepoint loops which encode as far as is reasonable
but no farther without too many error-handling gymnastics.

-- 
- DML