Re: RFR(s): 8212828: (process) Change the Process launch mechanism default on Linux to be posix_spawn

[Thomas Stüfe]

On Mon, Feb 11, 2019 at 9:18 PM Martin Buchholz  wrote:

> Looks good to me.
>
>
Thank you, I just pushed.


> It's true that these tests depending on external tools are very brittle.
> In particular, strace is in the middle of a flag re-org
>
>-e trace=%process
>-e trace=process (deprecated)
>
> Nevertheless, we have such tests - are they worth the maintenance burden?
> My own Zombies.java test is a good example.
>

It is more useful than my proposed test was. I wince a bit at the perl
requirement though. Especially since the test silently quits if no perl is
installed.

(As a side note, I wonder whether we could have a mechanism to signal
requirements not met, eg. with a TestRequirementsNotMetException, and then
let the test executor decide what to do: warn, ignore, error...)

I guess part of this could be redone nowadays with Rogers ProcessHandle API
(the child seeking), but we still would need to find out if the child is a
zombie.

I like the test name btw. Very succinct :)

..Thomas



>
> On Mon, Feb 11, 2019 at 10:50 AM Thomas Stüfe 
> wrote:
>
>> Hi Roger, Martin,
>>
>> hopefully final version:
>>
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.03/webrev/
>>
>> I removed the test and the changes in the test library made for the test.
>> Test is just too brittle with too little nourishing value. Everything else
>> is unchanged from webrev.02.
>>
>> Thank you, Thomas
>>
>>
>>
>> On Fri, Feb 8, 2019 at 10:38 AM Thomas Stüfe 
>> wrote:
>>
>>> Hi Roger, Martin,
>>>
>>> next version:
>>>
>>>
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.02/webrev
>>>
>>> - did massage the comment in ProcessImpl.c
>>> - made the test more resilient by scanning for the strace tool and by
>>> silently ignoring all problems stemming from strace or the payload binary
>>> not being there. The test now only fails if the forks were successully
>>> executed but it does not seem to use posix-spawn.
>>> - added output to the test by printing the "interesting" lines of the
>>> strace output. Note that this filtering is not really sophisticated and
>>> will show all thread related clone() calls as well:
>>>
>>> 614   [pid 12447] <... clone resumed> child_stack=0x7fe00c4baff0,
>>> flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
>>> parent_tidptr=0x7fe00c4bb9d0, tls=0x7fe00c4bb700,
>>> child_tidptr=0x7fe00c4bb9d0) = 12452
>>> 646   [pid 12447] clone(/usr/bin/strace: Process 12453 attached
>>> 649   [pid 12447] <... clone resumed> child_stack=0x7fe00c3b9ff0,
>>> flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
>>> parent_tidptr=0x7fe00c3ba9d0, tls=0x7fe00c3ba700,
>>> child_tidptr=0x7fe00c3ba9d0) = 12453
>>> 
>>>
>>> I am sure this could be made more intelligent but lets keep it simple
>>> for now.
>>>
>>> - I removed the helperPath() methods Roger mentioned, they are not
>>> needed anymore.
>>>
>>> @Martin: I like the -e signal=none -e trace=process idea but I'm not
>>> sure if all versions of strace support these options. I think the strace
>>> output is small enough for this small use case, some kB only.
>>>
>>> Cheers, Thomas
>>>
>>>
>>>
>>>
>>>
>>> On Thu, Feb 7, 2019 at 6:01 PM Thomas Stüfe 
>>> wrote:
>>>
 Hi all,

 second version, including the updated comment in ProcessImpl.c Martin
 requested:


 http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.01/webrev/index.html

 @Roger: thanks for feeding this into your tests. I still try to get it
 to run thru jdk-submit, but that seems to be stuck again..

 Cheers, Thomas





 On Wed, Feb 6, 2019 at 10:29 AM Thomas Stüfe 
 wrote:

> Hi all
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8213192
> webrev:
> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.00/webrev/index.html
>
> (@Roger: I hope you do not mind? The bug is assigned to you but since
> I happened to play around with posix_spawn I prepared this webrev. If you
> rather do this change, that is fine and I will leave it to you.)
>
> When we added the possibility to use posix_spawn as underlying
> implementation for Runtime.exec() on Linux with
> https://bugs.openjdk.java.net/browse/JDK-8212828, we agreed to keep
> VFORK as default until work on 13 starts. So now would be a good time to
> switch the default to posix_spawn to get a good testing window. Note that
> at SAP we run our VMs internally with posix_spawn as default since 

Re: RFR 8218228 : The constructor StringBuffer(CharSequence) violates spec for negatively sized argument

[Ivan Gerasimov]

Hi Roger!


On 2/11/19 7:30 AM, Roger Riggs wrote:

Hi Ivan,

I called it out because the CSR does not mention that the behavior
of some of the cases (-1..-16) is changing and some of the emails asserted
there was no change in behavior.

I'm fine with one changeset as long as both changes are explicit.
The bug and the CSR should both be updated.


