RFR 8200788 : Optimal initial capacity of java.lang.VarHandle.AccessMode.methodNameToAccessMode
Hello! Yet another HashMap that can be preallocated more accurately. Currently, the map is created as the following: // Initial capacity of # values is sufficient to avoid resizes // for the smallest table size (32) methodNameToAccessMode = new HashMap<>(AccessMode.values().length); Even though the comment suggests that no resizes of the table should occur, in fact the threshold is calculated as 32 * 0.75 = 24, so when 24th element is inserted then the internal storage is reallocated and the content of the table is reinserted. Also, a missing @modules line is added to the regression test that was pushed with the fix for JDK-8200696. Would you please help review this? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8200788 WEBREV: http://cr.openjdk.java.net/~igerasim/8200788/00/webrev/ -- With kind regards, Ivan Gerasimov
Re: RFR: Export InitializeEncoding for JVM access
Hi Roger,
The OpenJ9 VM implementation provides its own java.lang.System, which
performs similar type early-on VM initialization to the OpenJDK core
library classes. The basic reason for invoking the method is to initialize
the platform encoding either called from a related core library method,
eg.initProperties(), or from the VM in the early stages of initialization.
My suggested comparison with canonicalize was based on it's indicated
usage:
/*
* Export the platform dependent path canonicalization so that
* VM can find it when loading system classes.
* This function is also used by the instrumentation agent.
*/
extern int canonicalize(char *path, const char *out, int len);
JNIEXPORT int
Canonicalize(JNIEnv *unused, char *orig, char *out, int len)
{
/* canonicalize an already natived path */
return canonicalize(orig, out, len);
}
Many thanks,
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number
741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
RE: RFR: 8164781: Pattern.asPredicate specification is incomplete
>> >> Hi, >> I have incorporated the changes as per the feedback and here is the >> updated webrev . >> http://cr.openjdk.java.net/~rraghavan/8164781/webrev.02/ . >>Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 >> >+1 Thanks Paul >I know it’s picky, but would you mind sticking closer to the existing line >length in the source file >(no need for another review) Here is my attempt. Hope it is better now. http://cr.openjdk.java.net/~rraghavan/8164781/webrev.03 >Did you run the jtreg test to verify it passes? I missed the problem >initially, glad Stuart caught it, but i presume the test would of reported a >failure? if not there is something wrong with the test itself that should be >investigated. I missed it earlier. As you rightly pointed out, without Stuart's inputs the test fails and it passes with it as expected. >> Here is the related csr https://bugs.openjdk.java.net/browse/JDK-8200603 >> >Ok, i tweaked some of the information (after creating a CSR one often needs to >edit it to fill in the gaps). >Can you blockquote the markdown for the embedded patch since the formatting is >all messed up? I have restored the formatting. Hope it would suffice. https://bugs.openjdk.java.net/browse/JDK-8200603 Regards Vivek
Re: RFR 8200788 : Optimal initial capacity of java.lang.VarHandle.AccessMode.methodNameToAccessMode
Hi, looks ok, but the cost/benefit ration of adding a standalone regression test for every such inefficiency seems dubious to me. Could we group these together, somewhere? Bonus startup points if you rewrote so that AccessMode.values() is only called once, since it clones the backing array. /Claes On 2018-04-05 09:04, Ivan Gerasimov wrote: Hello! Yet another HashMap that can be preallocated more accurately. Currently, the map is created as the following: // Initial capacity of # values is sufficient to avoid resizes // for the smallest table size (32) methodNameToAccessMode = new HashMap<>(AccessMode.values().length); Even though the comment suggests that no resizes of the table should occur, in fact the threshold is calculated as 32 * 0.75 = 24, so when 24th element is inserted then the internal storage is reallocated and the content of the table is reinserted. Also, a missing @modules line is added to the regression test that was pushed with the fix for JDK-8200696. Would you please help review this? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8200788 WEBREV: http://cr.openjdk.java.net/~igerasim/8200788/00/webrev/
Re: RFR: 8164781: Pattern.asPredicate specification is incomplete
Hi Vivek, Can we do something to make the first sentence less confusing? How about: * Creates a predicate that tests a given input string for a subsequence that matches this pattern. -or- * Creates a predicate that tests if this pattern is found in a given input string. Thanks, Roger On 4/5/18 6:34 AM, Vivek Theeyarath wrote: Hi, I have incorporated the changes as per the feedback and here is the updated webrev . http://cr.openjdk.java.net/~rraghavan/8164781/webrev.02/ . Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 +1 Thanks Paul I know it’s picky, but would you mind sticking closer to the existing line length in the source file (no need for another review) Here is my attempt. Hope it is better now. http://cr.openjdk.java.net/~rraghavan/8164781/webrev.03 Did you run the jtreg test to verify it passes? I missed the problem initially, glad Stuart caught it, but i presume the test would of reported a failure? if not there is something wrong with the test itself that should be investigated. I missed it earlier. As you rightly pointed out, without Stuart's inputs the test fails and it passes with it as expected. Here is the related csr https://bugs.openjdk.java.net/browse/JDK-8200603 Ok, i tweaked some of the information (after creating a CSR one often needs to edit it to fill in the gaps). Can you blockquote the markdown for the embedded patch since the formatting is all messed up? I have restored the formatting. Hope it would suffice. https://bugs.openjdk.java.net/browse/JDK-8200603 Regards Vivek
RFR: 8201178: Remove sun.nio.cs.FastCharsetProvider
Hi, since JDK-8073152 sun.nio.cs.FastCharsetProvider is unused and can be removed. Patch: hg rm src/java.base/share/classes/sun/nio/cs/FastCharsetProvider.java Bug: https://bugs.openjdk.java.net/browse/JDK-8201178 Thanks! /Claes
Re: RFR: 8201178: Remove sun.nio.cs.FastCharsetProvider
On 05/04/2018 14:42, Claes Redestad wrote: Hi, since JDK-8073152 sun.nio.cs.FastCharsetProvider is unused and can be removed. Patch: hg rm src/java.base/share/classes/sun/nio/cs/FastCharsetProvider.java Bug: https://bugs.openjdk.java.net/browse/JDK-8201178 I don't see any references to it (checked the tests too) so I think it's safe to remove it. -Alan
RFR [10] 8194554: filterArguments runs multiple filters in the wrong order
Hi,
Please review this jdk10 backport.
Original JDK 11 bug:
https://bugs.openjdk.java.net/browse/JDK-8194554
Original JDK 11 fix:
http://hg.openjdk.java.net/jdk/jdk/rev/050352ed64d5
Please note the discussion in JBS comments about the issue. It seems to include
the more verbose
specification text, and Rob says it makes the patch not directly backportable.
Therefore, requesting
to backport without the Javadoc change. I just dropped that single line from
the original changeset:
8194554: filterArguments runs multiple filters in the wrong order
Reviewed-by: psandoz, jrose
diff -r b09e56145e11 -r ab2dc3096cdb
src/java.base/share/classes/java/lang/invoke/MethodHandles.java
--- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java Thu Mar
08 04:23:31 2018 +
+++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java Wed Jan
17 15:17:50 2018 -0800
@@ -3836,11 +3836,12 @@
MethodHandle filterArguments(MethodHandle target, int pos, MethodHandle...
filters) {
filterArgumentsCheckArity(target, pos, filters);
MethodHandle adapter = target;
-int curPos = pos-1; // pre-incremented
-for (MethodHandle filter : filters) {
-curPos += 1;
+// process filters in reverse order so that the invocation of
+// the resulting adapter will invoke the filters in left-to-right order
+for (int i = filters.length - 1; i >= 0; --i) {
+MethodHandle filter = filters[i];
if (filter == null) continue; // ignore null elements of filters
-adapter = filterArgument(adapter, curPos, filter);
+adapter = filterArgument(adapter, pos + i, filter);
}
return adapter;
}
diff -r b09e56145e11 -r ab2dc3096cdb
test/jdk/java/lang/invoke/FilterArgumentsTest.java
--- /dev/null Thu Jan 01 00:00:00 1970 +
+++ b/test/jdk/java/lang/invoke/FilterArgumentsTest.javaWed Jan 17
15:17:50 2018 -0800
@@ -0,0 +1,132 @@
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8194554
+ * @run testng/othervm test.java.lang.invoke.FilterArgumentsTest
+ */
+
+package test.java.lang.invoke;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.List;
+
+import static java.lang.invoke.MethodHandles.*;
+import static java.lang.invoke.MethodType.methodType;
+
+import org.testng.annotations.*;
+import static org.testng.Assert.*;
+
+public class FilterArgumentsTest {
+
+@Test
+public static void testFilterA_B_C() throws Throwable {
+FilterTest test = new FilterTest(
+filterArguments(MH_TEST, 0, MH_FILTER_A, MH_FILTER_B,
MH_FILTER_C));
+test.run(List.of("A", "B", "C"));
+}
+
+@Test
+public static void testFilterA_B() throws Throwable {
+FilterTest test = new FilterTest(
+filterArguments(MH_TEST, 0, MH_FILTER_A, MH_FILTER_B));
+test.run(List.of("A", "B"));
+}
+
+@Test
+public static void testFilterB_C() throws Throwable {
+FilterTest test = new FilterTest(
+filterArguments(MH_TEST, 1, MH_FILTER_B, MH_FILTER_C));
+test.run(List.of("B", "C"));
+}
+
+@Test
+public static void testFilterB() throws Throwable {
+FilterTest test = new FilterTest(filterArguments(MH_TEST, 1,
MH_FILTER_B));
+test.run(List.of("B"));
+}
+
+@Test
+public static void testFilterC() throws Throwable {
+FilterTest test = new FilterTest(filterArguments(MH_TEST, 2,
MH_FILTER_C));
+test.run(List.of("C"));
+}
+
+static class FilterTest {
+static List filters = new ArrayList<>();
+
+final MethodHandle mh;
+FilterTest(MethodHandle mh) {
+this.mh = mh;
+}
+
+void run(List expected) throws Throwable {
+filters.clear();
+assertEquals("x-0-z", (String)mh.invokeExact("x", 0, '
RFR: jsr166 jdk integration 2018-04
http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html 8200728: Docs (Comparison of Stack and Deque methods) for Deque is not correct' src/main/java/util/Deque.java http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/Deque-Stack/index.html https://bugs.openjdk.java.net/browse/JDK-8200728 8200520: forkjoin tasks interrupted after shutdown http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/ForkJoinPool-shutdown/index.html https://bugs.openjdk.java.net/browse/JDK-8200520 8200258: Improve CopyOnWriteArrayList subList code http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/CopyOnWriteArrayList-subList/index.html https://bugs.openjdk.java.net/browse/JDK-8200258 8197531: Miscellaneous changes imported from jsr166 CVS 2018-04 http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/miscellaneous/index.html https://bugs.openjdk.java.net/browse/JDK-8197531
8194734 : Handle to jimage file inherited into child processes (win)
The problem in this bug is that jimage file is mistakenly opened with
"inherit by child processes" flag. In our case, the child process is
started with Runtime.exec() and serves as updater that can also replace
embedded JRE. However, due to jimage ("lib/modules") file handle being
inherited, embedded JRE can not be replaced.
The bug was introduced in commits "8087181: Move native jimage code to
its own library (maybe libjimage)"
Before this commit, os::open() was used in libjimage code and it handles
file inheritance correctly, see:
/hotspot/src/share/vm/classfile/imageFile.cpp
* ImageFileReader::open() uses os::open(_name, 0, O_RDONLY)
/hotspot/src/os/windows/vm/os_windows.cpp
* os::open() calls to ::open(pathbuf, oflag | O_BINARY | O_NOINHERIT, mode);
/hotspot/src/os/linux/vm/os_linux.cpp
* os::open() uses both O_CLOEXEC and FD_CLOEXEC
After this commit, a new osSupport::openReadOnly() is implemented and it
does not handle file inheritance properly.
Please find patch attached.
As far as I understand from OpenJDK's "How to contribute" page, I need a
"sponsor" who will get this patch applied.
Windows - compiled and tested:
Bug fixed.
Ubuntu - compiled and tested:
Bug did not occur before my patch due to another bug in childProcess()
function (src/java.base/unix/native/libjava/childproc.c) According to my
debugging, FD_CLOEXEC is applied to jimage file there because
childProcess() thinks that it's a error pipe (FAIL_FILENO). Should the
bug in childProcess() be fixed, it can unearth currently fixed bug.
# HG changeset patch
# User Alexander Miloslavskiy
# Date 1522703926 -10800
# Tue Apr 03 00:18:46 2018 +0300
# Node ID 87954e967cc67cab4b480a4ec7ff54a334e0f4ce
# Parent de0fd2c8a401a564f06d42fac15559154c42358a
8194734: jimage file descriptor was inherited into processes started with
Runtime.exec()
Summary: Prevented inheritance with FD_CLOEXEC on unix, O_NOINHERIT on windows.
Refactoring: added O_RDONLY (equals 0)
Refactoring: added O_BINARY on windows (currently ignored by osSupport::read
and osSupport::map_memory)
diff -r de0fd2c8a401 -r 87954e967cc6
src/java.base/unix/native/libjimage/osSupport_unix.cpp
--- a/src/java.base/unix/native/libjimage/osSupport_unix.cppFri Mar 30
14:36:18 2018 -0700
+++ b/src/java.base/unix/native/libjimage/osSupport_unix.cppTue Apr 03
00:18:46 2018 +0300
@@ -38,7 +38,16 @@
* Return the file descriptor.
*/
jint osSupport::openReadOnly(const char *path) {
-return ::open(path, 0);
+int fd = ::open(path, O_RDONLY);
+
+// jimage file descriptors must not be inherited by child processes.
+// This is especially true for child processes started with Runtime.exec
(see 8194734)
+#ifdef FD_CLOEXEC
+int flags = ::fcntl(fd, F_GETFD);
+::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
+#endif
+
+return fd;
}
/**
diff -r de0fd2c8a401 -r 87954e967cc6
src/java.base/windows/native/libjimage/osSupport_windows.cpp
--- a/src/java.base/windows/native/libjimage/osSupport_windows.cpp Fri Mar
30 14:36:18 2018 -0700
+++ b/src/java.base/windows/native/libjimage/osSupport_windows.cpp Tue Apr
03 00:18:46 2018 +0300
@@ -38,7 +38,9 @@
* Return the file descriptor.
*/
jint osSupport::openReadOnly(const char *path) {
-return ::open(path, 0, 0);
+// jimage file descriptors must not be inherited by child processes.
+// This is especially true for child processes started with Runtime.exec
(see 8194734)
+return ::open(path, O_BINARY | O_NOINHERIT, O_RDONLY);
}
/**
Re: RFR: 8196750 [Testbug] tools/launcher tests need to tolerate unrelated warnings
Looks good, thanks for doing this!. Kumar On 4/4/2018 5:09 PM, Andrey Nazarov wrote: On 22 Mar 2018, at 07:18, Kumar Srinivasan wrote: David, Why would the VM emit these warnings if the deprecated vm flag is not being used ? Andrey, As for the reviews I am ok with InfoStreams, wrt. ToolOpts it is not at all apparent, maybe more comments explaining what is going on ? Added more comments. see http://cr.openjdk.java.net/~anazarov/JDK-8196750/webrev.02/ Kumar Hi Andrei, On 22/03/2018 11:12 AM, Andrey Nazarov wrote: Hi, Please review fix in launcher tests. Review: http://cr.openjdk.java.net/~anazarov/JDK-8196750/webrev/ Bug: https://bugs.openjdk.java.net/browse/JDK-8196750?filter=-1 test/jdk/tools/launcher/ToolsOpts.java I don't understand how the change fixes the problem. IIUC the problem is that the test expects the output to consist of the command-line passed to the tool. Instead if it contains a Warning from the VM, it won't match and so we fail. The new code no longer uses a String[] and no longer assumes that output[i] == args[i], but it still searches the current "output" value for a match with one of the args. But that will still fail if what we actually have is a Warning. ?? I would have expected to see the Warning strings filtered out more directly. The other test change seems reasonable. Thanks, David —Thanks, Andrei
Re: RFR 8200788 : Optimal initial capacity of java.lang.VarHandle.AccessMode.methodNameToAccessMode
Thanks for review Claes! On 4/5/18 6:03 AM, Claes Redestad wrote: Hi, looks ok, but the cost/benefit ration of adding a standalone regression test for every such inefficiency seems dubious to me. Could we group these together, somewhere? Yes, I agree that this kind of tests should be combined with other sanity checks. However, I think it would be better organized, if the tests are placed in the tested-class specific directory. So, if we have some other sanity tests for VarHandle.AccessMode, then they probably should be combined with this one to save resources. Bonus startup points if you rewrote so that AccessMode.values() is only called once, since it clones the backing array. Good point. I'll do that before pushing. With kind regards, Ivan /Claes On 2018-04-05 09:04, Ivan Gerasimov wrote: Hello! Yet another HashMap that can be preallocated more accurately. Currently, the map is created as the following: // Initial capacity of # values is sufficient to avoid resizes // for the smallest table size (32) methodNameToAccessMode = new HashMap<>(AccessMode.values().length); Even though the comment suggests that no resizes of the table should occur, in fact the threshold is calculated as 32 * 0.75 = 24, so when 24th element is inserted then the internal storage is reallocated and the content of the table is reinserted. Also, a missing @modules line is added to the regression test that was pushed with the fix for JDK-8200696. Would you please help review this? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8200788 WEBREV: http://cr.openjdk.java.net/~igerasim/8200788/00/webrev/ -- With kind regards, Ivan Gerasimov
Promptly freeing the per-thread cached direct buffers when a thread exits
Hi all, We recently hit another interesting issue with the NIO thread-local DirectByteBuffer cache. One of our services seems to create and drop threads at regular intervals. I assume this is due to a thread pool dynamically resizing itself. Let's say a thread starts, lives long enough for its Thread object to be promoted to the old gen (along with its cached direct buffer), then exits. This will result in its cached direct buffer(s) being unreachable in the old gen and will only be reclaimed after the next full GC / concurrent GC cycle. Interestingly, the service in question does concurrent GC cycles really infrequently (one every few days) as it has a really low promotion rate. This results in the JVM's total direct size constantly increasing (which is effectively a native buffer leak). Has anyone come across this issue before? There are a few obvious ways to mitigate this, e.g., cause a Full GC / concurrent GC cycle at regular intervals. However, the best solution IMHO is to explicitly free any direct buffers that are still in the cache when a thread exits. I'll be happy to implement this and test it internally at Twitter, if it's not on anyone else's radar. However, to do what I'm proposing I need some sort of thread exit hook. Unfortunately, there doesn't seem to be one. Would proposing to introduce thread exit hooks be reasonable? Or is everyone going to freak out? :-) The hooks can be either per-Thread or even per-ThreadLocal. And it's OK if the hooks can only be used within java.base. FWIW, I did a simple prototype of this (I call the hooks from Thread::exit) and it seems to work as expected. Any thoughts / feedback on this will be very appreciated. Thanks, Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
Re: Promptly freeing the per-thread cached direct buffers when a thread exits
Hi Tony, On 6/04/2018 7:45 AM, Tony Printezis wrote: Hi all, We recently hit another interesting issue with the NIO thread-local DirectByteBuffer cache. One of our services seems to create and drop If it's in a ThreadLocal then aren't you just asking for thread-locals to be cleared at thread exit? Cheers, David threads at regular intervals. I assume this is due to a thread pool dynamically resizing itself. Let's say a thread starts, lives long enough for its Thread object to be promoted to the old gen (along with its cached direct buffer), then exits. This will result in its cached direct buffer(s) being unreachable in the old gen and will only be reclaimed after the next full GC / concurrent GC cycle. Interestingly, the service in question does concurrent GC cycles really infrequently (one every few days) as it has a really low promotion rate. This results in the JVM's total direct size constantly increasing (which is effectively a native buffer leak). Has anyone come across this issue before? There are a few obvious ways to mitigate this, e.g., cause a Full GC / concurrent GC cycle at regular intervals. However, the best solution IMHO is to explicitly free any direct buffers that are still in the cache when a thread exits. I'll be happy to implement this and test it internally at Twitter, if it's not on anyone else's radar. However, to do what I'm proposing I need some sort of thread exit hook. Unfortunately, there doesn't seem to be one. Would proposing to introduce thread exit hooks be reasonable? Or is everyone going to freak out? :-) The hooks can be either per-Thread or even per-ThreadLocal. And it's OK if the hooks can only be used within java.base. FWIW, I did a simple prototype of this (I call the hooks from Thread::exit) and it seems to work as expected. Any thoughts / feedback on this will be very appreciated. Thanks, Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
Re: Promptly freeing the per-thread cached direct buffers when a thread exits
On Thu, Apr 5, 2018 at 4:45 PM, Tony Printezis wrote: > Would proposing to introduce thread exit hooks be reasonable? Or is > everyone going to freak out? :-) The hooks can be either per-Thread or even > per-ThreadLocal. And it's OK if the hooks can only be used within java.base. User-accessible thread exit hooks would be nice, from a user perspective. From a JDK perspective - it adds new opportunities for user code to jam things up, so it's a tradeoff. ThreadLocal clearing on thread exit would have been nice back in the beginning, but now I think it would be a fairly substantial behavior change. Adding a new exit() method on ThreadLocal would be better (but not perfect) compatibility-wise, and see prior note about users jamming things up... I think that at a minimum, explicitly releasing thread-local NIO direct buffers on thread exit (without introducing a user facing API) is probably safe. Some kind of user-accessible hook would be nice, but I can't imagine it would take anything less than a massive discussion to get there. -- - DML
Re: RFR 8200788 : Optimal initial capacity of java.lang.VarHandle.AccessMode.methodNameToAccessMode
> On Apr 5, 2018, at 12:04 AM, Ivan Gerasimov wrote: > > Hello! > > Yet another HashMap that can be preallocated more accurately. > > Currently, the map is created as the following: >// Initial capacity of # values is sufficient to avoid resizes >// for the smallest table size (32) >methodNameToAccessMode = new HashMap<>(AccessMode.values().length); > > Even though the comment suggests that no resizes of the table should occur, > in fact the threshold is calculated as 32 * 0.75 = 24, so when 24th element > is inserted then the internal storage is reallocated and the content of the > table is reinserted. > Thanks for fixing this. I suspect that code and comment were written when there were fewer access modes. Paul. > Also, a missing @modules line is added to the regression test that was pushed > with the fix for JDK-8200696. > > Would you please help review this? > > BUGURL: https://bugs.openjdk.java.net/browse/JDK-8200788 > WEBREV: http://cr.openjdk.java.net/~igerasim/8200788/00/webrev/ > > -- > With kind regards, > Ivan Gerasimov >
Re: RFR: 8164781: Pattern.asPredicate specification is incomplete
> On Apr 5, 2018, at 3:34 AM, Vivek Theeyarath > wrote: > >>> >>> Hi, >>> I have incorporated the changes as per the feedback and here is the >>> updated webrev . >>> http://cr.openjdk.java.net/~rraghavan/8164781/webrev.02/ . >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 >>> > >> +1 > Thanks Paul > >> I know it’s picky, but would you mind sticking closer to the existing line >> length in the source file >> (no need for another review) > Here is my attempt. Hope it is better now. > http://cr.openjdk.java.net/~rraghavan/8164781/webrev.03 > Nope, still not aligned to ~80 chars. >> Did you run the jtreg test to verify it passes? I missed the problem >> initially, glad Stuart caught it, but i presume the test would of reported a >> failure? if not there is something wrong with the test itself that should be >> investigated. > > > I missed it earlier. As you rightly pointed out, without Stuart's inputs the > test fails and it passes with it as expected. > >>> Here is the related csr https://bugs.openjdk.java.net/browse/JDK-8200603 >>> > >> Ok, i tweaked some of the information (after creating a CSR one often needs >> to edit it to fill in the gaps). > >> Can you blockquote the markdown for the embedded patch since the formatting >> is all messed up? > > I have restored the formatting. Hope it would suffice. > https://bugs.openjdk.java.net/browse/JDK-8200603 > Much better. Paul.
Re: RFR: 8184692: add Pattern.asMatchPredicate
> On Apr 4, 2018, at 10:47 AM, Vivek Theeyarath > wrote: > > Hi All, > > Please review. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8184692 > > Webrev : http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.00/ > Like with your other patch, alignment to ~80 chars would be good, as that is mostly consistent with other code in the same source file. Let’s not use the word “find" here, so as not to confuse with matcher(s).find(). 5833 * @return The predicate which can be used for finding if an input string matches this pattern. I suggest: @return The predicate which can be used for matching an input string against this pattern You could also add a @see Matcher#matches Paul. > > > The related jtreg test was run and the test passed . > > > > Regards > > Vivek
RE: RFR: 8164781: Pattern.asPredicate specification is incomplete
Hi Roger, The sentence has been changed and here is the updated one http://cr.openjdk.java.net/~rraghavan/8164781/webrev.04/ . The line length has also been fixed as suggested by Paul to adhere to ~80 chars. I have finalized the csr too. Regards Vivek -Original Message- From: Roger Riggs Sent: Thursday, April 05, 2018 6:35 PM To: core-libs-dev@openjdk.java.net Subject: Re: RFR: 8164781: Pattern.asPredicate specification is incomplete Hi Vivek, Can we do something to make the first sentence less confusing? How about: * Creates a predicate that tests a given input string for a subsequence that matches this pattern. -or- * Creates a predicate that tests if this pattern is found in a given input string. Thanks, Roger On 4/5/18 6:34 AM, Vivek Theeyarath wrote: >>> Hi, >>> I have incorporated the changes as per the feedback and here is the >>> updated webrev . >>> http://cr.openjdk.java.net/~rraghavan/8164781/webrev.02/ . >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 >>> >> +1 > Thanks Paul > >> I know it’s picky, but would you mind sticking closer to the existing >> line length in the source file (no need for another review) > Here is my attempt. Hope it is better now. > http://cr.openjdk.java.net/~rraghavan/8164781/webrev.03 > >> Did you run the jtreg test to verify it passes? I missed the problem >> initially, glad Stuart caught it, but i presume the test would of reported a >> failure? if not there is something wrong with the test itself that should be >> investigated. > > I missed it earlier. As you rightly pointed out, without Stuart's inputs the > test fails and it passes with it as expected. > >>> Here is the related csr >>> https://bugs.openjdk.java.net/browse/JDK-8200603 >>> >> Ok, i tweaked some of the information (after creating a CSR one often needs >> to edit it to fill in the gaps). >> Can you blockquote the markdown for the embedded patch since the formatting >> is all messed up? > I have restored the formatting. Hope it would suffice. > https://bugs.openjdk.java.net/browse/JDK-8200603 > > Regards > Vivek
