Re: RFR JDK-6321472: Add CRC-32C API
On 10/23/2014 03:52 AM, Staffan Friberg wrote:
Webrev with these last updates. Added more tests to make sure CRC32C,
CRC32 and Checksum default methods all are covered.
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.07
Hi Staffan,
Regarding default case:
168 } else {
169 byte[] b = new byte[Math.min(buffer.remaining(), 4096)];
170 while (buffer.hasRemaining()) {
171 int length = Math.min(buffer.remaining(), b.length);
172 buffer.get(b, 0, length);
173 update(b, 0, length);
174 }
175 }
Have you tried using get()/getInt() directly on the (ro) ByteBuffer
instead of copying to byte[] chunks? Intuitively one would expect it
perform faster if a redundant copy is avoided. Ah, you already told us
that you plan to use intrinsic for CRC32C in the future, so you want to
have "addresses" at hand.
A hackish way to avoid copying in this case is to access the byte[] and
offset using reflection. But this would have to be wrapped with
doPrivileged() which would worsen performance for small buffers. A way
to avoid repeated access checks is to do them at class initialization
time, using MethodHandle(s). For example, something like:
private static final MethodHandle bbArrayGetter;
private static final MethodHandle bbArrayOffsetGetter;
static {
MethodHandle hbGetter;
MethodHandle offsetGetter;
try {
Field hbField = ByteBuffer.class.getDeclaredField("hb");
Field offsetField =
ByteBuffer.class.getDeclaredField("offset");
AccessController.doPrivileged(new PrivilegedAction() {
@Override
public Void run() {
hbField.setAccessible(true);
offsetField.setAccessible(true);
return null;
}
});
hbGetter = MethodHandles.lookup().unreflectGetter(hbField);
offsetGetter =
MethodHandles.lookup().unreflectGetter(offsetField);
} catch (NoSuchFieldException | IllegalAccessException e) {
hbGetter = null;
offsetGetter = null;
}
bbArrayGetter = hbGetter;
bbArrayOffsetGetter = offsetGetter;
}
private static byte[] getArrayOrNull(ByteBuffer bb) {
if (bb.hasArray()) return bb.array();
if (bbArrayGetter != null) {
try {
return (byte[]) bbArrayGetter.invokeExact(bb);
} catch (Throwable e) {
throw new InternalError(e);
}
}
return null;
}
private static int getArrayOffset(ByteBuffer bb) {
if (bb.hasArray()) return bb.arrayOffset();
if (bbArrayOffsetGetter != null) {
try {
return (int) bbArrayOffsetGetter.invokeExact(bb);
} catch (Throwable e) {
throw new InternalError(e);
}
}
throw new UnsupportedOperationException();
}
Regards, Peter
//Staffan
On 10/22/2014 05:37 PM, Stanimir Simeonoff wrote:
Hi Staffan,
The readonly buffer (ByteBuffer.asReadOnlyBuffer()) don't have
array() "working".
You can use "int length = Math.min(buffer.remaining, b.length)"
instead, same with new byte[Math.min(4096, buffer.remaining)]. Using
smaller chunks will be more performance friendly than
allocating/eating up a huge byte[].
If you feel like, add a test with a heap bytebuffer.asReadOnlyBuffer().
Stanimir
On Thu, Oct 23, 2014 at 3:06 AM, Staffan Friberg
mailto:staffan.frib...@oracle.com>> wrote:
Hi,
I was thinking about this earlier when I started writing the patch
and then I forgot about it again. I haven't been able to figure
out when the code will be executed. ByteBuffer is implemented in
such a way that only the JDK can extend it and as far as I can
tell you can only create 3 types of ByteBuffers (Direct, Mapped
and Heap), all of which will be handled by the more efficient
calls above.
That said just to make the code a bit safer from OOM it is
probably best to update the default method and all current
implementations which all use the same pattern.
A reasonable solution should be the following code
byte[] b = new byte[(buffer.remaining() < 4096)
? buffer.remaining() : 4096];
while (buffer.hasRemaining()) {
int length = (buffer.remaining() < b.length)
? buffer.remaining() : b.length;
buffer.get(b, 0, length);
update(b, 0, length);
}
Xueming, do you have any further comment?
Regards,
Staffan
On 10/22/2014 03:04 PM, Stanimir Simeonoff wrote:
On Thu, Oct 23, 2014 at 12:10 AM, Bernd Eckenfels
mailto:e...@zusammenkunft.net>> wrote:
Hello,
just a question in the default impl:
Re: RFR JDK-6321472: Add CRC-32C API
Unsafe is available, so the fields (array, offset) can be read directly
UNSAFE.getObject(buffer, hbOffset), UNSAFE.getObject(buffer, offsetOffset).
No need for MethodHandlers.
During class init the offsets have to be resolved, pretty much like any CAS
utilizing algorithm.
I didn't propose it as readOnlyBuffers are very, very rarely used and even
more unlikely to be used to calculate checksums. It just makes the code
ugly.
Stanimir
On Thu, Oct 23, 2014 at 11:05 AM, Peter Levart
wrote:
> On 10/23/2014 03:52 AM, Staffan Friberg wrote:
>
>> Webrev with these last updates. Added more tests to make sure CRC32C,
>> CRC32 and Checksum default methods all are covered.
>>
>> http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.07
>>
>
> Hi Staffan,
>
> Regarding default case:
>
> 168 } else {
> 169 byte[] b = new byte[Math.min(buffer.remaining(), 4096)];
> 170 while (buffer.hasRemaining()) {
> 171 int length = Math.min(buffer.remaining(), b.length);
> 172 buffer.get(b, 0, length);
> 173 update(b, 0, length);
> 174 }
> 175 }
>
>
> Have you tried using get()/getInt() directly on the (ro) ByteBuffer
> instead of copying to byte[] chunks? Intuitively one would expect it
> perform faster if a redundant copy is avoided. Ah, you already told us that
> you plan to use intrinsic for CRC32C in the future, so you want to have
> "addresses" at hand.
>
> A hackish way to avoid copying in this case is to access the byte[] and
> offset using reflection. But this would have to be wrapped with
> doPrivileged() which would worsen performance for small buffers. A way to
> avoid repeated access checks is to do them at class initialization time,
> using MethodHandle(s). For example, something like:
>
>
> private static final MethodHandle bbArrayGetter;
> private static final MethodHandle bbArrayOffsetGetter;
>
> static {
> MethodHandle hbGetter;
> MethodHandle offsetGetter;
> try {
> Field hbField = ByteBuffer.class.getDeclaredField("hb");
> Field offsetField = ByteBuffer.class.
> getDeclaredField("offset");
> AccessController.doPrivileged(new PrivilegedAction() {
> @Override
> public Void run() {
> hbField.setAccessible(true);
> offsetField.setAccessible(true);
> return null;
> }
> });
> hbGetter = MethodHandles.lookup().unreflectGetter(hbField);
> offsetGetter = MethodHandles.lookup().
> unreflectGetter(offsetField);
> } catch (NoSuchFieldException | IllegalAccessException e) {
> hbGetter = null;
> offsetGetter = null;
> }
> bbArrayGetter = hbGetter;
> bbArrayOffsetGetter = offsetGetter;
> }
>
> private static byte[] getArrayOrNull(ByteBuffer bb) {
> if (bb.hasArray()) return bb.array();
> if (bbArrayGetter != null) {
> try {
> return (byte[]) bbArrayGetter.invokeExact(bb);
> } catch (Throwable e) {
> throw new InternalError(e);
> }
> }
> return null;
> }
>
> private static int getArrayOffset(ByteBuffer bb) {
> if (bb.hasArray()) return bb.arrayOffset();
> if (bbArrayOffsetGetter != null) {
> try {
> return (int) bbArrayOffsetGetter.invokeExact(bb);
> } catch (Throwable e) {
> throw new InternalError(e);
> }
> }
> throw new UnsupportedOperationException();
> }
>
>
>
> Regards, Peter
>
>
>> //Staffan
>>
>> On 10/22/2014 05:37 PM, Stanimir Simeonoff wrote:
>>
>>> Hi Staffan,
>>>
>>> The readonly buffer (ByteBuffer.asReadOnlyBuffer()) don't have array()
>>> "working".
>>> You can use "int length = Math.min(buffer.remaining, b.length)" instead,
>>> same with new byte[Math.min(4096, buffer.remaining)]. Using smaller chunks
>>> will be more performance friendly than allocating/eating up a huge byte[].
>>> If you feel like, add a test with a heap bytebuffer.asReadOnlyBuffer().
>>>
>>> Stanimir
>>>
>>>
>>> On Thu, Oct 23, 2014 at 3:06 AM, Staffan Friberg <
>>> staffan.frib...@oracle.com > wrote:
>>>
>>> Hi,
>>>
>>> I was thinking about this earlier when I started writing the patch
>>> and then I forgot about it again. I haven't been able to figure
>>> out when the code will be executed. ByteBuffer is implemented in
>>> such a way that only the JDK can extend it and as far as I can
>>> tell you can only create 3 types of ByteBuffers (Direct, Mapped
>>> and Heap), all of which will be handled by the more efficient
>>> calls above.
>>>
>>> That said just to make the code a bit safer from OOM it is
>>> probably best to update the default method and all current
>>> imp
Re: RFR JDK-6321472: Add CRC-32C API
>>, UNSAFE.getObject(buffer, offsetOffset)
Obviously should be Unsafe.getInt(buffer, offsetOffset)
On Thu, Oct 23, 2014 at 11:16 AM, Stanimir Simeonoff
wrote:
> Unsafe is available, so the fields (array, offset) can be read directly
> UNSAFE.getObject(buffer, hbOffset), UNSAFE.getObject(buffer, offsetOffset).
> No need for MethodHandlers.
> During class init the offsets have to be resolved, pretty much like any
> CAS utilizing algorithm.
>
> I didn't propose it as readOnlyBuffers are very, very rarely used and even
> more unlikely to be used to calculate checksums. It just makes the code
> ugly.
>
> Stanimir
>
> On Thu, Oct 23, 2014 at 11:05 AM, Peter Levart
> wrote:
>
>> On 10/23/2014 03:52 AM, Staffan Friberg wrote:
>>
>>> Webrev with these last updates. Added more tests to make sure CRC32C,
>>> CRC32 and Checksum default methods all are covered.
>>>
>>> http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.07
>>>
>>
>> Hi Staffan,
>>
>> Regarding default case:
>>
>> 168 } else {
>> 169 byte[] b = new byte[Math.min(buffer.remaining(), 4096)];
>> 170 while (buffer.hasRemaining()) {
>> 171 int length = Math.min(buffer.remaining(), b.length);
>> 172 buffer.get(b, 0, length);
>> 173 update(b, 0, length);
>> 174 }
>> 175 }
>>
>>
>> Have you tried using get()/getInt() directly on the (ro) ByteBuffer
>> instead of copying to byte[] chunks? Intuitively one would expect it
>> perform faster if a redundant copy is avoided. Ah, you already told us that
>> you plan to use intrinsic for CRC32C in the future, so you want to have
>> "addresses" at hand.
>>
>> A hackish way to avoid copying in this case is to access the byte[] and
>> offset using reflection. But this would have to be wrapped with
>> doPrivileged() which would worsen performance for small buffers. A way to
>> avoid repeated access checks is to do them at class initialization time,
>> using MethodHandle(s). For example, something like:
>>
>>
>> private static final MethodHandle bbArrayGetter;
>> private static final MethodHandle bbArrayOffsetGetter;
>>
>> static {
>> MethodHandle hbGetter;
>> MethodHandle offsetGetter;
>> try {
>> Field hbField = ByteBuffer.class.getDeclaredField("hb");
>> Field offsetField = ByteBuffer.class.
>> getDeclaredField("offset");
>> AccessController.doPrivileged(new PrivilegedAction() {
>> @Override
>> public Void run() {
>> hbField.setAccessible(true);
>> offsetField.setAccessible(true);
>> return null;
>> }
>> });
>> hbGetter = MethodHandles.lookup().unreflectGetter(hbField);
>> offsetGetter = MethodHandles.lookup().
>> unreflectGetter(offsetField);
>> } catch (NoSuchFieldException | IllegalAccessException e) {
>> hbGetter = null;
>> offsetGetter = null;
>> }
>> bbArrayGetter = hbGetter;
>> bbArrayOffsetGetter = offsetGetter;
>> }
>>
>> private static byte[] getArrayOrNull(ByteBuffer bb) {
>> if (bb.hasArray()) return bb.array();
>> if (bbArrayGetter != null) {
>> try {
>> return (byte[]) bbArrayGetter.invokeExact(bb);
>> } catch (Throwable e) {
>> throw new InternalError(e);
>> }
>> }
>> return null;
>> }
>>
>> private static int getArrayOffset(ByteBuffer bb) {
>> if (bb.hasArray()) return bb.arrayOffset();
>> if (bbArrayOffsetGetter != null) {
>> try {
>> return (int) bbArrayOffsetGetter.invokeExact(bb);
>> } catch (Throwable e) {
>> throw new InternalError(e);
>> }
>> }
>> throw new UnsupportedOperationException();
>> }
>>
>>
>>
>> Regards, Peter
>>
>>
>>> //Staffan
>>>
>>> On 10/22/2014 05:37 PM, Stanimir Simeonoff wrote:
>>>
Hi Staffan,
The readonly buffer (ByteBuffer.asReadOnlyBuffer()) don't have array()
"working".
You can use "int length = Math.min(buffer.remaining, b.length)"
instead, same with new byte[Math.min(4096, buffer.remaining)]. Using
smaller chunks will be more performance friendly than allocating/eating up
a huge byte[].
If you feel like, add a test with a heap bytebuffer.asReadOnlyBuffer().
Stanimir
On Thu, Oct 23, 2014 at 3:06 AM, Staffan Friberg <
staffan.frib...@oracle.com > wrote:
Hi,
I was thinking about this earlier when I started writing the patch
and then I forgot about it again. I haven't been able to figure
out when the code will be executed. ByteBuffer is implemented in
such a way that only the JDK can extend it and as far as I can
>
Re: RFR (JAXP): 8061686: Size limits in BufferAllocator should have been final
On 21.10.2014 23:09, huizhe wang wrote: > http://cr.openjdk.java.net/~joehw/jdk9/8061686/webrev/ +1 (not a Reviewer) -Aleksey
Re: RFR JDK-6321472: Add CRC-32C API
On 10/23/2014 10:16 AM, Stanimir Simeonoff wrote:
Unsafe is available, so the fields (array, offset) can be read directly
UNSAFE.getObject(buffer, hbOffset), UNSAFE.getObject(buffer, offsetOffset).
No need for MethodHandlers.
During class init the offsets have to be resolved, pretty much like any CAS
utilizing algorithm.
I didn't propose it as readOnlyBuffers are very, very rarely used and even
more unlikely to be used to calculate checksums. It just makes the code
ugly.
Agreed. And when Staffan introduces intrinsic, he could pass the
ByteBuffer instance to it and extract the byte[] there...
Peter
Stanimir
On Thu, Oct 23, 2014 at 11:05 AM, Peter Levart
wrote:
On 10/23/2014 03:52 AM, Staffan Friberg wrote:
Webrev with these last updates. Added more tests to make sure CRC32C,
CRC32 and Checksum default methods all are covered.
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.07
Hi Staffan,
Regarding default case:
168 } else {
169 byte[] b = new byte[Math.min(buffer.remaining(), 4096)];
170 while (buffer.hasRemaining()) {
171 int length = Math.min(buffer.remaining(), b.length);
172 buffer.get(b, 0, length);
173 update(b, 0, length);
174 }
175 }
Have you tried using get()/getInt() directly on the (ro) ByteBuffer
instead of copying to byte[] chunks? Intuitively one would expect it
perform faster if a redundant copy is avoided. Ah, you already told us that
you plan to use intrinsic for CRC32C in the future, so you want to have
"addresses" at hand.
A hackish way to avoid copying in this case is to access the byte[] and
offset using reflection. But this would have to be wrapped with
doPrivileged() which would worsen performance for small buffers. A way to
avoid repeated access checks is to do them at class initialization time,
using MethodHandle(s). For example, something like:
private static final MethodHandle bbArrayGetter;
private static final MethodHandle bbArrayOffsetGetter;
static {
MethodHandle hbGetter;
MethodHandle offsetGetter;
try {
Field hbField = ByteBuffer.class.getDeclaredField("hb");
Field offsetField = ByteBuffer.class.
getDeclaredField("offset");
AccessController.doPrivileged(new PrivilegedAction() {
@Override
public Void run() {
hbField.setAccessible(true);
offsetField.setAccessible(true);
return null;
}
});
hbGetter = MethodHandles.lookup().unreflectGetter(hbField);
offsetGetter = MethodHandles.lookup().
unreflectGetter(offsetField);
} catch (NoSuchFieldException | IllegalAccessException e) {
hbGetter = null;
offsetGetter = null;
}
bbArrayGetter = hbGetter;
bbArrayOffsetGetter = offsetGetter;
}
private static byte[] getArrayOrNull(ByteBuffer bb) {
if (bb.hasArray()) return bb.array();
if (bbArrayGetter != null) {
try {
return (byte[]) bbArrayGetter.invokeExact(bb);
} catch (Throwable e) {
throw new InternalError(e);
}
}
return null;
}
private static int getArrayOffset(ByteBuffer bb) {
if (bb.hasArray()) return bb.arrayOffset();
if (bbArrayOffsetGetter != null) {
try {
return (int) bbArrayOffsetGetter.invokeExact(bb);
} catch (Throwable e) {
throw new InternalError(e);
}
}
throw new UnsupportedOperationException();
}
Regards, Peter
//Staffan
On 10/22/2014 05:37 PM, Stanimir Simeonoff wrote:
Hi Staffan,
The readonly buffer (ByteBuffer.asReadOnlyBuffer()) don't have array()
"working".
You can use "int length = Math.min(buffer.remaining, b.length)" instead,
same with new byte[Math.min(4096, buffer.remaining)]. Using smaller chunks
will be more performance friendly than allocating/eating up a huge byte[].
If you feel like, add a test with a heap bytebuffer.asReadOnlyBuffer().
Stanimir
On Thu, Oct 23, 2014 at 3:06 AM, Staffan Friberg <
staffan.frib...@oracle.com > wrote:
Hi,
I was thinking about this earlier when I started writing the patch
and then I forgot about it again. I haven't been able to figure
out when the code will be executed. ByteBuffer is implemented in
such a way that only the JDK can extend it and as far as I can
tell you can only create 3 types of ByteBuffers (Direct, Mapped
and Heap), all of which will be handled by the more efficient
calls above.
That said just to make the code a bit safer from OOM it is
probably best to update the default method and all current
implementations which all use t
Re: RFR: 8061830: [asm] refresh internal ASM version v5.0.3
On Oct 22, 2014, at 5:09 PM, Kumar Srinivasan wrote: > Hello, > > Please review fix for JDK-8061830, this is merely a refresh of the existing > source > base from upstream ObjectWeb/ASM, the webrev is here: > http://cr.openjdk.java.net/~ksrini/8061830/webrev.00/ > +1 Paul.
Re: Loading classes with many methods is very expensive
Hi Martin, On 23.10.2014 02:53, Martin Buchholz wrote: > If you have a class with ~64k methods with a superclass that also has ~64k > methods, class loading that single class will cost you ~30sec and calling > Class.getMethods another ~10sec. Both are unacceptably slow. I think both > are due to O(N^2) algorithms, the first in hotspot, and the second in > Class.java. Interesting, this is the profile: http://cr.openjdk.java.net/~shade/8061949/calltree-1.txt ...and I submitted two issues as the result of this quick performance investigation. We also need better benchmarks for this. https://bugs.openjdk.java.net/browse/JDK-8061949 https://bugs.openjdk.java.net/browse/JDK-8061950 Nashorn also generates the mammoth classes on many cases, and therefore solving both should (at least marginally) help there as well. -Aleksey.
Re: [9] Review request : JDK-8059070: [TESTBUG] java/lang/invoke/LFCaching/LFMultiThreadCachingTest.java failed - timeout
Gently reminder
On 17.10.2014 13:38, Konstantin Shefov wrote:
Hi,
I have updated the webrev:
http://cr.openjdk.java.net/~kshefov/8059070/webrev.01/
-Konstantin
16.10.2014 17:24, Igor Ignatyev пишет:
Konstantin,
I haven't looked at code religiously, so I wouldn't say that I have
reviewed it. however I have some comments, please see them inline.
On 10/16/2014 02:03 PM, Konstantin Shefov wrote:
Paul,
Thanks for reviewing
In the jtreg scripts of the three existing LFCaching tests timeout is
set explicitly to 300 seconds. The file currently being changed is
not a
test itself, it is parent class of tests.
In fact we can unset this explicit timeout and use the current fix
together with default JTREG timeout and jtreg timeout factor option.
These test cases are randomly generated, so the more iterations, the
better, but in fact we need at least one iteration.
As for "if (avgIterTime > 2 * remainTime)", I think 2 is enough.
-Konstantin
On 16.10.2014 13:30, Paul Sandoz wrote:
On Oct 16, 2014, at 10:43 AM, Konstantin Shefov
wrote:
Gently reminder
On 14.10.2014 16:58, Konstantin Shefov wrote:
Hello,
Please review the test bug fix
https://bugs.openjdk.java.net/browse/JDK-8059070
Webrev is http://cr.openjdk.java.net/~kshefov/8059070/webrev.00/
45 private static final long TIMEOUT = 30;
Does jtreg define a system property for this value? and is 300s the
same as what jtreg defines as the default?
unfortunately, jtreg doesn't define a property for timeout, but I
think it should do. we need to file an RFE against jtreg to add such
a property and an RFE against these tests/testlibrary to use this
property. could you please file them?
The online documentation (jtreg -onlineHelp) says the default is 2
minutes, but that might be out of date.
Might be more readable to use:
TimeUnit.SECONDS.toMillis(300);
143 long passedTime = new Date().getTime() - startTime;
Why don't you use System.currentTimeMillis() instead of "new
Date().getTime()" ?
145 double timeoutFactor = new
Double(System.getProperty("test.timeout.factor", "1.0"));
You can that pull out into a static and perhaps merge with TIMEOUT.
+ you should use jdk.testlibrary.Utils::adjustTimeout instead of
writing this code again.
147 if (avgIterTime > 2 * remainTime) {
That seems sufficient but it will be interesting to see if
intermittent failures still occur due to high variance.
Paul.
Igor
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/23/2014 02:54 AM, Mandy Chung wrote:
On 10/22/2014 4:58 PM, Claes Redestad wrote:
This test uses a special class loader that delegates to null class
loader only. Since you have the verification in place, it'd be good
to also call Package.getPackage and Package.getPackages to verify
that the same "package2" instance is returned. As a sanity check,
you could check "java.lang" package must be present.
I've added some sanity checks as suggested. Sadly, it seems that
since the application classloader will define and load package2 first
in this test, there'll always be a Package defined in the
ClassLoader.pkgs that will mask the Package instance in Package.pkgs
when retrieved via the public methods in Package.
Can you remove package2/Class2.class after you create the jar files?
Sorry for the confusion. I realized I did some of my test development on
an unpatched JDK and got caught up in a subtle issue. Prior to the
changes proposed, calling into Package.getSystemPackages() actually
created new Package objects on each call(!!), breaking the assumption
that only one Package object will ever be defined.
The proposed patch ensures we don't create new objects and that only one
Package object can ever be created for a system package. This bug(?) was
partially hidden by the fact that calling Package.getPackage() cached
Package objects in the ClassLoader package maps, thus subsequent calls
to Package.getPackages() would provide identical objects on subsequent
calls. I've updated the test to verify Package identity (which it fails
to do on an unpatched JDK).
If JDK-8061804 is resolved, we could change to check for identity in
the Package.getPackages() case, which would improve the specificness
of the test... For now, perhaps we could trick things in this test
and put our dummy class into java.lang in our test here and ensure
that the package retrieved is identical. Worth the hassle?
What I meant is to check if the returned Package contains "java.lang"
package that always exists in the system packages. You don't need to
put a dummy class in java.lang to get "java.lang" Package since it
must be defined in the running VM. e.g. you can refactor line 169-176
to a findPackage method that returns Package matching the given
package name such that in line 164 you can call
findPackage("java.lang") that should return non-null.
http://cr.openjdk.java.net/~redestad/8060130/webrev.10
/Claes
Re: Review request: JDK-8043277: Update jdk regression tests to extend the default security policy instead of override
This looks good to me Mandy, and nice to see the superfluous grants being removed. -Chris. On 23 Oct 2014, at 02:08, Mandy Chung wrote: > jtreg policy option overrides the system security policy file and hence > some existing test policy files have to duplicate the entries to grant > permissions for JDK. > > jtreg has been enhanced to provide "java.security.policy" option to > specify using the specified policy file in the same way as > -Djava.security.policy= to extend the system security > policy. The current jtreg "policy" option actually translates > to -Djava.security.policy== (double equals) to > override the system security policy. > > This patch updates several tests to use the new java.security.policy > options along with removing the duplicated entries from the test policy files. > > Webrev: > http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8043277/webrev.00/ > > thanks > Mandy > [1] https://bugs.openjdk.java.net/browse/CODETOOLS-7900898 >
Re: Loading classes with many methods is very expensive
On 10/23/2014 01:57 AM, Stanimir Simeonoff wrote: Class.java can easily be improved by adding an auxiliary HasMap indexing the position in the array by name and then checking only few overloaded signatures in case of a match. The auxiliary map can be deployed only past some threshold of methods, so it should not affect the common case. Alternatively sorting the array and using binary search is quite trivial to implement as well. Java Class.getMethods() implementation is complicated by the fact that, although not specified, the order of methods in returned array is important. Once it changed, if I remember correctly, and broke many programs, so it had to be restored... Peter Btw, really nice benchmark. Stanimir On Thu, Oct 23, 2014 at 1:53 AM, Martin Buchholz wrote: Here at Google we have both kinds of scalability problems - loading classes from a classpath with 10,000 jars, and loading a single class file packed with the maximal number of methods. This message is about the latter. If you have a class with ~64k methods with a superclass that also has ~64k methods, class loading that single class will cost you ~30sec and calling Class.getMethods another ~10sec. Both are unacceptably slow. I think both are due to O(N^2) algorithms, the first in hotspot, and the second in Class.java. I have the start of a fix for Class.java, but it makes the common case slower. A really good fix is harder to find. In general, I think Class.java could benefit from some performance-oriented rework. Is anyone else working on class loading performance, especially in hotspot? Here's the benchmark (that could perhaps be integrated into openjdk even without a fix) http://cr.openjdk.java.net/~martin/webrevs/openjdk9/Class.getMethods-benchmark/test/java/lang/Class/getMethods/ManyMethodsBenchmark.java.html Base class load time: 186.44 ms getDeclaredMethods: Methods: 65521, Total time: 43.27 ms, Time per method: 0.0007 ms getMethods: Methods: 65530, Total time: 60.82 ms, Time per method: 0.0009 ms Derived class load time: 33440.13 ms getDeclaredMethods: Methods: 65521, Total time: 39.71 ms, Time per method: 0.0006 ms getMethods: Methods: 65530, Total time: 11582.54 ms, Time per method: 0.1768 ms
Re: Review request: JDK-8043277: Update jdk regression tests to extend the default security policy instead of override
On 23/10/2014 02:08, Mandy Chung wrote: jtreg policy option overrides the system security policy file and hence some existing test policy files have to duplicate the entries to grant permissions for JDK. This looks okay to me too. I think this will be the first use of a jtreg4.1-b10 feature and maybe someone should send a note to jdk9-dev to tell folks that they will need an up-to-date jtreg in order to test jdk9/dev. -Alan
Re: Loading classes with many methods is very expensive
Peter, Which hotspot algorithm is the one you identified as slow? Would that be for loading the class or for Class.getMethods? thanks, Karen On Oct 23, 2014, at 9:37 AM, Peter Levart wrote: > On 10/23/2014 01:57 AM, Stanimir Simeonoff wrote: >> Class.java can easily be improved by adding an auxiliary HasMap> Integer/int[]> indexing the position in the array by name and then checking >> only few overloaded signatures in case of a match. >> The auxiliary map can be deployed only past some threshold of methods, so >> it should not affect the common case. >> >> Alternatively sorting the array and using binary search is quite trivial to >> implement as well. > > Java Class.getMethods() implementation is complicated by the fact that, > although not specified, the order of methods in returned array is important. > Once it changed, if I remember correctly, and broke many programs, so it had > to be restored... > > Peter > >> >> Btw, really nice benchmark. >> >> Stanimir >> >> >> On Thu, Oct 23, 2014 at 1:53 AM, Martin Buchholz >> wrote: >> >>> Here at Google we have both kinds of scalability problems - loading classes >>> from a classpath with 10,000 jars, and loading a single class file packed >>> with the maximal number of methods. This message is about the latter. >>> >>> If you have a class with ~64k methods with a superclass that also has ~64k >>> methods, class loading that single class will cost you ~30sec and calling >>> Class.getMethods another ~10sec. Both are unacceptably slow. I think both >>> are due to O(N^2) algorithms, the first in hotspot, and the second in >>> Class.java. >>> >>> I have the start of a fix for Class.java, but it makes the common case >>> slower. A really good fix is harder to find. In general, I think >>> Class.java could benefit from some performance-oriented rework. Is anyone >>> else working on class loading performance, especially in hotspot? >>> >>> Here's the benchmark (that could perhaps be integrated into openjdk even >>> without a fix) >>> >>> >>> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/Class.getMethods-benchmark/test/java/lang/Class/getMethods/ManyMethodsBenchmark.java.html >>> >>> Base class load time: 186.44 ms >>> getDeclaredMethods: Methods: 65521, Total time: 43.27 ms, Time per method: >>> 0.0007 ms >>> getMethods: Methods: 65530, Total time: 60.82 ms, Time per method: >>> 0.0009 ms >>> Derived class load time: 33440.13 ms >>> getDeclaredMethods: Methods: 65521, Total time: 39.71 ms, Time per method: >>> 0.0006 ms >>> getMethods: Methods: 65530, Total time: 11582.54 ms, Time per >>> method: 0.1768 ms >>> >
Re: Loading classes with many methods is very expensive
I was under the impression the declaring order has been abandoned in 1.7 which I considered quite a let down as allowed ordering the function calls in JMX in a non-chaotic way. On Thu, Oct 23, 2014 at 4:37 PM, Peter Levart wrote: > On 10/23/2014 01:57 AM, Stanimir Simeonoff wrote: > >> Class.java can easily be improved by adding an auxiliary HasMap> Integer/int[]> indexing the position in the array by name and then >> checking >> only few overloaded signatures in case of a match. >> The auxiliary map can be deployed only past some threshold of methods, so >> it should not affect the common case. >> >> Alternatively sorting the array and using binary search is quite trivial >> to >> implement as well. >> > > Java Class.getMethods() implementation is complicated by the fact that, > although not specified, the order of methods in returned array is > important. Once it changed, if I remember correctly, and broke many > programs, so it had to be restored... > > Peter > > > >> Btw, really nice benchmark. >> >> Stanimir >> >> >> On Thu, Oct 23, 2014 at 1:53 AM, Martin Buchholz >> wrote: >> >> Here at Google we have both kinds of scalability problems - loading >>> classes >>> from a classpath with 10,000 jars, and loading a single class file packed >>> with the maximal number of methods. This message is about the latter. >>> >>> If you have a class with ~64k methods with a superclass that also has >>> ~64k >>> methods, class loading that single class will cost you ~30sec and calling >>> Class.getMethods another ~10sec. Both are unacceptably slow. I think >>> both >>> are due to O(N^2) algorithms, the first in hotspot, and the second in >>> Class.java. >>> >>> I have the start of a fix for Class.java, but it makes the common case >>> slower. A really good fix is harder to find. In general, I think >>> Class.java could benefit from some performance-oriented rework. Is >>> anyone >>> else working on class loading performance, especially in hotspot? >>> >>> Here's the benchmark (that could perhaps be integrated into openjdk even >>> without a fix) >>> >>> >>> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/Class. >>> getMethods-benchmark/test/java/lang/Class/getMethods/ >>> ManyMethodsBenchmark.java.html >>> >>> Base class load time: 186.44 ms >>> getDeclaredMethods: Methods: 65521, Total time: 43.27 ms, Time per >>> method: >>> 0.0007 ms >>> getMethods: Methods: 65530, Total time: 60.82 ms, Time per >>> method: >>> 0.0009 ms >>> Derived class load time: 33440.13 ms >>> getDeclaredMethods: Methods: 65521, Total time: 39.71 ms, Time per >>> method: >>> 0.0006 ms >>> getMethods: Methods: 65530, Total time: 11582.54 ms, Time per >>> method: 0.1768 ms >>> >>> >
Re: Loading classes with many methods is very expensive
Hi Martin, On 23 okt 2014, at 00:53, Martin Buchholz wrote: > Here at Google we have both kinds of scalability problems - loading classes > from a classpath with 10,000 jars, and loading a single class file packed > with the maximal number of methods. This message is about the latter. > > If you have a class with ~64k methods with a superclass that also has ~64k > methods, class loading that single class will cost you ~30sec and calling > Class.getMethods another ~10sec. Both are unacceptably slow. I think both > are due to O(N^2) algorithms, the first in hotspot, and the second in > Class.java. Throw in lots of interfaces with lots of default methods and i suspect this can get even worse. I spent some time thinking about this when fixing an issue with reflection for default methods, reviewed here [1]. > I have the start of a fix for Class.java, but it makes the common case > slower. A really good fix is harder to find. In general, I think > Class.java could benefit from some performance-oriented rework. Is anyone > else working on class loading performance, especially in hotspot? > We have been thinking about replacing the duplication of the method lookup logic in j.l.Class with a call to the VM which should already have the information needed. I’m not sure why the logic was duplicated on the library side way back when this was written. This isn’t something we are actively working on though. As an aside, I often found myself wanting for the actual method descriptor when working with this, but considering there can be *a lot* of instances of Method/Constructor adding a descriptor field to Executable wasn’t an obvious win to me. cheers /Joel [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-May/026782.html
Re: Loading classes with many methods is very expensive
On 10/23/2014 03:48 PM, Karen Kinnear wrote: Peter, Which hotspot algorithm is the one you identified as slow? Would that be for loading the class or for Class.getMethods? thanks, Karen Martin did it. And Alexey profiled it. I just noted that Java part of Class.getMethods(), should it be rewritten, might be required to keep the order of Method objects returned in the array although it is not specified. Class.getMethods() is based on native Class methods which return declared methods. They are not slow. The Java algorithm that combines those into a compiled list of public methods (which includes inherited from supertypes) is slow for large number of methods since it is O(n^2). Regards, Peter On Oct 23, 2014, at 9:37 AM, Peter Levart wrote: On 10/23/2014 01:57 AM, Stanimir Simeonoff wrote: Class.java can easily be improved by adding an auxiliary HasMap indexing the position in the array by name and then checking only few overloaded signatures in case of a match. The auxiliary map can be deployed only past some threshold of methods, so it should not affect the common case. Alternatively sorting the array and using binary search is quite trivial to implement as well. Java Class.getMethods() implementation is complicated by the fact that, although not specified, the order of methods in returned array is important. Once it changed, if I remember correctly, and broke many programs, so it had to be restored... Peter Btw, really nice benchmark. Stanimir On Thu, Oct 23, 2014 at 1:53 AM, Martin Buchholz wrote: Here at Google we have both kinds of scalability problems - loading classes from a classpath with 10,000 jars, and loading a single class file packed with the maximal number of methods. This message is about the latter. If you have a class with ~64k methods with a superclass that also has ~64k methods, class loading that single class will cost you ~30sec and calling Class.getMethods another ~10sec. Both are unacceptably slow. I think both are due to O(N^2) algorithms, the first in hotspot, and the second in Class.java. I have the start of a fix for Class.java, but it makes the common case slower. A really good fix is harder to find. In general, I think Class.java could benefit from some performance-oriented rework. Is anyone else working on class loading performance, especially in hotspot? Here's the benchmark (that could perhaps be integrated into openjdk even without a fix) http://cr.openjdk.java.net/~martin/webrevs/openjdk9/Class.getMethods-benchmark/test/java/lang/Class/getMethods/ManyMethodsBenchmark.java.html Base class load time: 186.44 ms getDeclaredMethods: Methods: 65521, Total time: 43.27 ms, Time per method: 0.0007 ms getMethods: Methods: 65530, Total time: 60.82 ms, Time per method: 0.0009 ms Derived class load time: 33440.13 ms getDeclaredMethods: Methods: 65521, Total time: 39.71 ms, Time per method: 0.0006 ms getMethods: Methods: 65530, Total time: 11582.54 ms, Time per method: 0.1768 ms
Re: Loading classes with many methods is very expensive
On 10/23/2014 05:06 PM, Joel Borggrén-Franck wrote:
Hi Martin,
On 23 okt 2014, at 00:53, Martin Buchholz wrote:
Here at Google we have both kinds of scalability problems - loading classes
from a classpath with 10,000 jars, and loading a single class file packed
with the maximal number of methods. This message is about the latter.
If you have a class with ~64k methods with a superclass that also has ~64k
methods, class loading that single class will cost you ~30sec and calling
Class.getMethods another ~10sec. Both are unacceptably slow. I think both
are due to O(N^2) algorithms, the first in hotspot, and the second in
Class.java.
Throw in lots of interfaces with lots of default methods and i suspect this can
get even worse. I spent some time thinking about this when fixing an issue with
reflection for default methods, reviewed here [1].
Hi Joel,
I may have found another issue:
interface A4 extends B4, C4 {}
interface B4 extends D4 { void m(); }
interface C4 extends D4 {}
interface D4 { void m(); }
Calling A4.class.getMethods() returns B4.m, D4.m - this is supposed to
be OK as per JDK 7 spec.
Changing both methods to default methods:
interface A5 extends B5, C5 {}
interface B5 extends D5 { default void m() {} }
interface C5 extends D5 {}
interface D5 { default void m() {}; }
A5.class.getMethods() returns B5.m - this is new in JDK 8 spec for
default methods.
Now see the following two examples (just one method is default, the
other is abstract):
interface A6 extends B6, C6 {}
interface B6 extends D6 { void m(); }
interface C6 extends D6 {}
interface D6 { default void m() {}; }
A6.class.getMethods() returns B6.m, D6.m
// B.m, D.m
interface A7 extends B7, C7 {}
interface B7 extends D7 { default void m() {} }
interface C7 extends D7 {}
interface D7 { void m(); }
A7.class.getMethods() returns B7.m
Do last two examples give expected result? Why are A6 and A7 different?
What is the rule here? I would expect A6.class.getMethods() only return
the D6.m default method. This is the non-abstract method that gets used
when implementing an interface.
If A6 example is really showing a bug, then I may have a solution that
fixes it *and* is faster than current code for getMethods() + it is O(n)...
Is there a test that validates correctness of getMethods() or at least a
set of interfaces and/or classes to exercise the algorithm and compare
it to a different implementation?
I have the start of a fix for Class.java, but it makes the common case
slower. A really good fix is harder to find. In general, I think
Class.java could benefit from some performance-oriented rework. Is anyone
else working on class loading performance, especially in hotspot?
We have been thinking about replacing the duplication of the method lookup
logic in j.l.Class with a call to the VM which should already have the
information needed. I’m not sure why the logic was duplicated on the library
side way back when this was written. This isn’t something we are actively
working on though.
Does VM have enough information to quickly compile the list of public
methods (getMethods() equivalent) or only to look-up the public method
(getMethod() equivalent)?
Regards, Peter
As an aside, I often found myself wanting for the actual method descriptor when
working with this, but considering there can be *a lot* of instances of
Method/Constructor adding a descriptor field to Executable wasn’t an obvious
win to me.
cheers
/Joel
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-May/026782.html
Re: Loading classes with many methods is very expensive
On 10/23/2014 05:44 PM, Peter Levart wrote:
interface A6 extends B6, C6 {}
interface B6 extends D6 { void m(); }
interface C6 extends D6 {}
interface D6 { default void m() {}; }
A6.class.getMethods() returns B6.m, D6.m
Ah, B6.m re-abstracts the default method D6.m. I can see the rule here.
Never mind my previous question.
Regards, Peter
Re: [9] Review request : JDK-8059070: [TESTBUG] java/lang/invoke/LFCaching/LFMultiThreadCachingTest.java failed - timeout
On Oct 23, 2014, at 1:25 PM, Konstantin Shefov wrote: > Gently reminder > > On 17.10.2014 13:38, Konstantin Shefov wrote: >> Hi, >> >> I have updated the webrev: >> http://cr.openjdk.java.net/~kshefov/8059070/webrev.01/ >> +1 Sorry for the delay, Paul.
Re: Loading classes with many methods is very expensive
On 23 Oct 2014, at 17:44, Peter Levart wrote: > > Is there a test that validates correctness of getMethods() or at least a set > of interfaces and/or classes to exercise the algorithm and compare it to a > different implementation? > There is :) http://hg.openjdk.java.net/jdk9/dev/jdk/file/8b4aa51c8744/test/java/lang/reflect/DefaultMethodMembers/FilterNotMostSpecific.java IIRC 75% or so of the cases passes both before and after the change. I have highlighted a number of interesting cases with line comments. cheers /Joel
Re: Review request: JDK-8043277: Update jdk regression tests to extend the default security policy instead of override
Sean, I have included a few security tests in this patch: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8043277/webrev.01/ Mandy On 10/22/14 6:08 PM, Mandy Chung wrote: jtreg policy option overrides the system security policy file and hence some existing test policy files have to duplicate the entries to grant permissions for JDK. jtreg has been enhanced to provide "java.security.policy" option to specify using the specified policy file in the same way as -Djava.security.policy= to extend the system security policy. The current jtreg "policy" option actually translates to -Djava.security.policy== (double equals) to override the system security policy. This patch updates several tests to use the new java.security.policy options along with removing the duplicated entries from the test policy files. Webrev: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8043277/webrev.00/ thanks Mandy [1] https://bugs.openjdk.java.net/browse/CODETOOLS-7900898
Re: Review request: JDK-8043277: Update jdk regression tests to extend the default security policy instead of override
On 10/23/2014 02:10 PM, Mandy Chung wrote: Sean, I have included a few security tests in this patch: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8043277/webrev.01/ Looks good. --Sean
Re: RFR JDK-6321472: Add CRC-32C API
Hi,
Using unsafe to read fields will make the code very brittle and require
us to further detect and buffer types and which fields to read and
handle. Currently this seems to be a rather uncommon case, as the code
in Adler32 and CRC32 has done a full allocation in JDK 8, and by far
either wrapped ByteBuffers or DirectByteBuffers are expected to be used.
So for now I will keep the code as is without further complications.
The performance with the copying looks reasonable. The result is
multiplied with array length (@OperationsPerInvocation(65536 or 1024))
so score ends up being bytes/ms.
64k buffer length
Benchmark Mode SamplesScore Score
error Units
o.c.CRC32C.directByteBuffer_64K thrpt5 998151.406
1539.451 ops/ms
o.c.CRC32C.readonlyByteBuffer_64Kthrpt5 961665.219
36823.059 ops/ms
o.c.CRC32C.wrappedByteBuffer_64K thrpt5 1030445.833
9069.889 ops/ms
1k buffer length
BenchmarkMode Samples Score Score
error Units
o.c.CRC32C.directByteBuffer_1K thrpt5 987614.289 3506.829
ops/ms
o.c.CRC32C.readonlyByteBuffer_1Kthrpt5 691888.488
217080.205 ops/ms
o.c.CRC32C.wrappedByteBuffer_1K thrpt5 974676.434
463.487 ops/ms
//Staffan
On 10/23/2014 02:05 AM, Peter Levart wrote:
On 10/23/2014 10:16 AM, Stanimir Simeonoff wrote:
Unsafe is available, so the fields (array, offset) can be read directly
UNSAFE.getObject(buffer, hbOffset), UNSAFE.getObject(buffer,
offsetOffset).
No need for MethodHandlers.
During class init the offsets have to be resolved, pretty much like
any CAS
utilizing algorithm.
I didn't propose it as readOnlyBuffers are very, very rarely used and
even
more unlikely to be used to calculate checksums. It just makes the code
ugly.
Agreed. And when Staffan introduces intrinsic, he could pass the
ByteBuffer instance to it and extract the byte[] there...
Peter
Stanimir
On Thu, Oct 23, 2014 at 11:05 AM, Peter Levart
wrote:
On 10/23/2014 03:52 AM, Staffan Friberg wrote:
Webrev with these last updates. Added more tests to make sure CRC32C,
CRC32 and Checksum default methods all are covered.
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.07
Hi Staffan,
Regarding default case:
168 } else {
169 byte[] b = new byte[Math.min(buffer.remaining(),
4096)];
170 while (buffer.hasRemaining()) {
171 int length = Math.min(buffer.remaining(),
b.length);
172 buffer.get(b, 0, length);
173 update(b, 0, length);
174 }
175 }
Have you tried using get()/getInt() directly on the (ro) ByteBuffer
instead of copying to byte[] chunks? Intuitively one would expect it
perform faster if a redundant copy is avoided. Ah, you already told
us that
you plan to use intrinsic for CRC32C in the future, so you want to have
"addresses" at hand.
A hackish way to avoid copying in this case is to access the byte[] and
offset using reflection. But this would have to be wrapped with
doPrivileged() which would worsen performance for small buffers. A
way to
avoid repeated access checks is to do them at class initialization
time,
using MethodHandle(s). For example, something like:
private static final MethodHandle bbArrayGetter;
private static final MethodHandle bbArrayOffsetGetter;
static {
MethodHandle hbGetter;
MethodHandle offsetGetter;
try {
Field hbField = ByteBuffer.class.getDeclaredField("hb");
Field offsetField = ByteBuffer.class.
getDeclaredField("offset");
AccessController.doPrivileged(new
PrivilegedAction() {
@Override
public Void run() {
hbField.setAccessible(true);
offsetField.setAccessible(true);
return null;
}
});
hbGetter =
MethodHandles.lookup().unreflectGetter(hbField);
offsetGetter = MethodHandles.lookup().
unreflectGetter(offsetField);
} catch (NoSuchFieldException | IllegalAccessException e) {
hbGetter = null;
offsetGetter = null;
}
bbArrayGetter = hbGetter;
bbArrayOffsetGetter = offsetGetter;
}
private static byte[] getArrayOrNull(ByteBuffer bb) {
if (bb.hasArray()) return bb.array();
if (bbArrayGetter != null) {
try {
return (byte[]) bbArrayGetter.invokeExact(bb);
} catch (Throwable e) {
throw new InternalError(e);
}
}
return null;
}
private static int getArrayOffset(ByteBuffer bb) {
if (bb.hasArray()) return bb.arrayOffset();
if (bbArrayOffsetGetter != null) {
try {
return (int) bbArrayOffsetGetter.invokeExact(b
RFR: JDK-8049522 - Move @implNote in org.omg.CORBA.ORB to init method
Hi, please oblige and review the change http://cr.openjdk.java.net/~msheppar/8049522/webrev/ to address the issue https://bugs.openjdk.java.net/browse/JDK-8049522 this is javadoc change, which amends the implNote in ORB.java, relating to the ORBSingleton class loading strategy in JDK9. regards Mark
Re: [7u-dev ONLY] RFR: [8057530] (process) Runtime.exec throws garbled message in jp locale
Thanks Dalibor! Could someone with the Reviewer status review this please? Sincerely yours, Ivan On 23.10.2014 18:07, dalibor topic wrote: Approved pending positive review. On 21.10.2014 08:36, Ivan Gerasimov wrote: Hello everybody! This is a request for review of a fix which is applicable to 7u only. The issue has already been addressed in 8 and later releases. The fix is a combined backport of https://bugs.openjdk.java.net/browse/JDK-8016579 , which addresses the problem with the encoding in windows console and partial backport of https://bugs.openjdk.java.net/browse/JDK-8001334 , which reorganizes the code at the files of our interest. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8057530 WEBREV: http://cr.openjdk.java.net/~igerasim/8057530/0/webrev/ The fix has been verified on the localized versions of Windows (jp, rus). Thanks in advance, Ivan
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 2)
Hi Mandy,
Thanks for the review. I have updated the patch:
http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v2/
Please see comments in-line.
On 10/21/14, 12:56 PM, Mandy Chung wrote:
Hi Ioi,
On 10/21/14 10:27 AM, Ioi Lam wrote:
Please review this fix:
http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v1/
This looks better than the preliminary webrev you sent me offline
earlier. I only review the jdk repo change.
Launcher.java
line 290: it can be made a final field.
line 297: this.ucp =
Done
line 317-319: I think they are meant to be two sentences.
I have the following suggested text:
// The class of the given name is not found in the parent
// class loader as well as its local URLClassPath.
// Check if this class has already been defined dynamically;
// if so, return the loaded class; otherwise, skip the parent
// delegation and findClass.
I have used your comment and removed the old one.
URLClassPath.java
line 76: this long line should be broken into multiple line.
Maybe you can addsun.security.action.GetPropertyAction in the
import and update line 72, 74, 76, 78.
Done
line 319-326: Is this LoaderType enum necessary? I think it can
simply pass the ClassLoader instance to VM rather than having
this enum type just for this purpose. I suggest to get rid
of the LoaderType enum type.
I changed the API to pass the ClassLoader instance and removed the
LoaderType.
line 398: what happens if loaders.size() > maxindex? Shouldn't
it return null?
In this case, all the loaders that's needed by the cache[] have been
opened. So we should return cache[].
404 if (getNextLoader(null, maxindex) == null ||
405 !lookupCacheEnabled) {
line 405 should indent to the right for better readability.
The comment helps as I was initially confused why lookupCacheEnabled
is not checked first. Am I right to say line 398-415 is equivalent
to (excluding the debugging statements):
if (loaders.size() <= maxindex &&getLoader(maxindex) != null) {
returnlookupCacheEnabled ? cache : null;
}
return null;
I think that is easier to understand.
I changed the code to this. What do you think?
if (loaders.size() <= maxindex) {
boolean failed = false;
// Call getNextLoader() with a null cache to open all
// Loaders up to, and including, maxindex.
if (getNextLoader(null, maxindex) == null) {
failed = true;
} else if (!lookupCacheEnabled) {
// As a side effect of the getNextLoader call,
// The cache has been invalidated.
failed = true;
}
if (failed) {
if (DEBUG_LOOKUP_CACHE) {
System.out.println("Expanded loaders FAILED " +
loaders.size() + " for maxindex=" +
maxindex);
}
return null;
}
if (DEBUG_LOOKUP_CACHE) {
System.out.println("Expanded loaders " +
loaders.size() +
" to maxindex=" + maxindex);
}
}
432 urlNoFragString.equals(
433 URLUtil.urlNoFragString(lookupCacheURLs[index]))) {
Should you store URLUtil.urlNoFragString form of URL in
lookupCacheURLs such that they can be compared with equals?
urlNoFragString is called exactly once for every URL in the
lookupCacheURLs so they live very shortly. In contrast, the URLs stored
in the lookupCacheURLs live forever (HotSpot has an internal reference
to them and use them for other purposes). So for memory footprint, it's
better to not cache the urlNoFragString.
line 423-426: formatting nit: these lines look a little long
and would be good to rewrap them.
Fixed
jvm.h
1394: typo s/Retruns/Returns
As suggested above, pass the classloader instance instead of
defining a new loader_type interface
Fixed. Now the class loader instance is passed.
URLClassPath.c
line 42: nit: align the parameters
Fixed
There are several lines calling
49JNU_ThrowNullPointerException(env, 0).
55JNU_ThrowOutOfMemoryError(env, NULL);
The msg argument probably better to be consistent and pass NULL.
ClassLoader.c is a bit inconsistent.
Fixed.
Can you add regression tests in the jdk repo? Sanity tests
like with the lookup cache enabled but invalidated at startup
or due to addURL call.
Added.
Mandy
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 2)
Hi Jiangli, Thanks for the review. Please see comments in-line: On 10/21/14, 2:04 PM, Jiangli Zhou wrote: Hi Ioi, Here are some comments from me: -src/share/vm/classfile/classLoader.cpp In ClassLoader::setup_search_path, if canonicalize is true it's not necessary to allocate the 'path' and copy the 'class_path' to 'path'. we need to allocate path[] because we need to copy only a part of the classpath[] into it. E.g. classpath[] = "foo.jar:bar.jar" path[] = "foo.jar" <- need to be zero-terminated -src/share/vm/memory/metadataFactory.hpp In free_array, is it possible to disable it only when PrintSharedSpaces is enabled, instead of disabling it for all? I am not sure. For safety, I have filed a bug and will fix that separately: https://bugs.openjdk.java.net/browse/JDK-8062016 -jdk/src/share/native/sun/misc/URLClassPath.c Is verifying class name needed in Java_sun_misc_URLClassPath_knownToNotExist0? The class name will be verified when the class loader loads the class. I coped the code from Java_java_lang_ClassLoader_findBootstrapClass. VerifyFixClassname is needed, as it translate "." -> "/". I am not sure if VerifyClassname is needed, but I guess it doesn't hurt. -jdk/src/share/classes/sun/misc/URLClassPath.java Can lookupCacheEnabled flag change during runtime? In getLookupCache() the flag is checked more than once. Yes, it can change if disableAllLookupCaches() is called in the middle of getLookupCache(). I have added more comments in the new webrev. Please take a look to see if it makes sense. http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v2/ Thanks - Ioi Thanks, Jiangli On 10/21/2014 10:27 AM, Ioi Lam wrote: Please review this fix: http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v1/ Bug: Add an interface to the JVM's Class/Resource Lookup Index Cache for improving sun.misc.URLClassPath search time https://bugs.openjdk.java.net/browse/JDK-8061651 Summary of fix: Some J2EE environments have lots of JAR files in the classpath. However, during run-time, most classes are loaded by custom classloaders which delegate to the system loader. As a result, each class loaded by a custom classloader will cause many JAR search failures (in sun.misc.URLClassPath). To limit the number of searches in URLClassPath, the 8uX HotSpot VM is going to include a Class/Resource Lookup Index Cache that records information about the class/resource entries in the JAR files in the classpath. This can be used to improve sun.misc.URLClassPath search time. This is part 1 of the REF -- we are adding appropriate interfaces in both the JDK code and the HotSpot code to access this cache. URLClassPath.java is modified to use JNI to call into the HotSpot JVM_XXX APIs to access the cache. The implementation of this cache is done in part 2 of this RFE. Note that this enhancement is for JDK 8uX only. In JDK9 and beyond, we are considering an alternative implementation where the cache is maintained in the core libs code, outside of HotSpot. Tests: JPRT Adhoc SQE tests Thanks - Ioi
Re: Review request: JDK-8055723 Replace concat String to append in StringBuilder parameters
Thank you Ulf.
I removed the fix in toString method and in debug classes:
http://cr.openjdk.java.net/~weijun/8055723/webrev.00/
On Mon, Oct 20, 2014 at 10:26 PM, Ulf Zibis wrote:
>
> Am 21.10.2014 um 01:02 schrieb Otávio Gonçalves de Santana:
>
>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8055723
>>
>>
>> WEBREV: http://cr.openjdk.java.net/~weijun/8055723/client/webrev.02/
>> WEBREV: http://cr.openjdk.java.net/~weijun/8055723/core/webrev.03/
>>
>
> I did not look through all sources.
> In Scanner.java I discovered:
> 1307 sb.append("[delimiters=").append(delimPattern).append(']');
> 1308 sb.append("[position=").append(position).append(']');
> ...
> Maybe better:
> 1307 sb.append("[delimiters=").append(delimPattern);
> 1308 sb.append("][position=").append(position);
> ...
>
> -Ulf
>
>
--
Otávio Gonçalves de Santana
blog: http://otaviosantana.blogspot.com.br/
twitter: http://twitter.com/otaviojava
site: *http://about.me/otaviojava *
55 (11) 98255-3513