Yes, this makes sense.
I've updated CSR accordingly, and added a comment to the bug to make the 
intention of the fix clear.


Could you please review the updated CSR?

With kind regards,
Ivan




Thanks, Roger


On 02/08/2019 05:12 PM, Ivan Gerasimov wrote:

Hi Roger!

That's because two tiny changes are combined in the patch:

1) remove a problematic statement from the javadoc, as it doesn't 
hold anyway.  This part does not change the behavior.


2) unify type of the exception thrown for all negative values of 
CharSequence.length().  The regression test is to verify this, second 
change, so it fails prior the fix for length in [-16, -1].


I can separate them into different bugs, if you think it will make it 
clearer.  Though I thought they can be fixed together.


With kind regards,

Ivan


On 2/8/19 1:49 PM, Roger Riggs wrote:

Hi Ivan,

In the direction of not changing the behavior; the webrev does 
change the behavior.


In the case of CharSeqence with length -1..-16, the new behavior 
throws NegativeArrayIndexException

instead of java.lang.IndexOutOfBoundsException.


AbstractStringBuilder:101-102 throw an exception for length < 0.
However, the current behavior is to create a StringBuffer with 
length + 16 and
then append the contents.  For -1..-16, it will succeed in creating 
a StringBuffer
but fail with IndexOutOfBoundsException during the append of the 
contents.


The new Test should pass both before and after the change to the code.

Roger


On 02/07/2019 10:19 PM, Ivan Gerasimov wrote:



On 2/7/19 6:33 PM, David Holmes wrote:

On 8/02/2019 11:59 am, Ivan Gerasimov wrote:

Hi David!


On 2/7/19 5:16 PM, David Holmes wrote:

Hi Ivan,

On 8/02/2019 11:02 am, Ivan Gerasimov wrote:

Hello!

The specification states:
"""
If the length of the specified CharSequence is less than or 
equal to zero, then an empty buffer of capacity 16 is returned.

"""

However, the current implementation throws either 
NegativeArraySizeException or IndexOutOfBounds instead (the 
actual exception type depends on the value of reported negative 
length).


How can you have a negative length CharSequence ??


A custom CharSequence returning negative length() can be created.
Not sure how useful/popular this may be, though.


Seems pretty meaningless so just treating that as an error seems 
fine. Though somewhat debatable whether you need to add an 
appropriate @throws.



Right.  There were two reasons not to add @throws here:
- by removing the problematic paragraph we just make the javadoc 
almost the same as for StringBuilder(CharSequence),
- we don't seem to have any other places (at least I couldn't find 
any) specifying exceptions due to negatively sized CharSequence.


(To be precise, there is one candidate, but I'll file a separate 
bug to fix it.)


That's why I propose just removing the mentioning negative 
length, and not changing the behavior.


The proposed code change is to only unify the behavior for any 
negative value of length.


Ok.

If its an empty CharSequence then it should return the empty 
buffer as indicated.



Empty CharSequence is processed correctly already.


Okay so by removing this part:

-  * 
-  * If the length of the specified {@code CharSequence} is
-  * less than or equal to zero, then an empty buffer of capacity
-  * {@code 16} is returned.

you're relying on the main specification to implicitly handle the 
empty case.



Yes.

With kind regards,
Ivan



David
-


With kind regards,
Ivan


Cheers,
David


It is proposed to do two things:
1) remove the problematic sentence from the javadoc (CSR is 
filed).
2) unify the exception type thrown, if the argument reports 
negative length.
NegativeArraySizeException will be consistent with, for 
example, StringBuffer(negativeCapacity).


Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228
WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/
CRS: https://bugs.openjdk.java.net/browse/JDK-8218649


















--
With kind regards,
Ivan Gerasimov

Re: JDK-6982173: Small problem causing thousands of wasted CPU hours

[Michael Rasmussen]

The current implementation seems very counter-intuitive with anything that 
doesn't have the same comparison semantics, for instance a TreeSet/Map with a 
Comparator, Identity-based Set/Map etc.

