Re: RFR: 8071585: Update JAX-WS RI integration to latest version (2.2.11-b150127.1410)
Hi Alan, Miran,
If I understood correctly we can fix gt; in the next bulk update (in
early March). If it's so, can I have an approval for this fix?
Thank you,
Aleksej
On 02/02/2015 01:46 PM, Miroslav Kos wrote:
On 29/01/15 16:06, Alan Bateman wrote:
On 29/01/2015 14:51, Aleksej Efimov wrote:
Hi,
Can I have a review for a bulk update of JAX-B/WS from upstream
projects -
webrev: http://cr.openjdk.java.net/~aefimov/8071585/webrev/
more details in JBS: https://bugs.openjdk.java.net/browse/JDK-8071585
There is a lot of changes (947 lines) but almost all of them are
minor (comments changes/(c) years)
Thank you,
Aleksej
I see several changes where is replaced by gt; in pre-formatted
text (looks like was changed to lt; at some point in the past).
Hi Alan, thanks for catching this, we'll fix that. Is it ok to fix it
in the next (in a month) bulk update?
Miran
This is probably a question for the upstream projects but I would
think pre{@code ..}/pre would make the source much easier to read.
-Alan.
Re: 8072909: TimSort fails with ArrayIndexOutOfBoundsException on arrays longer than 1073741824
Hi Lev, The fix looks fine. Did you consider the improvements suggested in the paper to reestablish the invariant? Roger On 2/11/2015 11:29 AM, Lev Priima wrote: Hi, Stack length increased previously by JDK-8011944 was insufficient for some cases. Please review and push: webrev: http://cr.openjdk.java.net/~lpriima/8072909/webrev.00/ issue: https://bugs.openjdk.java.net/browse/JDK-8072909
Re: RFR: 8071585: Update JAX-WS RI integration to latest version (2.2.11-b150127.1410)
On 11/02/2015 16:20, Aleksej Efimov wrote: Hi Alan, Miran, If I understood correctly we can fix gt; in the next bulk update (in early March). If it's so, can I have an approval for this fix? Thank you, Aleksej Yes, fixing at the next sync up is fine (I didn't realize my mail was holding this up, apologies!). -Alan
8072909: TimSort fails with ArrayIndexOutOfBoundsException on arrays longer than 1073741824
Hi, Stack length increased previously by JDK-8011944 was insufficient for some cases. Please review and push: webrev: http://cr.openjdk.java.net/~lpriima/8072909/webrev.00/ issue: https://bugs.openjdk.java.net/browse/JDK-8072909 -- Lev
Re: JEP 102 Process Updates revised API draft
Thanks for the suggestions and confirmation that Duration is not a misuse of that type for this purpose. Roger On 2/10/2015 2:38 AM, Stephen Colebourne wrote: On 9 February 2015 at 23:44, David M. Lloyd david.ll...@redhat.com wrote: ProcessHandle.Info provides a startTime and totalTime. But it seems odd and inconsistent that startTime is an Instant, yet totalTime is a raw long as opposed to the arguably more consistent Duration. Is there a reason you went with a raw long for this property? I think using Duration would be OK here, even though the CPU time was used up in a discontinuous manner. The returned value is, in essence, a sum of many smaller durations. I'd also suggest adjusting the two method names: startTime() - startInstant() totalTime() - totalCpuDuration() Those communicate the intent more clearly IMO. Stephen
Re: JEP 102 Process Updates revised API draft
Hi David, Thanks for the suggestion: Forcible process destruction is defined as the immediate termination of a process, whereas regular destruction allows a process to shut down cleanly. It is very OS and application specific as to how and if an application does cleanly exit but adding qualifications would make it less readable. Roger On 2/10/2015 8:17 AM, David M. Lloyd wrote: On 02/09/2015 07:52 PM, Roger Riggs wrote: On 2/9/15 6:44 PM, David M. Lloyd wrote: Also, as a general comment, there isn't really a good explanation as to what the difference is between a normal destroy and a forcible destroy. Given that you've added an isDestroyForcible() method, I think it might be a good idea to explain what it means when this method returns true. There must be some criteria in the implementation to return true here, so at the least, that criteria should be explained. Also the destroy() method now has the odd characteristic that its implementation *may* forcibly destroy a process, but you can't really determine that from the API at all. From an implementation perspective, for Unix it is the distinction between SIGTERM and SIGKILL;one is allowed/expected to be caught and handled by the application for a clean shutdown,the other is not interceptable. But the OS variations and caveats make it hard to write anything more than an informative statement. Understood, but I'm thinking that such a statement should be added; something along the lines of Forcible process destruction is defined as the immediate termination of a process, whereas regular destruction allows a process to shut down cleanly. This gives a clear criterion as to what it means when isDestroyForcible returns true, since each of these behaviors (at least on Unix) are readily identified with SIGKILL and SIGTERM. Upon rereading the API I see that isDestroyForcible() actually reflects the behavior of destroy(), which is opposite of my original reading of it, and that clarifies things a bit. But there is still no way in the API to know if forcible process termination is supported; can it be assumed that it is supported on all platforms? The descriptions are copied from Process, which previously did not offer an explanation.
Re: RFR 8071479: Stream and lamdification improvements to j.u.regex.Matcher
On 2/11/15 2:25 AM, Paul Sandoz wrote: Hi Stuart, Thanks for the detailed review. Here is a possible way forward: 1) Add the methods to Matcher, as proposed in the initial webrev. Yes, I think this is the way to go, and it seems that Sherman concurs. 1.1) Change the specification of Matcher.results to reset the stream before matching, making it consistent with the replace* methods. I'm not sure about this. The current replaceAll/replaceFirst methods reset the matcher before doing any matching, so the lambda-based overloads should do the same. However, the model for StreamMatchResult results() seems to me to be a stream of matches that would be returned by successive calls to find(). (Indeed, that's how it's implemented.) The no-arg find() call doesn't reset the Matcher, and it respects the existing region of the Matcher. I think results() should do the same. Now there's also a find(int start) overload that does reset the matcher (discarding the region) but starts at the given position. This suggests another overload, StreamMatchResult results(int start) which also resets the matcher. I don't think this is necessary, though, since I believe the equivalent effect can be achieved by setting the region before calling the no-arg results() -- as long as it doesn't reset the matcher. No matter what, I think results() will have to check for concurrent modification. It seems to be in the nature of this API, sigh. By the way, I think Matcher.results() is a fine name. The overload of Pattern.matches() is what bothers me. 2) Add convenience methods for all replace*() and matches() on Pattern that defer to those on Matcher. I'm not sure convenience methods are necessary. After all, there are the existing replaceFirst/replaceAll methods on Matcher that aren't on Pattern. In addition, if you don't need to hang onto the Pattern or Matcher instances, my example from earlier can be compressed to: String result = Pattern.compile(a*b) .matcher(input).replaceAll(mr - mr.group().toUpperCase()); which ain't too bad. We can do that in two stages, focusing on 1) in this review. Yes. I was not too concerned about the static method and the stream returning method having the same name as the context is quite different. For stream returning methods there is a de-facto pattern of using a plural of the stream element kind, so i prefer that to findAll. What about the name Pattern.matchResults? which chains well with Pattern.match(...).results(). -- Regarding the disparity between MatchResult and Matcher. I think that would require a new sub-interface of MatchResult from which Matcher extends from and returns. If we think it important we should do that for 9 otherwise we will be stuck for the stream-based methods. Why do you think we need a new sub-interface? Wouldn't it be sufficient to add default methods? There is of course the possibility of existing 3rd party classes that implement MatchResult having conflicting methods. But I don't think MatchResult is implemented quite as often as the collection interfaces, where we've been reticent to add default methods because of so many implementations. I did a quick web search and I did find a few implementations/extensions of MatchResult, but there were no obvious conflicts. This isn't definitive, of course. The default implementations of group(String), start(String), and end(String) could throw IllegalArgumentException, since that's what the ones on Matcher do if there's no such named capture group. I admit this is a little weird, since it's only safe to use these new methods if you know where the MatchResult instance came from. So logically perhaps it's a different type. My hunch, though, is that MatchResults aren't passed around, they're created from Patterns/Matchers and processed within the same code, so in practice this won't be a problem. s'marks Paul. On Feb 11, 2015, at 2:02 AM, Stuart Marks stuart.ma...@oracle.com wrote: Hi Paul, I spent some time looking at this API. Overall it seems to me that things work a bit more nicely when these methods are added to Pattern instead of Matcher. Unfortunately there are some odd things with the existing API that make this tradeoff not so obvious. First, here's what a simple replacement operation looks like when replaceAll() is added to Matcher: String input = foobbfooabbbfoo; Pattern p = Pattern.compile(a*b); Matcher m = p.matcher(input); String result = m.replaceAll(mr - mr.group().toUpperCase()); But if replaceAll() is on Pattern, we can skip a step: String input = foobbfooabbbfoo; Pattern p = Pattern.compile(a*b); String result = p.replaceAll(input, mr - mr.group().toUpperCase()); Getting stream of match results is similar. So yes, I agree that it simplifies things to have these be on Pattern instead of Matcher. An advantage of having these on Pattern is
Re: 8072909: TimSort fails with ArrayIndexOutOfBoundsException on arrays longer than 1073741824
Just briefly looked at it, w/o evaluating formal proof. But original Python implementation(written on C) works on stack size even more simple, AFAIU it: /* The maximum number of entries in a MergeState's pending-runs stack. * This is enough to sort arrays of size up to about * 32 * phi ** MAX_MERGE_PENDING * where phi ~= 1.618. 85 is ridiculouslylarge enough, good for an array * with 2**64 elements. */ #define MAX_MERGE_PENDING 85 Lev On 02/11/2015 08:32 PM, Roger Riggs wrote: Hi Lev, The fix looks fine. Did you consider the improvements suggested in the paper to reestablish the invariant? Roger On 2/11/2015 11:29 AM, Lev Priima wrote: Hi, Stack length increased previously by JDK-8011944 was insufficient for some cases. Please review and push: webrev: http://cr.openjdk.java.net/~lpriima/8072909/webrev.00/ issue: https://bugs.openjdk.java.net/browse/JDK-8072909
Re: [9] 8064562: (doc) errors in java.io.PushbackInputStream API documentation
Hi Brian, Looks good. Thanks, Roger On 2/10/2015 7:06 PM, Brian Burkhalter wrote: Please review at your convenience. Issue: https://bugs.openjdk.java.net/browse/JDK-8064562 Patch: http://cr.openjdk.java.net/~bpb/8064562/webrev.00/ Thanks, Brian
Re: [9] 8064562: (doc) errors in java.io.PushbackInputStream API documentation
Thanks, Lance, Indeed, I was merely the amanuensis, or more precisely, the scribe. ;-) Cheers, Brian On Feb 11, 2015, at 7:08 AM, Lance Andersen lance.ander...@oracle.com wrote: looks fine brian, the bug did a good job of explaining the issues.
Re: [9] 8064562: (doc) errors in java.io.PushbackInputStream API documentation
Thanks, Roger. On Feb 11, 2015, at 7:03 AM, Roger Riggs roger.ri...@oracle.com wrote: Hi Brian, Looks good. Thanks, Roger On 2/10/2015 7:06 PM, Brian Burkhalter wrote: Please review at your convenience. Issue: https://bugs.openjdk.java.net/browse/JDK-8064562 Patch: http://cr.openjdk.java.net/~bpb/8064562/webrev.00/ Thanks, Brian
Re: RFR (xs): JDK-8072611: (process) ProcessBuilder redirecting output to file should work with long file names (win)
Thank you Volker :) On Wed, Feb 11, 2015 at 2:36 PM, Volker Simonis volker.simo...@gmail.com wrote: Hi Thomas, I've just notices that your test has no copyright info and is indented with an indent width of 2 spaces instead of 4. If you don't mind I'll fix this before pushing so there's no need for a new webrev. Regards, Volker On Tue, Feb 10, 2015 at 10:58 AM, Thomas Stüfe thomas.stu...@gmail.com wrote: Hi Roger, Volker, thanks for reviewing! I added all requested changes: @Roger - use now Files.createTempDirectory to get a unique directory - wrapped test in try/finally to cleanup afterwards @Volker - moved include up and added dependency to comment in io_util_md.h - Now I use hostname.exe, which I hope is part of every Windows :) http://cr.openjdk.java.net/~stuefe/webrevs/8072611/webrev.02/webrev/ Regards, Thomas On Mon, Feb 9, 2015 at 7:48 PM, Volker Simonis volker.simo...@gmail.com wrote: Hi Thomas, the change looks good and I can sponsor it once it is reviewed. Just some small notes: - it seems that getPath() isn't used anywhere else in ProcessImpl_md.c and because it is static it can't be used anywhere else. So can you please remove it completely. - io_util_md.h has a list of files which use the prototypes like pathToNTPath before the declaration of the prototypes. Could you please also add ProcessImpl_md.c to this list - can you pleae place the new include in ProcessImpl_md.c as follows: #include io_util.h +#include io_util_md.h #include windows.h #include io.h I saw that system and local headers are already intermixed in that file but at I think at least we shouldn't introduce more disorder. - is robocopy really available on all supported Windows system. In http://en.wikipedia.org/wiki/Robocopy I read that it was introduced in Server 2008. I just verified that Java 8 only supports Server 2008 and above but just think of our internal test system:) Maybe we can use a program which is supported in every Windows version? Regards, Volker On Mon, Feb 9, 2015 at 2:30 PM, Thomas Stüfe thomas.stu...@gmail.com wrote: Hi all, please review this small change at your convenience: http://cr.openjdk.java.net/~stuefe/webrevs/8072611/webrev.01/webrev/ It fixes a small issue which causes ProcessBuilder not to be able to open files to redirect into (when using Redirect.append) on windows, if that file name is longer than 255 chars. Kind Regards, Thomas Stuefe
Re: [9] 8064562: (doc) errors in java.io.PushbackInputStream API documentation
looks fine brian, the bug did a good job of explaining the issues. Best Lance On Feb 10, 2015, at 7:06 PM, Brian Burkhalter brian.burkhal...@oracle.com wrote: Please review at your convenience. Issue:https://bugs.openjdk.java.net/browse/JDK-8064562 Patch:http://cr.openjdk.java.net/~bpb/8064562/webrev.00/ Thanks, Brian Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: RFR 8071479: Stream and lamdification improvements to j.u.regex.Matcher
Hi It might be more consistent with the existing design to add those methods into Matcher. I agree the better name for the stream return method is findAll. -sherman On 2/11/15 2:25 AM, Paul Sandoz wrote: Hi Stuart, Thanks for the detailed review. Here is a possible way forward: 1) Add the methods to Matcher, as proposed in the initial webrev. 1.1) Change the specification of Matcher.results to reset the stream before matching, making it consistent with the replace* methods. 2) Add convenience methods for all replace*() and matches() on Pattern that defer to those on Matcher. We can do that in two stages, focusing on 1) in this review. I was not too concerned about the static method and the stream returning method having the same name as the context is quite different. For stream returning methods there is a de-facto pattern of using a plural of the stream element kind, so i prefer that to findAll. What about the name Pattern.matchResults? which chains well with Pattern.match(...).results(). -- Regarding the disparity between MatchResult and Matcher. I think that would require a new sub-interface of MatchResult from which Matcher extends from and returns. If we think it important we should do that for 9 otherwise we will be stuck for the stream-based methods. Paul. On Feb 11, 2015, at 2:02 AM, Stuart Marks stuart.ma...@oracle.com wrote: Hi Paul, I spent some time looking at this API. Overall it seems to me that things work a bit more nicely when these methods are added to Pattern instead of Matcher. Unfortunately there are some odd things with the existing API that make this tradeoff not so obvious. First, here's what a simple replacement operation looks like when replaceAll() is added to Matcher: String input = foobbfooabbbfoo; Pattern p = Pattern.compile(a*b); Matcher m = p.matcher(input); String result = m.replaceAll(mr - mr.group().toUpperCase()); But if replaceAll() is on Pattern, we can skip a step: String input = foobbfooabbbfoo; Pattern p = Pattern.compile(a*b); String result = p.replaceAll(input, mr - mr.group().toUpperCase()); Getting stream of match results is similar. So yes, I agree that it simplifies things to have these be on Pattern instead of Matcher. An advantage of having these on Pattern is that the matcher that gets created is encapsulated, and its state isn't exposed to being mucked about by the application. Thus you can avoid the additional concurrent modification checks that you have to do if replaceAll et. al. are on Matcher. Unfortunately, putting these on Pattern now creates some difficulties meshing with the existing API. One issue is that Matcher already has replaceAll(String) and replaceFirst(String). It would be strange to have these here and to have replaceAll(replacer) and replaceFirst(replacer) on Pattern. Another issue is that Matcher supports matching on region (subrange) of its input. For example, today you can do this: pattern.matcher(input).region(start, end) The region will constrain the matching for certain operations, such as find() (but not replaceAll or replaceFirst). If something like results() were added to Matcher, I'd expect that it would respect the Matcher's region, but if results() (or matches() as you called it) were on Pattern, the region constraint would be lacking. Also note that Pattern already has this: static boolean matches(regex, input) so I don't think an overload of matches() that returns a Stream would be a good idea. (Maybe findAll()?) Another issue, not directly related to where the new lambda/streams methods get added, is that MatchResult allows references only numbered capturing groups. Matcher, which implements MatchResult, also supports named capturing groups, with the new overloaded methods group(String), start(String), and end(String). These were added in Java 7. Logically these also belong on MatchResult, but they probably weren't added because of the compatibility issue of adding methods to interfaces. Maybe we should add these as default methods to MatchResult. (But what would the supported implementation be? Just throw UnsupportedOperationException?) -- I'm not entirely sure where this leaves things. It certainly seems more convenient to have the new methods on Pattern. But given the way the existing API is set up, it seems like it's a better fit to add them to Matcher. s'marks On 2/9/15 6:18 AM, Paul Sandoz wrote: Here is an alternative that pushes the methods on to Pattern instead: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071479--Matcher-stream-results/on-Pattern/webrev/ (Whe webrev reports some files as empty, please ingore those, i have this webrev stacked on the previous one.) I have also included replaceFirst. This simplifies things for streaming on the match results and also for replacing. Note that the existing replace* methods on Matcher reset the matcher
Re: RFR 8071479: Stream and lamdification improvements to j.u.regex.Matcher
On Feb 11, 2015, at 8:23 PM, Stuart Marks stuart.ma...@oracle.com wrote: On 2/11/15 2:25 AM, Paul Sandoz wrote: Hi Stuart, Thanks for the detailed review. Here is a possible way forward: 1) Add the methods to Matcher, as proposed in the initial webrev. Yes, I think this is the way to go, and it seems that Sherman concurs. 1.1) Change the specification of Matcher.results to reset the stream before matching, making it consistent with the replace* methods. I'm not sure about this. The current replaceAll/replaceFirst methods reset the matcher before doing any matching, so the lambda-based overloads should do the same. Yes, we are in agreement on that. However, the model for StreamMatchResult results() seems to me to be a stream of matches that would be returned by successive calls to find(). (Indeed, that's how it's implemented.) The no-arg find() call doesn't reset the Matcher, It cannot, otherwise it would not work for repeated calls to obtain subsequent matches. and it respects the existing region of the Matcher. I think results() should do the same. That matches my original thinking on the matter and is reflected in the patch. It's very simple to support. If the method was named findAll then it would be misleading and imply a reset was needed. Now there's also a find(int start) overload that does reset the matcher (discarding the region) but starts at the given position. This suggests another overload, StreamMatchResult results(int start) which also resets the matcher. I don't think this is necessary, though, since I believe the equivalent effect can be achieved by setting the region before calling the no-arg results() -- as long as it doesn't reset the matcher. Yes. No matter what, I think results() will have to check for concurrent modification. Yes, as in the patch. It seems to be in the nature of this API, sigh. By the way, I think Matcher.results() is a fine name. The overload of Pattern.matches() is what bothers me. Ok. 2) Add convenience methods for all replace*() and matches() on Pattern that defer to those on Matcher. I'm not sure convenience methods are necessary. After all, there are the existing replaceFirst/replaceAll methods on Matcher that aren't on Pattern. In addition, if you don't need to hang onto the Pattern or Matcher instances, my example from earlier can be compressed to: String result = Pattern.compile(a*b) .matcher(input).replaceAll(mr - mr.group().toUpperCase()); which ain't too bad. Agreed. We can do that in two stages, focusing on 1) in this review. Yes. Ok, the first webrev i sent is already implemented as agreed so lets review that code. I was not too concerned about the static method and the stream returning method having the same name as the context is quite different. For stream returning methods there is a de-facto pattern of using a plural of the stream element kind, so i prefer that to findAll. What about the name Pattern.matchResults? which chains well with Pattern.match(...).results(). -- Regarding the disparity between MatchResult and Matcher. I think that would require a new sub-interface of MatchResult from which Matcher extends from and returns. If we think it important we should do that for 9 otherwise we will be stuck for the stream-based methods. Why do you think we need a new sub-interface? Wouldn't it be sufficient to add default methods? What would the defaults do? Throwing an exception seems a poor solution to me. My guess it will be possible to evolve Matcher in binary and source compatible way to use the sub-type. Paul. There is of course the possibility of existing 3rd party classes that implement MatchResult having conflicting methods. But I don't think MatchResult is implemented quite as often as the collection interfaces, where we've been reticent to add default methods because of so many implementations. I did a quick web search and I did find a few implementations/extensions of MatchResult, but there were no obvious conflicts. This isn't definitive, of course. The default implementations of group(String), start(String), and end(String) could throw IllegalArgumentException, since that's what the ones on Matcher do if there's no such named capture group. I admit this is a little weird, since it's only safe to use these new methods if you know where the MatchResult instance came from. So logically perhaps it's a different type. My hunch, though, is that MatchResults aren't passed around, they're created from Patterns/Matchers and processed within the same code, so in practice this won't be a problem. s'marks
Re: RFR 9 8055330: (process spec) ProcessBuilder.start and Runtime.exec should throw UnsupportedOperationException ...
Hi Martin, I'm still looking for additional support for using IOException instead of UnsupportedOperationException; the prevailing view is that UOE is appropriate. The usual definition of UnsupportedOperationException seem to fit this case well. The behavior of the methods is not supported by the implementation; it is unconditional, it is not a question of OS policy or Java runtime policy. The examples cited avoid using Process if the OS dependent programs are not available and in those cases the UOE would not be encountered so this should not raise issues for existing programs. Roger On 2/3/15 2:30 PM, Martin Buchholz wrote: I agree that we should not be throwing SecurityException. We should be throwing IOException. But given a choice between SecurityException and UnsupportedOperationException, I would regard the former as the lesser of two evils. On Tue, Feb 3, 2015 at 12:40 AM, Alan Bateman alan.bate...@oracle.com mailto:alan.bate...@oracle.com wrote: On 02/02/2015 21:06, Martin Buchholz wrote: : The historic spec allows for both SecurityException and IOException (but not UOE). Locked-down systems typically (but not necessarily) have a SecurityManager that helps enforce the restrictions and so SecurityException seems vaguely appropriate. There are a small number of places where SecurityException is thrown when not running with a security manager (defineClass, package sealing) but it can be confusing for developers to get this exception when there isn't a Java security manager. So I don't think we should be using it here. -Alan
Re: RFR 8071600: Add a flat-mapping collector
On 2/11/15 1:54 AM, Paul Sandoz wrote:
On Feb 11, 2015, at 12:02 AM, Stuart Marks stuart.ma...@oracle.com wrote:
Hi Paul,
On 2/3/15 5:48 AM, Paul Sandoz wrote:
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071600-Collector-flatMapping/webrev/
This patch adds a new flat mapping collector to Collectors. This can be useful
if one needs to map 0 or more items into a downstream collector.
Mostly pretty good, just a couple minor nits.
Re the comment at TabulatorsTest.java line 513, this isn't a two-level groupBy
anymore, it's a groupByWithMapping (as the test name indicates).
I removed the comment. I also added the bug id to the test. Updated in place:
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071600-Collector-flatMapping-test-rename/webrev/
Thanks for the updates. I'm not sure exactly what this webrev represents now,
but the only change I requested was to change the comment, so no big deal.
Given the new tests of closing in FlatMapOpTest.java, is it necessary to have
testFlatMappingClose() in TabulatorsTest.java?
Yes, they are testing different code paths.
Thanks for the explanation.
s'marks
TabulatorsTest.testFlatMappingClose tests the closing functionality of
Collectors.flatMapping:
public static T, U, A, R
CollectorT, ?, R flatMapping(Function? super T, ? extends Stream? extends
U mapper,
Collector? super U, A, R downstream) {
BiConsumerA, ? super U downstreamAccumulator = downstream.accumulator();
return new CollectorImpl(downstream.supplier(),
(r, t) - {
try (Stream? extends U result = mapper.apply(t))
{
if (result != null)
result.sequential().forEach(u -
downstreamAccumulator.accept(r, u));
}
},
downstream.combiner(), downstream.finisher(),
downstream.characteristics());
}
Where as the tests of closing in FlatMapOpTest test the closing of
Stream.flatMap*:
@Override
public final R StreamR flatMap(Function? super P_OUT, ? extends Stream? extends
R mapper) {
Objects.requireNonNull(mapper);
// We can do better than this, by polling cancellationRequested when
stream is infinite
return new StatelessOpP_OUT, R(this, StreamShape.REFERENCE,
StreamOpFlag.NOT_SORTED |
StreamOpFlag.NOT_DISTINCT | StreamOpFlag.NOT_SIZED) {
@Override
SinkP_OUT opWrapSink(int flags, SinkR sink) {
return new Sink.ChainedReferenceP_OUT, R(sink) {
@Override
public void begin(long size) {
downstream.begin(-1);
}
@Override
public void accept(P_OUT u) {
try (Stream? extends R result = mapper.apply(u)) {
// We can do better that this too; optimize for
depth=0 case and just grab spliterator and forEach it
if (result != null)
result.sequential().forEach(downstream);
}
}
};
}
};
}
A following patch, which i plan to fold into the above patch, performs some
renames on the collectors test to be consistent with the current naming:
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071600-Collector-flatMapping-test-rename/webrev/
Renaming looks good and makes it easy to correlate the tests, the assertion classes, and the
collector implementations under test. Unfortunately, the word for the act of collecting is
collection which is confusing since collection already has another meaning,
but oh well.
Alas :-)
Thanks,
Paul.
Re: 8072909: TimSort fails with ArrayIndexOutOfBoundsException on arrays longer than 1073741824
On 12/02/2015 5:14 AM, Lev Priima wrote: Just briefly looked at it, w/o evaluating formal proof. But original Python implementation(written on C) works on stack size even more simple, AFAIU it: /* The maximum number of entries in a MergeState's pending-runs stack. * This is enough to sort arrays of size up to about * 32 * phi ** MAX_MERGE_PENDING * where phi ~= 1.618. 85 is ridiculouslylarge enough, good for an array * with 2**64 elements. */ #define MAX_MERGE_PENDING 85 So where did the new magic number 49 come from? And how do we know this is now big enough? Thanks, David Lev On 02/11/2015 08:32 PM, Roger Riggs wrote: Hi Lev, The fix looks fine. Did you consider the improvements suggested in the paper to reestablish the invariant? Roger On 2/11/2015 11:29 AM, Lev Priima wrote: Hi, Stack length increased previously by JDK-8011944 was insufficient for some cases. Please review and push: webrev: http://cr.openjdk.java.net/~lpriima/8072909/webrev.00/ issue: https://bugs.openjdk.java.net/browse/JDK-8072909
Re: 8072909: TimSort fails with ArrayIndexOutOfBoundsException on arrays longer than 1073741824
Am 11.02.2015 um 23:57 schrieb David Holmes: So where did the new magic number 49 come from? And how do we know this is now big enough? Thanks, David +1 Ulf
[9] RFC JDK-8068373: (prefs) FileSystemPreferences writes \0 to XML storage, causing loss of all preferences
This is a request for comment only, at this point. There is no associated formal test but there is one in the issue description. Issue: https://bugs.openjdk.java.net/browse/JDK-8068373 Patch: http://cr.openjdk.java.net/~bpb/8068373/webrev.02/ I mostly want to know how out to lunch this approach is, for the moment only in the context of FileSystemPreferences, not exporting/importing to/from XML in general from generic Preferences. Two things should be noted here. The Preferences (and Properties) can be exported to and imported from an XML format on all platforms. In the case of Unix (Linux and Solaris, I believe) this is also the format used to store the Preferences whereas on other platforms that is not the case. Therefore for these Unix cases there is the possibility of loss if things are not losslessly round trip encoded into XML. The present issue is one such case with dire consequences. The main problems as I see them are 1) how to maintain compatibility across Java versions, and 2) how to preserve what can be added as a Preference. With respect to the latter item, this should not be constrained because the datum happens to be cached in XML. This particular solution is data-preserving with respect to the issue addressed if the same or later version of a JVM is used, and does not cause older Java versions reading the XML cache to fail. Thanks, Brian
Re: RFR 8071479: Stream and lamdification improvements to j.u.regex.Matcher
On 2/11/15 12:45 PM, Paul Sandoz wrote: On Feb 11, 2015, at 8:23 PM, Stuart Marks stuart.ma...@oracle.com wrote: That matches my original thinking on the matter and is reflected in the patch. It's very simple to support. If the method was named findAll then it would be misleading and imply a reset was needed. OK, good, I see that, so it doesn't need to be changed. Ok, the first webrev i sent is already implemented as agreed so lets review that code. I looked at: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071479--Matcher-stream-results/webrev/ I think this was before you added Pattern.replaceFirst(), so that should be retrofitted here. Regarding the disparity between MatchResult and Matcher. I think that would require a new sub-interface of MatchResult from which Matcher extends from and returns. If we think it important we should do that for 9 otherwise we will be stuck for the stream-based methods. Why do you think we need a new sub-interface? Wouldn't it be sufficient to add default methods? What would the defaults do? Throwing an exception seems a poor solution to me. My guess it will be possible to evolve Matcher in binary and source compatible way to use the sub-type. OK, I filed an RFE to cover this, then immediately closed it as a duplicate :-) because I failed to search for the existing RFE that covers this: https://bugs.openjdk.java.net/browse/JDK-8065554 Either we add some default methods that throw exceptions (gross) or we add a sub-interface (also gross). It might be a matter of taste, or bike shed painting. Either way, I think this needs to be done. s'marks Paul. There is of course the possibility of existing 3rd party classes that implement MatchResult having conflicting methods. But I don't think MatchResult is implemented quite as often as the collection interfaces, where we've been reticent to add default methods because of so many implementations. I did a quick web search and I did find a few implementations/extensions of MatchResult, but there were no obvious conflicts. This isn't definitive, of course. The default implementations of group(String), start(String), and end(String) could throw IllegalArgumentException, since that's what the ones on Matcher do if there's no such named capture group. I admit this is a little weird, since it's only safe to use these new methods if you know where the MatchResult instance came from. So logically perhaps it's a different type. My hunch, though, is that MatchResults aren't passed around, they're created from Patterns/Matchers and processed within the same code, so in practice this won't be a problem. s'marks
Re: [9] RFR of 8066842: java.math.BigDecimal.divide(BigDecimal, RoundingMode) produces incorrect result
Hi Brian, Looks fine; just adjust the copyright dates before pushing. Thanks, -Joe On 2/6/2015 12:29 PM, Brian Burkhalter wrote: On Feb 6, 2015, at 7:46 AM, Brian Burkhalter brian.burkhal...@oracle.com wrote: On Feb 6, 2015, at 12:51 AM, Paul Sandoz paul.san...@oracle.com wrote: +1, but you should remove the @throws IAE on divRemNegativeLong before you push. Mea culpa. Thanks for pointing out the (failure of) omission. Will remove before pushing. FYI here is the updated webrev: http://cr.openjdk.java.net/~bpb/8066842/webrev.02/ Brian
Re: [9] RFR of 8066842: java.math.BigDecimal.divide(BigDecimal, RoundingMode) produces incorrect result
Hi Joe, All right; will do. Thanks, Brian On Feb 11, 2015, at 5:08 PM, Joseph D. Darcy joe.da...@oracle.com wrote: Hi Brian, Looks fine; just adjust the copyright dates before pushing. Thanks, -Joe
Re: List.indexOf and List.lastIndexOf lambda version.
The EG discussed these during Lambda and just couldn’t get excited about them. On Feb 11, 2015, at 5:02 AM, Andrey Nazarov andrey.x.naza...@oracle.com wrote: Hi everyone, I there any reason why java.util.List doesn't have lambda versions of indexOf and lastIndexOf methods like list.indexOf(Predicate) ? --Thanks, Andrey
Re: RFR 9 8055330: (process spec) ProcessBuilder.start and Runtime.exec should throw UnsupportedOperationException ...
Roger et al, Whichever way we go doesn't matter much. But I continue to think that IOE is a better choice than UOE and I have trouble seeing the motivation for the change to use UOE. If you wanted to provide a way to tell if subprocess support was available at all, then it would be better to add a new static boolean method to Process (but I wouldn't support that either). But (Roger and Alan): feel free to outvote me. On Wed, Feb 11, 2015 at 12:24 PM, Roger Riggs roger.ri...@oracle.com wrote: Hi Martin, I'm still looking for additional support for using IOException instead of UnsupportedOperationException; the prevailing view is that UOE is appropriate. The usual definition of UnsupportedOperationException seem to fit this case well. The behavior of the methods is not supported by the implementation; it is unconditional, it is not a question of OS policy or Java runtime policy. The examples cited avoid using Process if the OS dependent programs are not available and in those cases the UOE would not be encountered so this should not raise issues for existing programs. Roger On 2/3/15 2:30 PM, Martin Buchholz wrote: I agree that we should not be throwing SecurityException. We should be throwing IOException. But given a choice between SecurityException and UnsupportedOperationException, I would regard the former as the lesser of two evils. On Tue, Feb 3, 2015 at 12:40 AM, Alan Bateman alan.bate...@oracle.com mailto:alan.bate...@oracle.com wrote: On 02/02/2015 21:06, Martin Buchholz wrote: : The historic spec allows for both SecurityException and IOException (but not UOE). Locked-down systems typically (but not necessarily) have a SecurityManager that helps enforce the restrictions and so SecurityException seems vaguely appropriate. There are a small number of places where SecurityException is thrown when not running with a security manager (defineClass, package sealing) but it can be confusing for developers to get this exception when there isn't a Java security manager. So I don't think we should be using it here. -Alan
Re: 8072909: TimSort fails with ArrayIndexOutOfBoundsException on arrays longer than 1073741824
49 is also mentioned in the paper as possible solution. I've run test from webrev with array length 2147483644 (Integer.MAX_VALUE-4) and TimSort passed. Lev On 02/11/2015 10:57 PM, David Holmes wrote: On 12/02/2015 5:14 AM, Lev Priima wrote: Just briefly looked at it, w/o evaluating formal proof. But original Python implementation(written on C) works on stack size even more simple, AFAIU it: /* The maximum number of entries in a MergeState's pending-runs stack. * This is enough to sort arrays of size up to about * 32 * phi ** MAX_MERGE_PENDING * where phi ~= 1.618. 85 is ridiculouslylarge enough, good for an array * with 2**64 elements. */ #define MAX_MERGE_PENDING 85 So where did the new magic number 49 come from? And how do we know this is now big enough? Thanks, David Lev On 02/11/2015 08:32 PM, Roger Riggs wrote: Hi Lev, The fix looks fine. Did you consider the improvements suggested in the paper to reestablish the invariant? Roger On 2/11/2015 11:29 AM, Lev Priima wrote: Hi, Stack length increased previously by JDK-8011944 was insufficient for some cases. Please review and push: webrev: http://cr.openjdk.java.net/~lpriima/8072909/webrev.00/ issue: https://bugs.openjdk.java.net/browse/JDK-8072909
List.indexOf and List.lastIndexOf lambda version.
Hi everyone, I there any reason why java.util.List doesn't have lambda versions of indexOf and lastIndexOf methods like list.indexOf(Predicate) ? --Thanks, Andrey
Re: Unsafe.park/unpark, j.u.c.LockSupport and Thread
Paul,
I appreciate the issue with discontinuing use of Unsafe but this churn
to the public APIs seems unwarranted. Would we have done it differently
from day one? Sure. Does that mean we should change it all now? Not
without a very good reason. And I'm on the fence there.
The fact that LockSupport needs access to Thread internals is something
that modules could actually resolve :) - though I'm not up to date on
the interaction of module access and package access.
Let's see what Doug thinks of all this too.
Cheers,
David
On 11/02/2015 6:52 PM, Paul Sandoz wrote:
On Feb 11, 2015, at 2:43 AM, David Holmes david.hol...@oracle.com wrote:
Adding Doug Lea.
On 11/02/2015 12:51 AM, Paul Sandoz wrote:
On Feb 10, 2015, at 3:39 PM, Chris Hegarty chris.hega...@oracle.com wrote:
Adding native methods to 166 classes will introduce another form of dependency
on the platform that i would prefer to avoid.
Right. And I suspect that the separate distributable of 166, outside of the
Java Platform, would not want to have to carry a separate platform specific
native library.
Yes.
Right and for that reason jsr166 distribution would not want to have to know
whether to use Thread or Unsafe.
And that's why it should just be the former for the Java 9 platform and beyond.
The general strategy is to reduce dependency on Unsafe, including for 166
classes. The VarHandles effort is part of that strategy to replace Unsafe
enhanced access with a public alternative that is safe and just as performant.
But I don't see any reason why we couldn't move the implementation from unsafe.cpp to
jvm.cpp and invoke via a native method implementation of LockSupport. It would still be
just as unsafe of course.
Can you think of any reasons, beyond that of don't touch the core classes,
why we cannot copy this functionality over to java.lang.{*, Thread} ?
Would you be thinking of making the changes public, i.e. new standard API on
java.lang.Thread ?
Yes, j.l.Thread seems like the ideal location :-)
I don't see any point in moving it to Thread now. The public supported API
already exists in LockSupport.
Yes, but as i said the point is to tease apart the internal dependencies
between the platform and LockSupport class. (A simple solution is to copy and
rename this class to java.lang.ThreadSupport :-) .)
The issue is not LockSupport versus Thread, but the use of Unsafe.
It's because LockSupport uses Unsafe to access non-public fields of Thread, and
i also want to break that internal dependency.
I presume that LockSupport was created because agreement was not reached to add
such core *thread*-based functionality to Thread. I dunno why such agreement
was not reached, but so far i am not hearing any compelling technical reasons.
Paul.
snip
As I said you'd have to deprecate it in 9 with an intent to remove in 10 -
assuming you are even allowed to do that.
Opinions differ but i think we should be more serious about removing stuff that
has been marked deprecated for one major release if the intent to do so is made
clear when deprecating.
Re: RFR: JDK-8072842 Add support for building native JTReg tests
On 11 feb 2015, at 09:39, David Holmes david.hol...@oracle.com wrote: On 11/02/2015 6:34 PM, Staffan Larsen wrote: On 11 feb 2015, at 09:27, Magnus Ihse Bursie magnus.ihse.bur...@oracle.com mailto:magnus.ihse.bur...@oracle.com wrote: On 2015-02-11 09:23, David Holmes wrote: On 11/02/2015 6:09 PM, Magnus Ihse Bursie wrote: On 2015-02-11 02:35, David Holmes wrote: Hi Magnus, On 11/02/2015 12:23 AM, Magnus Ihse Bursie wrote: Here is an addition to the build system, that will compile native libraries and executables and make them available for JTReg tests written in Java. Sorry I'm missing the basics here: when does this compilation take place? As part of a normal build? Where will the build artifacts go? This is the first application of the new test-image/product-images separation we discussed previously. :) These tests are built as part of the test-image target. (Actually, they are built by individual rules like build-test-jdk-jtreg-native, and the relevant parts are put into the test image by test-image-jdk-jtreg-native, which test-image depends on.) Okay so if I just cd into hotspot/test and use the Makefile to try and run some jtreg tests it looks to me that it will define an invalid -nativepath I'm not sure if that is a supported use case. David or Staffan will have to answer to that. I have not tested that, only the whole forest approach. I’ve never done that. I’m always running all make commands from the top level. Is there a problem with that? I must confess I also haven't done that - though I often run jtreg directly from there. Other hotspot engineers may use it. If nothing else it would be a way to test out what you expect JPRT to be running. But perhaps we just don't add the -nativepath component if TESTNATIVE_DIR remains unset? Not adding -nativepath or adding it with an empty path will lead to the same errors I think: tests that need native code will fail. So it does not really matter. /Staffan Cheers, David /Staffan /Magnus
Re: RFR: JDK-8072842 Add support for building native JTReg tests
On 2015-02-11 09:23, David Holmes wrote: On 11/02/2015 6:09 PM, Magnus Ihse Bursie wrote: On 2015-02-11 02:35, David Holmes wrote: Hi Magnus, On 11/02/2015 12:23 AM, Magnus Ihse Bursie wrote: Here is an addition to the build system, that will compile native libraries and executables and make them available for JTReg tests written in Java. Sorry I'm missing the basics here: when does this compilation take place? As part of a normal build? Where will the build artifacts go? This is the first application of the new test-image/product-images separation we discussed previously. :) These tests are built as part of the test-image target. (Actually, they are built by individual rules like build-test-jdk-jtreg-native, and the relevant parts are put into the test image by test-image-jdk-jtreg-native, which test-image depends on.) Okay so if I just cd into hotspot/test and use the Makefile to try and run some jtreg tests it looks to me that it will define an invalid -nativepath I'm not sure if that is a supported use case. David or Staffan will have to answer to that. I have not tested that, only the whole forest approach. /Magnus
Re: RFR: JDK-8072842 Add support for building native JTReg tests
On 11 feb 2015, at 09:27, Magnus Ihse Bursie magnus.ihse.bur...@oracle.com wrote: On 2015-02-11 09:23, David Holmes wrote: On 11/02/2015 6:09 PM, Magnus Ihse Bursie wrote: On 2015-02-11 02:35, David Holmes wrote: Hi Magnus, On 11/02/2015 12:23 AM, Magnus Ihse Bursie wrote: Here is an addition to the build system, that will compile native libraries and executables and make them available for JTReg tests written in Java. Sorry I'm missing the basics here: when does this compilation take place? As part of a normal build? Where will the build artifacts go? This is the first application of the new test-image/product-images separation we discussed previously. :) These tests are built as part of the test-image target. (Actually, they are built by individual rules like build-test-jdk-jtreg-native, and the relevant parts are put into the test image by test-image-jdk-jtreg-native, which test-image depends on.) Okay so if I just cd into hotspot/test and use the Makefile to try and run some jtreg tests it looks to me that it will define an invalid -nativepath I'm not sure if that is a supported use case. David or Staffan will have to answer to that. I have not tested that, only the whole forest approach. I’ve never done that. I’m always running all make commands from the top level. Is there a problem with that? /Staffan /Magnus
Re: RFR: JDK-8072842 Add support for building native JTReg tests
On 11/02/2015 6:34 PM, Staffan Larsen wrote: On 11 feb 2015, at 09:27, Magnus Ihse Bursie magnus.ihse.bur...@oracle.com mailto:magnus.ihse.bur...@oracle.com wrote: On 2015-02-11 09:23, David Holmes wrote: On 11/02/2015 6:09 PM, Magnus Ihse Bursie wrote: On 2015-02-11 02:35, David Holmes wrote: Hi Magnus, On 11/02/2015 12:23 AM, Magnus Ihse Bursie wrote: Here is an addition to the build system, that will compile native libraries and executables and make them available for JTReg tests written in Java. Sorry I'm missing the basics here: when does this compilation take place? As part of a normal build? Where will the build artifacts go? This is the first application of the new test-image/product-images separation we discussed previously. :) These tests are built as part of the test-image target. (Actually, they are built by individual rules like build-test-jdk-jtreg-native, and the relevant parts are put into the test image by test-image-jdk-jtreg-native, which test-image depends on.) Okay so if I just cd into hotspot/test and use the Makefile to try and run some jtreg tests it looks to me that it will define an invalid -nativepath I'm not sure if that is a supported use case. David or Staffan will have to answer to that. I have not tested that, only the whole forest approach. I’ve never done that. I’m always running all make commands from the top level. Is there a problem with that? I must confess I also haven't done that - though I often run jtreg directly from there. Other hotspot engineers may use it. If nothing else it would be a way to test out what you expect JPRT to be running. But perhaps we just don't add the -nativepath component if TESTNATIVE_DIR remains unset? Cheers, David /Staffan /Magnus
Re: Unsafe.park/unpark, j.u.c.LockSupport and Thread
On Feb 11, 2015, at 2:43 AM, David Holmes david.hol...@oracle.com wrote:
Adding Doug Lea.
On 11/02/2015 12:51 AM, Paul Sandoz wrote:
On Feb 10, 2015, at 3:39 PM, Chris Hegarty chris.hega...@oracle.com wrote:
Adding native methods to 166 classes will introduce another form of
dependency on the platform that i would prefer to avoid.
Right. And I suspect that the separate distributable of 166, outside of the
Java Platform, would not want to have to carry a separate platform specific
native library.
Yes.
Right and for that reason jsr166 distribution would not want to have to know
whether to use Thread or Unsafe.
And that's why it should just be the former for the Java 9 platform and beyond.
The general strategy is to reduce dependency on Unsafe, including for 166
classes. The VarHandles effort is part of that strategy to replace Unsafe
enhanced access with a public alternative that is safe and just as performant.
But I don't see any reason why we couldn't move the implementation from
unsafe.cpp to jvm.cpp and invoke via a native method implementation of
LockSupport. It would still be just as unsafe of course.
Can you think of any reasons, beyond that of don't touch the core
classes, why we cannot copy this functionality over to java.lang.{*,
Thread} ?
Would you be thinking of making the changes public, i.e. new standard API
on java.lang.Thread ?
Yes, j.l.Thread seems like the ideal location :-)
I don't see any point in moving it to Thread now. The public supported API
already exists in LockSupport.
Yes, but as i said the point is to tease apart the internal dependencies
between the platform and LockSupport class. (A simple solution is to copy and
rename this class to java.lang.ThreadSupport :-) .)
The issue is not LockSupport versus Thread, but the use of Unsafe.
It's because LockSupport uses Unsafe to access non-public fields of Thread, and
i also want to break that internal dependency.
I presume that LockSupport was created because agreement was not reached to add
such core *thread*-based functionality to Thread. I dunno why such agreement
was not reached, but so far i am not hearing any compelling technical reasons.
Paul.
snip
As I said you'd have to deprecate it in 9 with an intent to remove in 10 -
assuming you are even allowed to do that.
Opinions differ but i think we should be more serious about removing stuff that
has been marked deprecated for one major release if the intent to do so is made
clear when deprecating.
Re: Lexicographic array comparators
On Feb 11, 2015, at 12:26 AM, John Rose john.r.r...@oracle.com wrote: On Feb 10, 2015, at 3:15 PM, Martin Buchholz marti...@google.com wrote: I was surprised that System.arraycopy hasn't been specialized for every array type. I guess the JIT is good at specializing all callers. Yes. Usually the arguments have specific static types the JIT can see. The Object formal type doesn't obscure this. I don't know whether we will someday regret the explosion of array comparison methods. It's technical debt. When we go to value types there will be an infinite number of array types. That's why in Project Valhalla we are thinking hard, before that, about parametric polymorphism. We need to be able to have true polymorphism over APIs in T[], where T can be ref, prim, or value. At that point, hopefully, what we have won't be impossible to retrofit. Yes, i am hoping that many of the overloaded methods on Arrays with the same code shape can be anyfied and reduced in a source and binary compatible way. In that respect i feel a less guilty about proposing so many methods :-) In this case we have the interesting interaction with Comparable: public static any T extends Comparable? super T int compare(T[] left, T[] right) We certainly don't want to box for T == int when making comparisons between two ints. I presume, in this case, the intrinsic array match method will deal with that aspect. -- An alternative solution is to leave Arrays alone and just to go with the lower level System.arrayMismatch method proposed in JDK-8044082 and then wait for value types. However, it requires significantly more work to write an intrinsic, which may put it at risk for the 9 time frame. Paul.
Re: RFR: JDK-8072842 Add support for building native JTReg tests
On 11/02/2015 6:09 PM, Magnus Ihse Bursie wrote: On 2015-02-11 02:35, David Holmes wrote: Hi Magnus, On 11/02/2015 12:23 AM, Magnus Ihse Bursie wrote: Here is an addition to the build system, that will compile native libraries and executables and make them available for JTReg tests written in Java. Sorry I'm missing the basics here: when does this compilation take place? As part of a normal build? Where will the build artifacts go? This is the first application of the new test-image/product-images separation we discussed previously. :) These tests are built as part of the test-image target. (Actually, they are built by individual rules like build-test-jdk-jtreg-native, and the relevant parts are put into the test image by test-image-jdk-jtreg-native, which test-image depends on.) Okay so if I just cd into hotspot/test and use the Makefile to try and run some jtreg tests it looks to me that it will define an invalid -nativepath David - The test-image target, as we discussed earlier, are built as part of all or all-images, but not the old images or jdk targets. Also, the test target depends on test-image, so you can actually start off a complete build by specifying just what tests you want to run. (test driven from a makefile point of view :-)) The build artifacts go into the new test image, in build/$BUILD/images/test, which hitherto has only contained a placeholder. For Oracle engineers: This test image is built by default by JPRT, and some of the changes in jprt.properties allow JPRT to know to depend on that test image to run tests. /Magnus Thanks, David H. This patch is the result of the work (in serial, mostly) of Staffan Larsen, David Simms and me. (And possible more that I don't know about.) In it current form, the patch only provides the framework on which to attach real tests. To prove the concept, some initial dummy tests are present. These are supposed to be removed as soon as real tests starts to propagate into the jdk and hotspot jtreg test suites. The behavior is based on the following design: For directories with native jtreg compilation enabled, the build system searches for native files prefixed with either lib or exe, and compiles a free-standing library or an executable, respectively, for each such file. Since all compiled files are placed in a single directory (this is currently a requirement from the jtreg native support), there is the additional requirement that all files have unique names. My personal opinion is that a better long-term approach is to compile all tests into a single library, if possible. (I realize some tests need to be separate to test library loading, etc.) For that to work, however, JTReg needs to be changed. The build framework in the patch is designed in such a way that it will be easy to add compilation to a common library in the future, if that restriction is lifted from JTReg. This patch also lays the foundation for building additional tests, not only native jtreg tests, by the build system in the future. For instance, it codifies the suggested correspondence between make targets, intermediate test output and test image final build results. With the on-going test co-location project, we expect a stream of tests to be added to the build system in the future, and we hope this project can serve as an example of a suitable way of integration. The patch modifies hotspot code, but it's mostly due to the addition of the new dummy tests. My preference would be to integrate this patch via jdk9-dev, since it modifies the build system most of all, but I'm open for discussion. Bug: https://bugs.openjdk.java.net/browse/JDK-8072842 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8072842-build-native-jtreg-tests/webrev.01 /Magnus
Re: RFR 8071600: Add a flat-mapping collector
On Feb 11, 2015, at 12:02 AM, Stuart Marks stuart.ma...@oracle.com wrote:
Hi Paul,
On 2/3/15 5:48 AM, Paul Sandoz wrote:
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071600-Collector-flatMapping/webrev/
This patch adds a new flat mapping collector to Collectors. This can be
useful if one needs to map 0 or more items into a downstream collector.
Mostly pretty good, just a couple minor nits.
Re the comment at TabulatorsTest.java line 513, this isn't a two-level
groupBy anymore, it's a groupByWithMapping (as the test name indicates).
I removed the comment. I also added the bug id to the test. Updated in place:
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071600-Collector-flatMapping-test-rename/webrev/
Given the new tests of closing in FlatMapOpTest.java, is it necessary to have
testFlatMappingClose() in TabulatorsTest.java?
Yes, they are testing different code paths.
TabulatorsTest.testFlatMappingClose tests the closing functionality of
Collectors.flatMapping:
public static T, U, A, R
CollectorT, ?, R flatMapping(Function? super T, ? extends Stream? extends
U mapper,
Collector? super U, A, R downstream) {
BiConsumerA, ? super U downstreamAccumulator = downstream.accumulator();
return new CollectorImpl(downstream.supplier(),
(r, t) - {
try (Stream? extends U result = mapper.apply(t)) {
if (result != null)
result.sequential().forEach(u -
downstreamAccumulator.accept(r, u));
}
},
downstream.combiner(), downstream.finisher(),
downstream.characteristics());
}
Where as the tests of closing in FlatMapOpTest test the closing of
Stream.flatMap*:
@Override
public final R StreamR flatMap(Function? super P_OUT, ? extends Stream?
extends R mapper) {
Objects.requireNonNull(mapper);
// We can do better than this, by polling cancellationRequested when stream
is infinite
return new StatelessOpP_OUT, R(this, StreamShape.REFERENCE,
StreamOpFlag.NOT_SORTED |
StreamOpFlag.NOT_DISTINCT | StreamOpFlag.NOT_SIZED) {
@Override
SinkP_OUT opWrapSink(int flags, SinkR sink) {
return new Sink.ChainedReferenceP_OUT, R(sink) {
@Override
public void begin(long size) {
downstream.begin(-1);
}
@Override
public void accept(P_OUT u) {
try (Stream? extends R result = mapper.apply(u)) {
// We can do better that this too; optimize for depth=0
case and just grab spliterator and forEach it
if (result != null)
result.sequential().forEach(downstream);
}
}
};
}
};
}
A following patch, which i plan to fold into the above patch, performs some
renames on the collectors test to be consistent with the current naming:
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071600-Collector-flatMapping-test-rename/webrev/
Renaming looks good and makes it easy to correlate the tests, the assertion
classes, and the collector implementations under test. Unfortunately, the
word for the act of collecting is collection which is confusing since
collection already has another meaning, but oh well.
Alas :-)
Thanks,
Paul.
Re: RFR 8071479: Stream and lamdification improvements to j.u.regex.Matcher
Hi Stuart, Thanks for the detailed review. Here is a possible way forward: 1) Add the methods to Matcher, as proposed in the initial webrev. 1.1) Change the specification of Matcher.results to reset the stream before matching, making it consistent with the replace* methods. 2) Add convenience methods for all replace*() and matches() on Pattern that defer to those on Matcher. We can do that in two stages, focusing on 1) in this review. I was not too concerned about the static method and the stream returning method having the same name as the context is quite different. For stream returning methods there is a de-facto pattern of using a plural of the stream element kind, so i prefer that to findAll. What about the name Pattern.matchResults? which chains well with Pattern.match(...).results(). -- Regarding the disparity between MatchResult and Matcher. I think that would require a new sub-interface of MatchResult from which Matcher extends from and returns. If we think it important we should do that for 9 otherwise we will be stuck for the stream-based methods. Paul. On Feb 11, 2015, at 2:02 AM, Stuart Marks stuart.ma...@oracle.com wrote: Hi Paul, I spent some time looking at this API. Overall it seems to me that things work a bit more nicely when these methods are added to Pattern instead of Matcher. Unfortunately there are some odd things with the existing API that make this tradeoff not so obvious. First, here's what a simple replacement operation looks like when replaceAll() is added to Matcher: String input = foobbfooabbbfoo; Pattern p = Pattern.compile(a*b); Matcher m = p.matcher(input); String result = m.replaceAll(mr - mr.group().toUpperCase()); But if replaceAll() is on Pattern, we can skip a step: String input = foobbfooabbbfoo; Pattern p = Pattern.compile(a*b); String result = p.replaceAll(input, mr - mr.group().toUpperCase()); Getting stream of match results is similar. So yes, I agree that it simplifies things to have these be on Pattern instead of Matcher. An advantage of having these on Pattern is that the matcher that gets created is encapsulated, and its state isn't exposed to being mucked about by the application. Thus you can avoid the additional concurrent modification checks that you have to do if replaceAll et. al. are on Matcher. Unfortunately, putting these on Pattern now creates some difficulties meshing with the existing API. One issue is that Matcher already has replaceAll(String) and replaceFirst(String). It would be strange to have these here and to have replaceAll(replacer) and replaceFirst(replacer) on Pattern. Another issue is that Matcher supports matching on region (subrange) of its input. For example, today you can do this: pattern.matcher(input).region(start, end) The region will constrain the matching for certain operations, such as find() (but not replaceAll or replaceFirst). If something like results() were added to Matcher, I'd expect that it would respect the Matcher's region, but if results() (or matches() as you called it) were on Pattern, the region constraint would be lacking. Also note that Pattern already has this: static boolean matches(regex, input) so I don't think an overload of matches() that returns a Stream would be a good idea. (Maybe findAll()?) Another issue, not directly related to where the new lambda/streams methods get added, is that MatchResult allows references only numbered capturing groups. Matcher, which implements MatchResult, also supports named capturing groups, with the new overloaded methods group(String), start(String), and end(String). These were added in Java 7. Logically these also belong on MatchResult, but they probably weren't added because of the compatibility issue of adding methods to interfaces. Maybe we should add these as default methods to MatchResult. (But what would the supported implementation be? Just throw UnsupportedOperationException?) -- I'm not entirely sure where this leaves things. It certainly seems more convenient to have the new methods on Pattern. But given the way the existing API is set up, it seems like it's a better fit to add them to Matcher. s'marks On 2/9/15 6:18 AM, Paul Sandoz wrote: Here is an alternative that pushes the methods on to Pattern instead: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8071479--Matcher-stream-results/on-Pattern/webrev/ (Whe webrev reports some files as empty, please ingore those, i have this webrev stacked on the previous one.) I have also included replaceFirst. This simplifies things for streaming on the match results and also for replacing. Note that the existing replace* methods on Matcher reset the matcher before matching and indicate that the matcher should be reset afterwards for reuse. Thus there is no loss in functionality moving
Re: Unsafe.park/unpark, j.u.c.LockSupport and Thread
On Feb 11, 2015, at 11:04 AM, David Holmes david.hol...@oracle.com wrote: Paul, I appreciate the issue with discontinuing use of Unsafe but this churn to the public APIs seems unwarranted. Would we have done it differently from day one? Sure. Does that mean we should change it all now? Not without a very good reason. And I'm on the fence there. Fair enough, i am less conservative than yourself on this matter :-) The fact that LockSupport needs access to Thread internals is something that modules could actually resolve :) - though I'm not up to date on the interaction of module access and package access. In this case 166 is imported into the base module. We don't want modules sitting on top of the platform to depend on internal platform classes, ideally we want to reduce the internal coupling between platform modules (via the use of qualified exports), and ideally the less Unsafe usage within the base module the better. The particulars of 166 development and usage, in that it gets imported into the platform but also used on top of the platform, make this a tricky balancing act. Let's see what Doug thinks of all this too. Yes. Paul.
Re: RFR: JDK-8072842 Add support for building native JTReg tests
On 2015-02-11 02:35, David Holmes wrote: Hi Magnus, On 11/02/2015 12:23 AM, Magnus Ihse Bursie wrote: Here is an addition to the build system, that will compile native libraries and executables and make them available for JTReg tests written in Java. Sorry I'm missing the basics here: when does this compilation take place? As part of a normal build? Where will the build artifacts go? This is the first application of the new test-image/product-images separation we discussed previously. :) These tests are built as part of the test-image target. (Actually, they are built by individual rules like build-test-jdk-jtreg-native, and the relevant parts are put into the test image by test-image-jdk-jtreg-native, which test-image depends on.) The test-image target, as we discussed earlier, are built as part of all or all-images, but not the old images or jdk targets. Also, the test target depends on test-image, so you can actually start off a complete build by specifying just what tests you want to run. (test driven from a makefile point of view :-)) The build artifacts go into the new test image, in build/$BUILD/images/test, which hitherto has only contained a placeholder. For Oracle engineers: This test image is built by default by JPRT, and some of the changes in jprt.properties allow JPRT to know to depend on that test image to run tests. /Magnus Thanks, David H. This patch is the result of the work (in serial, mostly) of Staffan Larsen, David Simms and me. (And possible more that I don't know about.) In it current form, the patch only provides the framework on which to attach real tests. To prove the concept, some initial dummy tests are present. These are supposed to be removed as soon as real tests starts to propagate into the jdk and hotspot jtreg test suites. The behavior is based on the following design: For directories with native jtreg compilation enabled, the build system searches for native files prefixed with either lib or exe, and compiles a free-standing library or an executable, respectively, for each such file. Since all compiled files are placed in a single directory (this is currently a requirement from the jtreg native support), there is the additional requirement that all files have unique names. My personal opinion is that a better long-term approach is to compile all tests into a single library, if possible. (I realize some tests need to be separate to test library loading, etc.) For that to work, however, JTReg needs to be changed. The build framework in the patch is designed in such a way that it will be easy to add compilation to a common library in the future, if that restriction is lifted from JTReg. This patch also lays the foundation for building additional tests, not only native jtreg tests, by the build system in the future. For instance, it codifies the suggested correspondence between make targets, intermediate test output and test image final build results. With the on-going test co-location project, we expect a stream of tests to be added to the build system in the future, and we hope this project can serve as an example of a suitable way of integration. The patch modifies hotspot code, but it's mostly due to the addition of the new dummy tests. My preference would be to integrate this patch via jdk9-dev, since it modifies the build system most of all, but I'm open for discussion. Bug: https://bugs.openjdk.java.net/browse/JDK-8072842 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8072842-build-native-jtreg-tests/webrev.01 /Magnus
Re: Lexicographic array comparators
On Feb 11, 2015, at 12:15 AM, Martin Buchholz marti...@google.com wrote: The addition of array comparison intrinsics makes sense - I've missed them myself on occasion. Thanks. I was surprised that System.arraycopy hasn't been specialized for every array type. I guess the JIT is good at specializing all callers. I don't know whether we will someday regret the explosion of array comparison methods. The case for the intrinsic seems more compelling to me. (Hopefully someone else will do the real review) No problem. It's too early for a proper webrev review. For the moment i am seeking comments on the general direction. Thanks, Paul.
Re: Unsafe.park/unpark, j.u.c.LockSupport and Thread
On 02/10/2015 08:46 PM, David Holmes wrote:
This time really adding Doug Lea :)
I don't have very strong opinions about this. Placing
park/unpark in Thread was our initial (circa 2000)
proposal, so in that sense it's ideal, and in principle
easily done by relaying LockSupport.{park/unpark} to Thread.
But we have over time also exploited the current setup
in a couple of cases to independently deal with Unsafe
access to park and parkBlockers. These can be changed,
although doing so while remaining performance-neutral
will take some thought. (Which is similar to all the
other cases where we'd like to replace Unsafe usages
with VarHandles etc. I'm trying to keep up the attitude
that this is not merely a nuisance, but an opportunity to
revisit and improve implementations :-)
Logistically, we'll somehow cope: At some point our main 166
sources will need to be split into jdk8 vs jdk9 versions.
-Doug
On 11/02/2015 11:43 AM, David Holmes wrote:
Adding Doug Lea.
On 11/02/2015 12:51 AM, Paul Sandoz wrote:
On Feb 10, 2015, at 3:39 PM, Chris Hegarty chris.hega...@oracle.com
wrote:
Adding native methods to 166 classes will introduce another form of
dependency on the platform that i would prefer to avoid.
Right. And I suspect that the separate distributable of 166, outside
of the Java Platform, would not want to have to carry a separate
platform specific native library.
Yes.
Right and for that reason jsr166 distribution would not want to have to
know whether to use Thread or Unsafe.
But I don't see any reason why we couldn't move the implementation
from unsafe.cpp to jvm.cpp and invoke via a native method
implementation of LockSupport. It would still be just as unsafe
of course.
Can you think of any reasons, beyond that of don't touch the core
classes, why we cannot copy this functionality over to
java.lang.{*, Thread} ?
Would you be thinking of making the changes public, i.e. new standard
API on java.lang.Thread ?
Yes, j.l.Thread seems like the ideal location :-)
I don't see any point in moving it to Thread now. The public supported
API already exists in LockSupport. As I said you'd have to deprecate it
in 9 with an intent to remove in 10 - assuming you are even allowed to
do that.
The issue is not LockSupport versus Thread, but the use of Unsafe.
David
Paul.
Re: RFR: JDK-8072842 Add support for building native JTReg tests
On 11 feb 2015, at 12:15, David Holmes david.hol...@oracle.com wrote: On 11/02/2015 8:36 PM, Staffan Larsen wrote: On 11 feb 2015, at 09:39, David Holmes david.hol...@oracle.com wrote: On 11/02/2015 6:34 PM, Staffan Larsen wrote: On 11 feb 2015, at 09:27, Magnus Ihse Bursie magnus.ihse.bur...@oracle.com mailto:magnus.ihse.bur...@oracle.com wrote: On 2015-02-11 09:23, David Holmes wrote: On 11/02/2015 6:09 PM, Magnus Ihse Bursie wrote: On 2015-02-11 02:35, David Holmes wrote: Hi Magnus, On 11/02/2015 12:23 AM, Magnus Ihse Bursie wrote: Here is an addition to the build system, that will compile native libraries and executables and make them available for JTReg tests written in Java. Sorry I'm missing the basics here: when does this compilation take place? As part of a normal build? Where will the build artifacts go? This is the first application of the new test-image/product-images separation we discussed previously. :) These tests are built as part of the test-image target. (Actually, they are built by individual rules like build-test-jdk-jtreg-native, and the relevant parts are put into the test image by test-image-jdk-jtreg-native, which test-image depends on.) Okay so if I just cd into hotspot/test and use the Makefile to try and run some jtreg tests it looks to me that it will define an invalid -nativepath I'm not sure if that is a supported use case. David or Staffan will have to answer to that. I have not tested that, only the whole forest approach. I’ve never done that. I’m always running all make commands from the top level. Is there a problem with that? I must confess I also haven't done that - though I often run jtreg directly from there. Other hotspot engineers may use it. If nothing else it would be a way to test out what you expect JPRT to be running. But perhaps we just don't add the -nativepath component if TESTNATIVE_DIR remains unset? Not adding -nativepath or adding it with an empty path will lead to the same errors I think: tests that need native code will fail. So it does not really matter. If you add it with an invalid path (won't be empty as the variable is only a prefix) then tests that don't need native code may also fail. Though I don't know how jtreg responds to a non-existent nativepath. You are right. Jtreg validates the that the path is a directory. So better not to specify it. /Staffan David /Staffan Cheers, David /Staffan /Magnus
Re: RFR: JDK-8072842 Add support for building native JTReg tests
On 11/02/2015 8:36 PM, Staffan Larsen wrote: On 11 feb 2015, at 09:39, David Holmes david.hol...@oracle.com wrote: On 11/02/2015 6:34 PM, Staffan Larsen wrote: On 11 feb 2015, at 09:27, Magnus Ihse Bursie magnus.ihse.bur...@oracle.com mailto:magnus.ihse.bur...@oracle.com wrote: On 2015-02-11 09:23, David Holmes wrote: On 11/02/2015 6:09 PM, Magnus Ihse Bursie wrote: On 2015-02-11 02:35, David Holmes wrote: Hi Magnus, On 11/02/2015 12:23 AM, Magnus Ihse Bursie wrote: Here is an addition to the build system, that will compile native libraries and executables and make them available for JTReg tests written in Java. Sorry I'm missing the basics here: when does this compilation take place? As part of a normal build? Where will the build artifacts go? This is the first application of the new test-image/product-images separation we discussed previously. :) These tests are built as part of the test-image target. (Actually, they are built by individual rules like build-test-jdk-jtreg-native, and the relevant parts are put into the test image by test-image-jdk-jtreg-native, which test-image depends on.) Okay so if I just cd into hotspot/test and use the Makefile to try and run some jtreg tests it looks to me that it will define an invalid -nativepath I'm not sure if that is a supported use case. David or Staffan will have to answer to that. I have not tested that, only the whole forest approach. I’ve never done that. I’m always running all make commands from the top level. Is there a problem with that? I must confess I also haven't done that - though I often run jtreg directly from there. Other hotspot engineers may use it. If nothing else it would be a way to test out what you expect JPRT to be running. But perhaps we just don't add the -nativepath component if TESTNATIVE_DIR remains unset? Not adding -nativepath or adding it with an empty path will lead to the same errors I think: tests that need native code will fail. So it does not really matter. If you add it with an invalid path (won't be empty as the variable is only a prefix) then tests that don't need native code may also fail. Though I don't know how jtreg responds to a non-existent nativepath. David /Staffan Cheers, David /Staffan /Magnus
Re: RFR (xs): JDK-8072611: (process) ProcessBuilder redirecting output to file should work with long file names (win)
Hi Thomas, I've just notices that your test has no copyright info and is indented with an indent width of 2 spaces instead of 4. If you don't mind I'll fix this before pushing so there's no need for a new webrev. Regards, Volker On Tue, Feb 10, 2015 at 10:58 AM, Thomas Stüfe thomas.stu...@gmail.com wrote: Hi Roger, Volker, thanks for reviewing! I added all requested changes: @Roger - use now Files.createTempDirectory to get a unique directory - wrapped test in try/finally to cleanup afterwards @Volker - moved include up and added dependency to comment in io_util_md.h - Now I use hostname.exe, which I hope is part of every Windows :) http://cr.openjdk.java.net/~stuefe/webrevs/8072611/webrev.02/webrev/ Regards, Thomas On Mon, Feb 9, 2015 at 7:48 PM, Volker Simonis volker.simo...@gmail.com wrote: Hi Thomas, the change looks good and I can sponsor it once it is reviewed. Just some small notes: - it seems that getPath() isn't used anywhere else in ProcessImpl_md.c and because it is static it can't be used anywhere else. So can you please remove it completely. - io_util_md.h has a list of files which use the prototypes like pathToNTPath before the declaration of the prototypes. Could you please also add ProcessImpl_md.c to this list - can you pleae place the new include in ProcessImpl_md.c as follows: #include io_util.h +#include io_util_md.h #include windows.h #include io.h I saw that system and local headers are already intermixed in that file but at I think at least we shouldn't introduce more disorder. - is robocopy really available on all supported Windows system. In http://en.wikipedia.org/wiki/Robocopy I read that it was introduced in Server 2008. I just verified that Java 8 only supports Server 2008 and above but just think of our internal test system:) Maybe we can use a program which is supported in every Windows version? Regards, Volker On Mon, Feb 9, 2015 at 2:30 PM, Thomas Stüfe thomas.stu...@gmail.com wrote: Hi all, please review this small change at your convenience: http://cr.openjdk.java.net/~stuefe/webrevs/8072611/webrev.01/webrev/ It fixes a small issue which causes ProcessBuilder not to be able to open files to redirect into (when using Redirect.append) on windows, if that file name is longer than 255 chars. Kind Regards, Thomas Stuefe