The output of the following snippet would probably surprise most:
/* --- snip --- */
import java.util.*;
class Test {
  public static void main(String[] args) {
TreeMap> lengthMap = new 
TreeMap<>(Comparator.comparingInt(String::length));
List strings = new ArrayList<>(Arrays.asList("The quick brown fox 
jumps over the lazy dog".split(" ")));
for (String s : strings) {
  lengthMap.computeIfAbsent(s, k -> new ArrayList<>()).add(s);
}
 
Set toRemove = lengthMap.keySet();
 
System.out.println("List before: " + strings);
System.out.println("Set to remove: " + toRemove);

strings.removeAll(toRemove);
System.out.println("List after:" + strings);
  }
}
/* --- snip --- */
List before: [The, quick, brown, fox, jumps, over, the, lazy, dog]
Set to remove: [The, over, quick]
List after:[]


/Michael


From: core-libs-dev  on behalf of Tagir 
Valeev 
Sent: 08 February 2019 15:13
To: Stuart Marks
Cc: core-libs-dev; Jan Peter Stotz
Subject: Re: JDK-6982173: Small problem causing thousands of wasted CPU hours
  

Hello!

> I would argue that iterating the argument and calling remove() on "this" are 
> the
> right semantics, because you want set membership to be determined by this set,
> not by whatever collection you pass as an argument. However, I note that
> AbstractCollection.removeAll and other removeAll implementations iterate over
> "this" and do a contains() check on the argument. The revised
> AbstractSet.removeAll would be an outlier here, though it makes sense to me to
> do it this way.

For complete picture it should be noted that there's a slight
difference in remove and removeAll spec: remove removes at most one
element while removeAll removes all elements from the specified
collection.
E.g. c.removeAll(Collections.singleton(foo)) would remove all
instances of foo from c while c.remove(foo) would return only one foo.
These should be equivalent for Set where repeating elements should not
normally appear, but it's wrong for any Collection. That's why
AbstractCollection.removeAll
cannot be rewritten in the same way.

Another interesting thing is
java.util.IdentityHashMap.KeySet#removeAll implementation [1]:
it reimplements the AbstractCollection#removeAll, because of the same
reason: now
the first branch of AbstractSet#removeAll becomes incorrect. See the
comment before it:

    /*
 * Must revert from AbstractSet's impl to AbstractCollection's, as
 * the former contains an optimization that results in incorrect
 * behavior when c is a smaller "normal" (non-identity-based) Set.
 */

Btw this comment should be updated to remove a "smaller" condition if
the proposed
change for AbstractSet will be implemented.

[1]  
http://hg.openjdk.java.net/jdk/jdk/file/e57bcfd7bf79/src/java.base/share/classes/java/util/IdentityHashMap.java#l990

With best regards,
Tagir Valeev


On Fri, Jan 25, 2019 at 7:11 AM Stuart Marks  wrote:
>
> On 1/23/19 12:05 PM, Jan Peter Stotz wrote:
> > like many other I ran into bug JDK-698217 which is about 
> > AbstractSet.removeAll()
> > and it's "aptitude" in wasting CPU time if you accidentally hit this bug. 
> > And
> > there are hundred of developers hitting this bug every year.
> >
> > I simply don't understand why there was no progress in 8 years, on a severe
> > performance issue (a removeAll method on an efficient set that can require
> > O(n^2)!) caused by code that was written to speed-up the removeAll 
> > implementation.
> >
> > Which makes this bug worse is that it is triggered by the relative size of 
> > the
> > current set compared to the collection passed as parameter.
> > Therefore for most developers this means not to use this buggy function at 
> > all
> > (once they realized how worse it is).
>
> I wasn't aware that hundreds of developers are hitting this bug every year. I
> haven't seen any mention of it (besides in the bug database) on Twitter, on
> Reddit, on DZone, at the conferences I attend, or in several years of
> core-libs-dev emails. Well, it was mentioned on core-libs-dev once in 2011 [1]
> although that was a suggestion for improvement, not a complaint about 
> performance.
>
> Nonetheless this is a real problem, and if it's causing difficulties we can
> certainly take a look at it.
>
> There is, however, more to the story. The ACTUAL problem is a semantic one; 
> see
> JDK-6394757. [2] Briefly, consider x.removeAll(y). Depending on the relative
> sizes of x and y, this method might end up using either x's or y's definition 
> of
> membership, which could differ from each other. (See the bug report for an
> example.) Thus the semantics of this method depend upon the relative sizes of
> the collections, which is arguably flawed.
>
> Worse, this behavior is specified to iterate this set or the 

Java SSLSocketChannel/SSLSelector?

[Andi Mullaraj]

Hi java-core community,

I have been directed to this channel to discuss matters related to Java
performant ssl communications.  Maybe this is a topic that has been
discussed in the past, in which case I would appreciate if someone pointed
me to that particular topic.

Back in 2002 I personally applauded the java.nio.channels (jdk1.4) package,
realizing that there was no way to port this to the secure communications,
due to lack of a comparable implementation for SSL.  But I thought it was
going to just be matter of time.  A couple of years ago I had to go back
search for it, and was kind of surprised to find the following in
http://docs.oracle.com/javase/7/docs/technotes/guides/security/jsse/JSSERefGuide.html#SSLENG
:

--- begin quote ---
Newcomers to the API may wonder "Why not just have an SSLSocketChannel
which extends java.nio.channels.SocketChannel?" There are two main reasons:

1. There were a lot of very difficult questions about what a
SSLSocketChannel should be, including its class hierarchy and how it should
interoperate with Selectors and other types of SocketChannels. Each
proposal brought up more questions than answers. It was noted that any new
API abstraction extended to work with SSL/TLS would require the same
significant analysis and could result in large and complex APIs.
2. Any JSSE implementation of a new API would be free to choose the "best"
I/O & compute strategy, but hiding any of these details is inappropriate
for those applications needing full control. Any specific implementation
would be inappropriate for some application segment.
--- end quote ---

It has been a while since this was stated, and I would really like to
revisit it.  I personally believe that the question #1 has a quite natural
answer, while #2 should not really be a concern if we adhere to the spi
model where users can bring their own implementation if needed.  I know
because I have also implemented it (but would like to discuss that in a
later stage, if it comes to it).

The benefit of such implementation would be immense.  Imagine many Java
services written with nio and which struggle to move to SSL due to the
great complexity of using SSLEngine (zookeeper service to name one), while
all they would need to do (if SSLSocketChannels were available) is to
instantiate an instance of SSLSocketChannel/SSLSelector when the security
is required and the rest would stay the same (as in case of plain
communication using SocketChannel/Selector).  Another important use case is
the advent of IOT, where millions of devices, with relatively low
throughput would want to protect their communication via SSL.

The ways I have seen the community deal with this are mainly:

1. Go through the pain of learning SSLEngine (and hope to get it right)
2. Build their services around tested libraries (like Jetty, which handle
the SSL offload but don't resurface it in SocketChannel/Selector paradigm,
that also come with their huge set of dependencies, bringing in unavoidable
version collisions)
3. Use proxies (nginx, ha) to deal with the high number of SSL connections
and divide the load by scaling horizontally in the back end (making for a
harder deployment model)

We can make this a lot easier!

Any feedback is greatly appreciated,
Best,

Andi Mullaraj

Re: RFR(s): 8212828: (process) Change the Process launch mechanism default on Linux to be posix_spawn

[Martin Buchholz]

Looks good to me.

It's true that these tests depending on external tools are very brittle.
In particular, strace is in the middle of a flag re-org

   -e trace=%process
   -e trace=process (deprecated)

Nevertheless, we have such tests - are they worth the maintenance burden?
My own Zombies.java test is a good example.


On Mon, Feb 11, 2019 at 10:50 AM Thomas Stüfe 
wrote:

> Hi Roger, Martin,
>
> hopefully final version:
>
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.03/webrev/
>
> I removed the test and the changes in the test library made for the test.
> Test is just too brittle with too little nourishing value. Everything else
> is unchanged from webrev.02.
>
> Thank you, Thomas
>
>
>
> On Fri, Feb 8, 2019 at 10:38 AM Thomas Stüfe 
> wrote:
>
>> Hi Roger, Martin,
>>
>> next version:
>>
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.02/webrev
>>
>> - did massage the comment in ProcessImpl.c
>> - made the test more resilient by scanning for the strace tool and by
>> silently ignoring all problems stemming from strace or the payload binary
>> not being there. The test now only fails if the forks were successully
>> executed but it does not seem to use posix-spawn.
>> - added output to the test by printing the "interesting" lines of the
>> strace output. Note that this filtering is not really sophisticated and
>> will show all thread related clone() calls as well:
>>
>> 614   [pid 12447] <... clone resumed> child_stack=0x7fe00c4baff0,
>> flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
>> parent_tidptr=0x7fe00c4bb9d0, tls=0x7fe00c4bb700,
>> child_tidptr=0x7fe00c4bb9d0) = 12452
>> 646   [pid 12447] clone(/usr/bin/strace: Process 12453 attached
>> 649   [pid 12447] <... clone resumed> child_stack=0x7fe00c3b9ff0,
>> flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
>> parent_tidptr=0x7fe00c3ba9d0, tls=0x7fe00c3ba700,
>> child_tidptr=0x7fe00c3ba9d0) = 12453
>> 
>>
>> I am sure this could be made more intelligent but lets keep it simple for
>> now.
>>
>> - I removed the helperPath() methods Roger mentioned, they are not needed
>> anymore.
>>
>> @Martin: I like the -e signal=none -e trace=process idea but I'm not sure
>> if all versions of strace support these options. I think the strace output
>> is small enough for this small use case, some kB only.
>>
>> Cheers, Thomas
>>
>>
>>
>>
>>
>> On Thu, Feb 7, 2019 at 6:01 PM Thomas Stüfe 
>> wrote:
>>
>>> Hi all,
>>>
>>> second version, including the updated comment in ProcessImpl.c Martin
>>> requested:
>>>
>>>
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.01/webrev/index.html
>>>
>>> @Roger: thanks for feeding this into your tests. I still try to get it
>>> to run thru jdk-submit, but that seems to be stuck again..
>>>
>>> Cheers, Thomas
>>>
>>>
>>>
>>>
>>>
>>> On Wed, Feb 6, 2019 at 10:29 AM Thomas Stüfe 
>>> wrote:
>>>
 Hi all

 Bug: https://bugs.openjdk.java.net/browse/JDK-8213192
 webrev:
 http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.00/webrev/index.html

 (@Roger: I hope you do not mind? The bug is assigned to you but since I
 happened to play around with posix_spawn I prepared this webrev. If you
 rather do this change, that is fine and I will leave it to you.)

 When we added the possibility to use posix_spawn as underlying
 implementation for Runtime.exec() on Linux with
 https://bugs.openjdk.java.net/browse/JDK-8212828, we agreed to keep
 VFORK as default until work on 13 starts. So now would be a good time to
 switch the default to posix_spawn to get a good testing window. Note that
 at SAP we run our VMs internally with posix_spawn as default since some
 months and have not seen problems.

 As for the fix, I added a test which tests that the default is indeed
 posix_spawn - not sure whether this is overdoing it though. Also, I use
 strace for the test, and /bin/true, and while strace is usually available
 and reachable by path resolution, I am afraid on some test machines it may
 not. What do you think, should I leave the test out?

 The fix ran through all java/lang/ProcessBuilder jtreg tests ok.

 Thanks, Thomas

Re: RFR(s): 8212828: (process) Change the Process launch mechanism default on Linux to be posix_spawn

[Thomas Stüfe]

Thank you Roger!

On Mon, Feb 11, 2019 at 7:55 PM Roger Riggs  wrote:

> Great!  Looks fine.
>
> Thanks, Roger
>
> On 02/11/2019 01:50 PM, Thomas Stüfe wrote:
>
> Hi Roger, Martin,
>
> hopefully final version:
>
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.03/webrev/
>
> I removed the test and the changes in the test library made for the test.
> Test is just too brittle with too little nourishing value. Everything else
> is unchanged from webrev.02.
>
> Thank you, Thomas
>
>
>
> On Fri, Feb 8, 2019 at 10:38 AM Thomas Stüfe 
> wrote:
>
>> Hi Roger, Martin,
>>
>> next version:
>>
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.02/webrev
>>
>> - did massage the comment in ProcessImpl.c
>> - made the test more resilient by scanning for the strace tool and by
>> silently ignoring all problems stemming from strace or the payload binary
>> not being there. The test now only fails if the forks were successully
>> executed but it does not seem to use posix-spawn.
>> - added output to the test by printing the "interesting" lines of the
>> strace output. Note that this filtering is not really sophisticated and
>> will show all thread related clone() calls as well:
>>
>> 614   [pid 12447] <... clone resumed> child_stack=0x7fe00c4baff0,
>> flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
>> parent_tidptr=0x7fe00c4bb9d0, tls=0x7fe00c4bb700,
>> child_tidptr=0x7fe00c4bb9d0) = 12452
>> 646   [pid 12447] clone(/usr/bin/strace: Process 12453 attached
>> 649   [pid 12447] <... clone resumed> child_stack=0x7fe00c3b9ff0,
>> flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
>> parent_tidptr=0x7fe00c3ba9d0, tls=0x7fe00c3ba700,
>> child_tidptr=0x7fe00c3ba9d0) = 12453
>> 
>>
>> I am sure this could be made more intelligent but lets keep it simple for
>> now.
>>
>> - I removed the helperPath() methods Roger mentioned, they are not needed
>> anymore.
>>
>> @Martin: I like the -e signal=none -e trace=process idea but I'm not sure
>> if all versions of strace support these options. I think the strace output
>> is small enough for this small use case, some kB only.
>>
>> Cheers, Thomas
>>
>>
>>
>>
>>
>> On Thu, Feb 7, 2019 at 6:01 PM Thomas Stüfe 
>> wrote:
>>
>>> Hi all,
>>>
>>> second version, including the updated comment in ProcessImpl.c Martin
>>> requested:
>>>
>>>
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.01/webrev/index.html
>>>
>>> @Roger: thanks for feeding this into your tests. I still try to get it
>>> to run thru jdk-submit, but that seems to be stuck again..
>>>
>>> Cheers, Thomas
>>>
>>>
>>>
>>>
>>>
>>> On Wed, Feb 6, 2019 at 10:29 AM Thomas Stüfe 
>>> wrote:
>>>
 Hi all

 Bug: https://bugs.openjdk.java.net/browse/JDK-8213192
 webrev:
 http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.00/webrev/index.html

 (@Roger: I hope you do not mind? The bug is assigned to you but since I
 happened to play around with posix_spawn I prepared this webrev. If you
 rather do this change, that is fine and I will leave it to you.)

 When we added the possibility to use posix_spawn as underlying
 implementation for Runtime.exec() on Linux with
 https://bugs.openjdk.java.net/browse/JDK-8212828, we agreed to keep
 VFORK as default until work on 13 starts. So now would be a good time to
 switch the default to posix_spawn to get a good testing window. Note that
 at SAP we run our VMs internally with posix_spawn as default since some
 months and have not seen problems.

 As for the fix, I added a test which tests that the default is indeed
 posix_spawn - not sure whether this is overdoing it though. Also, I use
 strace for the test, and /bin/true, and while strace is usually available
 and reachable by path resolution, I am afraid on some test machines it may
 not. What do you think, should I leave the test out?

 The fix ran through all java/lang/ProcessBuilder jtreg tests ok.

 Thanks, Thomas


>

Re: RFR(s): 8212828: (process) Change the Process launch mechanism default on Linux to be posix_spawn

[Roger Riggs]

Great!  Looks fine.

Thanks, Roger

On 02/11/2019 01:50 PM, Thomas Stüfe wrote:

Hi Roger, Martin,

hopefully final version:

http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.03/webrev/ 



I removed the test and the changes in the test library made for the 
test. Test is just too brittle with too little nourishing value. 
Everything else is unchanged from webrev.02.


Thank you, Thomas



On Fri, Feb 8, 2019 at 10:38 AM Thomas Stüfe > wrote:


Hi Roger, Martin,

next version:


http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.02/webrev



- did massage the comment in ProcessImpl.c
- made the test more resilient by scanning for the strace tool and
by silently ignoring all problems stemming from strace or the
payload binary not being there. The test now only fails if the
forks were successully executed but it does not seem to use
posix-spawn.
- added output to the test by printing the "interesting" lines of
the strace output. Note that this filtering is not really
sophisticated and will show all thread related clone() calls as well:

614   [pid 12447] <... clone resumed> child_stack=0x7fe00c4baff0,

flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
parent_tidptr=0x7fe00c4bb9d0, tls=0x7fe00c4bb700,
child_tidptr=0x7fe00c4bb9d0) = 12452
646   [pid 12447] clone(/usr/bin/strace: Process 12453 attached
649   [pid 12447] <... clone resumed> child_stack=0x7fe00c3b9ff0,

flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
parent_tidptr=0x7fe00c3ba9d0, tls=0x7fe00c3ba700,
child_tidptr=0x7fe00c3ba9d0) = 12453


I am sure this could be made more intelligent but lets keep it
simple for now.

- I removed the helperPath() methods Roger mentioned, they are not
needed anymore.

@Martin: I like the -e signal=none -e trace=process idea but I'm
not sure if all versions of strace support these options. I think
the strace output is small enough for this small use case, some kB
only.

Cheers, Thomas





On Thu, Feb 7, 2019 at 6:01 PM Thomas Stüfe
mailto:thomas.stu...@gmail.com>> wrote:

Hi all,

second version, including the updated comment in ProcessImpl.c
Martin requested:


http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.01/webrev/index.html



@Roger: thanks for feeding this into your tests. I still try
to get it to run thru jdk-submit, but that seems to be stuck
again..

Cheers, Thomas





On Wed, Feb 6, 2019 at 10:29 AM Thomas Stüfe
mailto:thomas.stu...@gmail.com>> wrote:

Hi all

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

http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.00/webrev/index.html



(@Roger: I hope you do not mind? The bug is assigned to
you but since I happened to play around with posix_spawn I
prepared this webrev. If you rather do this change, that
is fine and I will leave it to you.)

When we added the possibility to use posix_spawn as
underlying implementation for Runtime.exec() on Linux with
https://bugs.openjdk.java.net/browse/JDK-8212828, we
agreed to keep VFORK as default until work on 13 starts.
So now would be a good time to switch the default to
posix_spawn to get a good testing window. Note that at SAP
we run our VMs internally with posix_spawn as default
since some months and have not seen problems.

As for the fix, I added a test which tests that the
default is indeed posix_spawn - not sure whether this is
overdoing it though. 

Re: RFR(s): 8212828: (process) Change the Process launch mechanism default on Linux to be posix_spawn

[Thomas Stüfe]

Hi Roger, Martin,

hopefully final version:

http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.03/webrev/

I removed the test and the changes in the test library made for the test.
Test is just too brittle with too little nourishing value. Everything else
is unchanged from webrev.02.

Thank you, Thomas



On Fri, Feb 8, 2019 at 10:38 AM Thomas Stüfe 
wrote:

> Hi Roger, Martin,
>
> next version:
>
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.02/webrev
>
> - did massage the comment in ProcessImpl.c
> - made the test more resilient by scanning for the strace tool and by
> silently ignoring all problems stemming from strace or the payload binary
> not being there. The test now only fails if the forks were successully
> executed but it does not seem to use posix-spawn.
> - added output to the test by printing the "interesting" lines of the
> strace output. Note that this filtering is not really sophisticated and
> will show all thread related clone() calls as well:
>
> 614   [pid 12447] <... clone resumed> child_stack=0x7fe00c4baff0,
> flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
> parent_tidptr=0x7fe00c4bb9d0, tls=0x7fe00c4bb700,
> child_tidptr=0x7fe00c4bb9d0) = 12452
> 646   [pid 12447] clone(/usr/bin/strace: Process 12453 attached
> 649   [pid 12447] <... clone resumed> child_stack=0x7fe00c3b9ff0,
> flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
> parent_tidptr=0x7fe00c3ba9d0, tls=0x7fe00c3ba700,
> child_tidptr=0x7fe00c3ba9d0) = 12453
> 
>
> I am sure this could be made more intelligent but lets keep it simple for
> now.
>
> - I removed the helperPath() methods Roger mentioned, they are not needed
> anymore.
>
> @Martin: I like the -e signal=none -e trace=process idea but I'm not sure
> if all versions of strace support these options. I think the strace output
> is small enough for this small use case, some kB only.
>
> Cheers, Thomas
>
>
>
>
>
> On Thu, Feb 7, 2019 at 6:01 PM Thomas Stüfe 
> wrote:
>
>> Hi all,
>>
>> second version, including the updated comment in ProcessImpl.c Martin
>> requested:
>>
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.01/webrev/index.html
>>
>> @Roger: thanks for feeding this into your tests. I still try to get it to
>> run thru jdk-submit, but that seems to be stuck again..
>>
>> Cheers, Thomas
>>
>>
>>
>>
>>
>> On Wed, Feb 6, 2019 at 10:29 AM Thomas Stüfe 
>> wrote:
>>
>>> Hi all
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8213192
>>> webrev:
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.00/webrev/index.html
>>>
>>> (@Roger: I hope you do not mind? The bug is assigned to you but since I
>>> happened to play around with posix_spawn I prepared this webrev. If you
>>> rather do this change, that is fine and I will leave it to you.)
>>>
>>> When we added the possibility to use posix_spawn as underlying
>>> implementation for Runtime.exec() on Linux with
>>> https://bugs.openjdk.java.net/browse/JDK-8212828, we agreed to keep
>>> VFORK as default until work on 13 starts. So now would be a good time to
>>> switch the default to posix_spawn to get a good testing window. Note that
>>> at SAP we run our VMs internally with posix_spawn as default since some
>>> months and have not seen problems.
>>>
>>> As for the fix, I added a test which tests that the default is indeed
>>> posix_spawn - not sure whether this is overdoing it though. Also, I use
>>> strace for the test, and /bin/true, and while strace is usually available
>>> and reachable by path resolution, I am afraid on some test machines it may
>>> not. What do you think, should I leave the test out?
>>>
>>> The fix ran through all java/lang/ProcessBuilder jtreg tests ok.
>>>
>>> Thanks, Thomas
>>>
>>>

Re: RFR 8218228 : The constructor StringBuffer(CharSequence) violates spec for negatively sized argument

[Roger Riggs]

Hi Ivan,

I called it out because the CSR does not mention that the behavior
of some of the cases (-1..-16) is changing and some of the emails asserted
there was no change in behavior.

I'm fine with one changeset as long as both changes are explicit.
The bug and the CSR should both be updated.

Thanks, Roger


On 02/08/2019 05:12 PM, Ivan Gerasimov wrote:

Hi Roger!

That's because two tiny changes are combined in the patch:

1) remove a problematic statement from the javadoc, as it doesn't hold 
anyway.  This part does not change the behavior.


2) unify type of the exception thrown for all negative values of 
CharSequence.length().  The regression test is to verify this, second 
change, so it fails prior the fix for length in [-16, -1].


I can separate them into different bugs, if you think it will make it 
clearer.  Though I thought they can be fixed together.


With kind regards,

Ivan


On 2/8/19 1:49 PM, Roger Riggs wrote:

Hi Ivan,

In the direction of not changing the behavior; the webrev does change 
the behavior.


In the case of CharSeqence with length -1..-16, the new behavior 
throws NegativeArrayIndexException

instead of java.lang.IndexOutOfBoundsException.


AbstractStringBuilder:101-102 throw an exception for length < 0.
However, the current behavior is to create a StringBuffer with length 
+ 16 and
then append the contents.  For -1..-16, it will succeed in creating a 
StringBuffer
but fail with IndexOutOfBoundsException during the append of the 
contents.


The new Test should pass both before and after the change to the code.

Roger


On 02/07/2019 10:19 PM, Ivan Gerasimov wrote:



On 2/7/19 6:33 PM, David Holmes wrote:

On 8/02/2019 11:59 am, Ivan Gerasimov wrote:

Hi David!


On 2/7/19 5:16 PM, David Holmes wrote:

Hi Ivan,

On 8/02/2019 11:02 am, Ivan Gerasimov wrote:

Hello!

The specification states:
"""
If the length of the specified CharSequence is less than or 
equal to zero, then an empty buffer of capacity 16 is returned.

"""

However, the current implementation throws either 
NegativeArraySizeException or IndexOutOfBounds instead (the 
actual exception type depends on the value of reported negative 
length).


How can you have a negative length CharSequence ??


A custom CharSequence returning negative length() can be created.
Not sure how useful/popular this may be, though.


Seems pretty meaningless so just treating that as an error seems 
fine. Though somewhat debatable whether you need to add an 
appropriate @throws.



Right.  There were two reasons not to add @throws here:
- by removing the problematic paragraph we just make the javadoc 
almost the same as for StringBuilder(CharSequence),
- we don't seem to have any other places (at least I couldn't find 
any) specifying exceptions due to negatively sized CharSequence.


(To be precise, there is one candidate, but I'll file a separate bug 
to fix it.)


That's why I propose just removing the mentioning negative length, 
and not changing the behavior.


The proposed code change is to only unify the behavior for any 
negative value of length.


Ok.

If its an empty CharSequence then it should return the empty 
buffer as indicated.



Empty CharSequence is processed correctly already.


Okay so by removing this part:

-  * 
-  * If the length of the specified {@code CharSequence} is
-  * less than or equal to zero, then an empty buffer of capacity
-  * {@code 16} is returned.

you're relying on the main specification to implicitly handle the 
empty case.



Yes.

With kind regards,
Ivan



David
-


With kind regards,
Ivan


Cheers,
David


It is proposed to do two things:
1) remove the problematic sentence from the javadoc (CSR is filed).
2) unify the exception type thrown, if the argument reports 
negative length.
NegativeArraySizeException will be consistent with, for example, 
StringBuffer(negativeCapacity).


Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228
WEBREV: http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/
CRS: https://bugs.openjdk.java.net/browse/JDK-8218649

Re: RFR(s): 8212828: (process) Change the Process launch mechanism default on Linux to be posix_spawn

[Roger Riggs]

Hi Thomas,

I'd be fine with leaving it out.  It only provides confirmation of the 
library call,

nothing algorithmic or complex and there is no bug to verify.

So yes, drop it and save the test time and maintenance.

Thanks, Roger

On 02/09/2019 10:24 AM, Thomas Stüfe wrote:

Hi Roger,

I start getting doubts about the whole test, honestly. Do you think it 
is worth the maintenance effort to test that we indeed call 
posix_spawn as we intended to? I wonder whether I should just scratch it.


..Thomas



On Fri, Feb 8, 2019 at 10:00 PM Roger Riggs > wrote:


Hi Thomas,

The new OutputAnalyzer methods are new:

496: now that we have Immutable Lists, can the filterByKeyword
argument be a List?

List interesting = List.of( "fork", "clone", "vfork",
"exec", "/bin/true", "jspawnhelper" );

497: Instead of passing an int, pass the String itself
480, 485: that is returned from getStdout() or getStderr().

The body of the method could probably be nicely written using the
Stream API.
(except for the line numbers).  I'm not sure the line numbers are
useful in this case
based since the strace output is not shown anywhere or seen in the
full.

   .lines().filter()...

Add the javadoc to the methods so its clear how to use them since
this is a public test framework.

Regards, Roger


On 02/08/2019 04:38 AM, Thomas Stüfe wrote:

Hi Roger, Martin,

next version:


http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.02/webrev



- did massage the comment in ProcessImpl.c
- made the test more resilient by scanning for the strace tool
and by silently ignoring all problems stemming from strace or the
payload binary not being there. The test now only fails if the
forks were successully executed but it does not seem to use
posix-spawn.
- added output to the test by printing the "interesting" lines of
the strace output. Note that this filtering is not really
sophisticated and will show all thread related clone() calls as well:

614   [pid 12447] <... clone resumed> child_stack=0x7fe00c4baff0,

flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
parent_tidptr=0x7fe00c4bb9d0, tls=0x7fe00c4bb700,
child_tidptr=0x7fe00c4bb9d0) = 12452
646   [pid 12447] clone(/usr/bin/strace: Process 12453 attached
649   [pid 12447] <... clone resumed> child_stack=0x7fe00c3b9ff0,

flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
parent_tidptr=0x7fe00c3ba9d0, tls=0x7fe00c3ba700,
child_tidptr=0x7fe00c3ba9d0) = 12453


I am sure this could be made more intelligent but lets keep it
simple for now.

- I removed the helperPath() methods Roger mentioned, they are
not needed anymore.

@Martin: I like the -e signal=none -e trace=process idea but I'm
not sure if all versions of strace support these options. I think
the strace output is small enough for this small use case, some
kB only.

Cheers, Thomas





On Thu, Feb 7, 2019 at 6:01 PM Thomas Stüfe
mailto:thomas.stu...@gmail.com>> wrote:

Hi all,

second version, including the updated comment in
ProcessImpl.c Martin requested:


http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.01/webrev/index.html



@Roger: thanks for feeding this into your tests. I still try
to get it to run thru jdk-submit, but that seems to be stuck
again..

Cheers, Thomas





On Wed, Feb 6, 2019 at 10:29 AM Thomas Stüfe
mailto:thomas.stu...@gmail.com>> wrote:

Hi all

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

http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.00/webrev/index.html



(@Roger: I hope you do not mind? The bug is assigned to
you but since I happened to play around with posix_spawn
I prepared this webrev. If you rather do this change,
that is fine and I will 

Re: RFR: jsr166 integration 2019-02

[Chris Hegarty]

> On 9 Feb 2019, at 19:25, Martin Buchholz  wrote:
> 
> Thanks, Chris.
> 
> On Sat, Feb 9, 2019 at 6:34 AM Chris Hegarty  > wrote:
>> 8215359: InnocuousForkJoinWorkerThread.setContextClassLoader needlessly
>> throws
>> https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/InnocuousForkJoinWorkerThread/index.html
>>  
>> 
> 
> Looks good. A positive test for setContextClassLoader(null) could be
> added.
> 
> good point - let's improve the test:
> 

The updated test looks good.

-Chris.