Re: RFR: 8252933: com.sun.tools.jdi.ObjectReferenceImpl#validateAssignment always requests referenceType
On Fri, 11 Sep 2020 01:08:53 GMT, Daniil Titov wrote: > The change fixes the regression introduced by > https://bugs.openjdk.java.net/browse/JDK-8241080. > > Method validateAssignment() in com.sun.tools.jdi.ObjectReferenceImpl now > always retrieves the reference type and that > requires one more JDWP packet if the value is not cached yet. Before > https://bugs.openjdk.java.net/browse/JDK-8241080 > this happened for arrays only. Testing: tier1-tier3 tests passes. The changes look good to me. Can I ask how you noticed the extra unnecessary JDWP packet? - Marked as reviewed by cjplummer (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/124
Re: RFR: 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… [v2]
On Fri, 11 Sep 2020 02:53:51 GMT, Leonid Mesnik wrote: >> It is not loom-specific and reproduced n jdk/jdk with >> -XX:+CheckUnhandledOops. > > What do you think about moving > Handle obj = ThreadService::get_current_contended_monitor(thread); > out of scope of block_object oop visibility? > It is my second patch. I'm missing something. How can a NULL oop get corrupted even if there is a GC? - PR: https://git.openjdk.java.net/jdk/pull/123
Re: RFR: 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… [v2]
On Fri, 11 Sep 2020 02:52:15 GMT, Leonid Mesnik wrote: >> I can't see anywhere a safepoint check would occur in that code. This issue >> was flagged as being in Loom so perhaps the >> loom code is different and is what introduces the safepoint check? But I >> agree with Coleen that the best solution is to >> just use Handles. > > It is not loom-specific and reproduced n jdk/jdk with -XX:+CheckUnhandledOops. What do you think about moving Handle obj = ThreadService::get_current_contended_monitor(thread); out of scope of block_object oop visibility? It is my second patch. - PR: https://git.openjdk.java.net/jdk/pull/123
Re: RFR: 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… [v2]
On Fri, 11 Sep 2020 02:49:04 GMT, David Holmes wrote:
>> src/hotspot/share/services/threadService.cpp line 888:
>>
>>> 886: _thread_status == java_lang_Thread::IN_OBJECT_WAIT_TIMED) {
>>> 887:
>>> 888: Handle obj = ThreadService::get_current_contended_monitor(thread);
>>
>> There must be a safepoint here then.
>> I think this would be better and safer if blocker_object and
>> blocker_object_owner are Handles. Can you change them to
>> Handles?
>
> I can't see anywhere a safepoint check would occur in that code. This issue
> was flagged as being in Loom so perhaps the
> loom code is different and is what introduces the safepoint check? But I
> agree with Coleen that the best solution is to
> just use Handles.
It is not loom-specific and reproduced n jdk/jdk with -XX:+CheckUnhandledOops.
-
PR: https://git.openjdk.java.net/jdk/pull/123
Re: RFR: 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… [v2]
On Fri, 11 Sep 2020 02:53:51 GMT, Leonid Mesnik wrote: >> The NULL oops are corrupted by CheckUnhandledOops and should be re-written >> with NULL to pass testing >> with -XX:+CheckUnhandledOops. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize(...) Changes requested by dholmes (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/123
Re: RFR: 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… [v2]
> The NULL oops are corrupted by CheckUnhandledOops and should be re-written > with NULL to pass testing > with -XX:+CheckUnhandledOops. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize(...) - Changes: - all: https://git.openjdk.java.net/jdk/pull/123/files - new: https://git.openjdk.java.net/jdk/pull/123/files/0db863a3..c89edef2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=123=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=123=00-01 Stats: 6 lines in 1 file changed: 2 ins; 4 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/123.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/123/head:pull/123 PR: https://git.openjdk.java.net/jdk/pull/123
Re: RFR: 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… [v2]
On Fri, 11 Sep 2020 00:19:47 GMT, Coleen Phillimore wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize(...)
>
> src/hotspot/share/services/threadService.cpp line 888:
>
>> 886: _thread_status == java_lang_Thread::IN_OBJECT_WAIT_TIMED) {
>> 887:
>> 888: Handle obj = ThreadService::get_current_contended_monitor(thread);
>
> There must be a safepoint here then.
> I think this would be better and safer if blocker_object and
> blocker_object_owner are Handles. Can you change them to
> Handles?
I can't see anywhere a safepoint check would occur in that code. This issue was
flagged as being in Loom so perhaps the
loom code is different and is what introduces the safepoint check? But I agree
with Coleen that the best solution is to
just use Handles.
-
PR: https://git.openjdk.java.net/jdk/pull/123
Integrated: 8252406: Introduce Thread::as_Java_thread() convenience function
On Mon, 7 Sep 2020 05:56:14 GMT, David Holmes wrote:
> This is a rather large but generally simple cleanup.
>
> We use a lot of raw C-style casts to "(JavaThread*)" typically after checking
> "thread->is_Java_thread()". To simplify
> this pattern, and make the code look somewhat cleaner we introduce a
> convenience function Thread::as_Java_thread(),
> which asserts is_Java_thread() and returns "this" cast to "JavaThread*". A
> const version, as_const_Java_thread(), was
> also added to allow use in cases where we start with "const Thread *".
> Summary of changes:
> - changed raw casts to use as_Java_thread()
> - removed redundant checks of is_Java_thread() as it is now done in
> as_Java_thread()
> - Removed checks that are checking the type system e.g.
> void foo(JavaThread* t) {
> assert(t->is_Java_thread(), "must be")
> the only way the assert can fire is if someone deliberately bypasses the type
> system, and if we are going to worry
> about that then we would need asserts like this on every method that expects
> a JavaThread! The right place for the
> assertion is at the head of any such call chain.
> - Removed asserts that are already guaranteed in the caller.
> - Use JavaThread::current() where appropriate.
> - Use pre-existing "thread" variable where available rather than casting
> THREAD
>
> Change locations found by grepping for variations of the cast syntax
> "(JavaThread*)" - it is possible some may have
> been missed.
> Testing: tiers 1-3
>
> Thanks,
> David
This pull request has now been integrated.
Changeset: 976acdde
Author:David Holmes
URL: https://git.openjdk.java.net/jdk/commit/976acdde
Stats: 476 lines in 110 files changed: 116 ins; 20 del; 340 mod
8252406: Introduce Thread::as_Java_thread() convenience function
Reviewed-by: shade, coleenp, kbarrett, dcubed
-
PR: https://git.openjdk.java.net/jdk/pull/37
RFR: 8252933: com.sun.tools.jdi.ObjectReferenceImpl#validateAssignment always requests referenceType
The change fixes the regression introduced by https://bugs.openjdk.java.net/browse/JDK-8241080. Method validateAssignment() in com.sun.tools.jdi.ObjectReferenceImpl now always retrieves the reference type and that requires one more JDWP packet if the value is not cached yet. Before https://bugs.openjdk.java.net/browse/JDK-8241080 this happened for arrays only. Testing: tier1-tier3 tests passes. - Commit messages: - 8252933: com.sun.tools.jdi.ObjectReferenceImpl#validateAssignment always requests referenceType Changes: https://git.openjdk.java.net/jdk/pull/124/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=124=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8252933 Stats: 6 lines in 1 file changed: 3 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/124.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/124/head:pull/124 PR: https://git.openjdk.java.net/jdk/pull/124
Re: RFR: 8252657: JVMTI agent is not unloaded when Agent_OnAttach is failed
On Sat, 5 Sep 2020 14:26:17 GMT, Yasumasa Suenaga wrote:
> If `Agent_OnAttach()` in JVMTI agent which is attempted to load via
> JVMTI.agent_load dcmd is failed, it would not be
> unloaded. We've [discussed it on
> serviceability-dev](https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-September/032839.html).
> This PR is
> a continuation of that. This PR also includes to call `Agent_OnUnload()`
> when `Agent_OnAttach()` failed.
>
> How to reproduce:
>
> 1. Build JVMTI agent for test
> $ git clone https://github.com/YaSuenag/jvmti-examples.git
> $ cd jvmti-examples/helloworld/out/build
> $ cmake ../..
>
> 2. Run JShell
>
> 3. Load JVMTI agent via `jcmd JVMTI.agent_load` with "error" ("error"
> means `Agent_OnAttach()` returns JNI_ERR)
> $ jcmd
> 89456 jdk.jshell.execution.RemoteExecutionControl 45651
> 89547 sun.tools.jcmd.JCmd
> 89436 jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider
> $ jcmd 89436 JVMTI.agent_load `pwd`/libhelloworld.so error
> 89436:
> return code: -1
>
> 4. Check loaded libraries via `jcmd VM.dynlibs`
> $ jcmd 89436 VM.dynlibs | grep libhelloworld
> 7f2f8b06b000-7f2f8b06c000 r--p fd:00 11818202
> /home/ysuenaga/github/jvmti-examples/helloworld/out/build/libhelloworld.so
> 7f2f8b06c000-7f2f8b06d000 r-xp 1000
> fd:00 11818202
> /home/ysuenaga/github/jvmti-examples/helloworld/out/build/libhelloworld.so
> 7f2f8b06d000-7f2f8b06e000
> r--p 2000 fd:00 11818202
> /home/ysuenaga/github/jvmti-examples/helloworld/out/build/libhelloworld.so
> 7f2f8b06e000-7f2f8b06f000 r--p 2000 fd:00 11818202
> /home/ysuenaga/github/jvmti-examples/helloworld/out/build/libhelloworld.so
> 7f2f8b06f000-7f2f8b07 rw-p 3000
> fd:00 11818202
> /home/ysuenaga/github/jvmti-examples/helloworld/out/build/libhelloworld.so
@edvbld Can you approve me to run tier1 tests with /test PR command again?
-
PR: https://git.openjdk.java.net/jdk/pull/19
RFR: 8252657: JVMTI agent is not unloaded when Agent_OnAttach is failed
If `Agent_OnAttach()` in JVMTI agent which is attempted to load via
JVMTI.agent_load dcmd is failed, it would not be
unloaded. We've [discussed it on
serviceability-dev](https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-September/032839.html).
This PR is
a continuation of that.
This PR also includes to call `Agent_OnUnload()` when `Agent_OnAttach()` failed.
How to reproduce:
1. Build JVMTI agent for test
$ git clone https://github.com/YaSuenag/jvmti-examples.git
$ cd jvmti-examples/helloworld/out/build
$ cmake ../..
2. Run JShell
3. Load JVMTI agent via `jcmd JVMTI.agent_load` with "error" ("error"
means `Agent_OnAttach()` returns JNI_ERR)
$ jcmd
89456 jdk.jshell.execution.RemoteExecutionControl 45651
89547 sun.tools.jcmd.JCmd
89436 jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider
$ jcmd 89436 JVMTI.agent_load `pwd`/libhelloworld.so error
89436:
return code: -1
4. Check loaded libraries via `jcmd VM.dynlibs`
$ jcmd 89436 VM.dynlibs | grep libhelloworld
7f2f8b06b000-7f2f8b06c000 r--p fd:00 11818202
/home/ysuenaga/github/jvmti-examples/helloworld/out/build/libhelloworld.so
7f2f8b06c000-7f2f8b06d000 r-xp 1000
fd:00 11818202
/home/ysuenaga/github/jvmti-examples/helloworld/out/build/libhelloworld.so
7f2f8b06d000-7f2f8b06e000
r--p 2000 fd:00 11818202
/home/ysuenaga/github/jvmti-examples/helloworld/out/build/libhelloworld.so
7f2f8b06e000-7f2f8b06f000 r--p 2000 fd:00 11818202
/home/ysuenaga/github/jvmti-examples/helloworld/out/build/libhelloworld.so
7f2f8b06f000-7f2f8b07 rw-p 3000
fd:00 11818202
/home/ysuenaga/github/jvmti-examples/helloworld/out/build/libhelloworld.so
-
Commit messages:
- JVMTI agent is not unloaded when Agent_OnAttach is failed
Changes: https://git.openjdk.java.net/jdk/pull/19/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=19=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8252657
Stats: 44 lines in 4 files changed: 32 ins; 6 del; 6 mod
Patch: https://git.openjdk.java.net/jdk/pull/19.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/19/head:pull/19
PR: https://git.openjdk.java.net/jdk/pull/19
Re: RFR: 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize…
On Thu, 10 Sep 2020 23:38:45 GMT, Leonid Mesnik wrote:
> The NULL oops are corrupted by CheckUnhandledOops and should be re-written
> with NULL to pass testing
> with -XX:+CheckUnhandledOops.
Changes requested by coleenp (Reviewer).
src/hotspot/share/services/threadService.cpp line 888:
> 886: _thread_status == java_lang_Thread::IN_OBJECT_WAIT_TIMED) {
> 887:
> 888: Handle obj = ThreadService::get_current_contended_monitor(thread);
There must be a safepoint here then.
I think this would be better and safer if blocker_object and
blocker_object_owner are Handles. Can you change them to
Handles?
-
PR: https://git.openjdk.java.net/jdk/pull/123
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Sun, 6 Sep 2020 13:57:19 GMT, Dmitriy Dumanskiy wrote: > **isEmpty** is faster + has less byte code + easier to read. Benchmarks could > be found > > [here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416). 1) This is un-necessary churn. 2) I can't even be sure I am finding the ones in my area because there's so much here 3) The ones I can find have no need of whatever performance improvement this might bring. I think this whole PR should be withdrawn, and there may an attempt at measuring the benefits of this for the various cases before submitting in appropriate smaller chunks. But I'll tell you now I don't see a need for the test updates you are making. - Changes requested by prr (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Thu, 10 Sep 2020 12:04:48 GMT, Dmitriy Dumanskiy wrote: > I have in mind dozens of improvements all over the code like this one. That sounds scary. Broad updates like these cause unnecessary churn in the codebase, and can make merging and back porting harder. Changes should be discussed ahead of time with the appropriate teams. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Thu, 10 Sep 2020 15:50:39 GMT, Alan Bateman wrote: >> 14 cc'd mailing lists is unworkable. You need to be subscribed to lists to >> post, but even if you are a reply-all will >> be delayed due to all of the mails being held up for moderator approval due >> to: >> " Too many recipients to the message" >> >> I have a longer email coming once it gets through the moderator approval >> delay. :( > > Patches that do global replace are always awkward. In this case, there are > 150 files changed and most seem to be tests > or changes to tools used in the build (includes > src/hotspot/share/prims/jvmtiEnvFill.java). If these are dropped from > the patch then it leaves just 43 files, many of which are from 3rd party code > that can also be dropped. This should > reduce the patch down to a manageable 24 or so files that should be trivial > to review. Since one of the motivations for this change is micro benchmark performance, please add a benchmark to the JDKs micro benchmark suite as well. (see e.g. https://github.com/openjdk/jdk/tree/master/test/micro/org/openjdk/bench/java/lang). Also, what testing has been done for these changes? - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Thu, 10 Sep 2020 13:53:10 GMT, David Holmes wrote: >> @dholmes-ora raises a good point. Hopefully there is a balance point between >> a dozen different bugs / pull requests >> each targeting one area and one bug / PR targeting a dozen separate areas. I >> don't have a good general rule to suggest. >> Maybe @AlanBateman or @jddarcy can offer some advice? > > 14 cc'd mailing lists is unworkable. You need to be subscribed to lists to > post, but even if you are a reply-all will > be delayed due to all of the mails being held up for moderator approval due > to: > " Too many recipients to the message" > > I have a longer email coming once it gets through the moderator approval > delay. :( Patches that do global replace are always awkward. In this case, there are 150 files changed and most seem to be tests or changes to tools used in the build (includes src/hotspot/share/prims/jvmtiEnvFill.java). If these are dropped from the patch then it leaves just 43 files, many of which are from 3rd party code that can also be dropped. This should reduce the patch down to a manageable 24 or so files that should be trivial to review. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252997: Null-proofing for linker_md.c
Also this fix needs to be reviewed by the Serviceability team. I've moved hotspot-dev@... to Bcc and added serviceability-dev@... Dan On 9/10/20 5:53 AM, Severin Gehwolf wrote: Hi Adam, jdk/jdk moved to skara[1]. Please re-create the patch as a github PR (or with the skara CLI tooling). Thanks, Severin [1] https://mail.openjdk.java.net/pipermail/jdk-dev/2020-September/004694.html On Thu, 2020-09-10 at 10:11 +0100, Adam Farley8 wrote: Hi All, Small change proposed here to protect against null return values from jvmtiAllocation. Requesting reviews and sponsor, please. Bug: https://bugs.openjdk.java.net/browse/JDK-8252997 Webrev: http://cr.openjdk.java.net/~afarley/8252997/webrev/ Best Regards Adam Farley IBM Runtimes 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: 8252406: Introduce Thread::as_Java_thread() convenience function [v8]
On Thu, 10 Sep 2020 03:18:23 GMT, David Holmes wrote:
>> This is a rather large but generally simple cleanup.
>>
>> We use a lot of raw C-style casts to "(JavaThread*)" typically after
>> checking "thread->is_Java_thread()". To simplify
>> this pattern, and make the code look somewhat cleaner we introduce a
>> convenience function Thread::as_Java_thread(),
>> which asserts is_Java_thread() and returns "this" cast to "JavaThread*". A
>> const version, as_const_Java_thread(), was
>> also added to allow use in cases where we start with "const Thread *".
>> Summary of changes:
>> - changed raw casts to use as_Java_thread()
>> - removed redundant checks of is_Java_thread() as it is now done in
>> as_Java_thread()
>> - Removed checks that are checking the type system e.g.
>> void foo(JavaThread* t) {
>> assert(t->is_Java_thread(), "must be")
>> the only way the assert can fire is if someone deliberately bypasses the
>> type system, and if we are going to worry
>> about that then we would need asserts like this on every method that expects
>> a JavaThread! The right place for the
>> assertion is at the head of any such call chain.
>> - Removed asserts that are already guaranteed in the caller.
>> - Use JavaThread::current() where appropriate.
>> - Use pre-existing "thread" variable where available rather than casting
>> THREAD
>>
>> Change locations found by grepping for variations of the cast syntax
>> "(JavaThread*)" - it is possible some may have
>> been missed.
>> Testing: tiers 1-3
>>
>> Thanks,
>> David
>
> David Holmes has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed serious bug in dependencies.cpp. The v1 change was wrong and got
> restored but
> not commited, then a later rollback to v1 lost the fix.
>
> Minor issues Dan spotted now fixed.
Incremental looks good to me.
-
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/37
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On 10/09/2020 10:07 pm, Dmitriy Dumanskiy wrote: On Thu, 10 Sep 2020 11:21:28 GMT, David Holmes wrote: The code in java.base was updated to use String::isEmpty in JDK 12 (JDK-8215281). There was follow-up in JDK 13 to do the same in the java.desktop module (JDK-8223237). Changing the remaining usages make sense although I see that more more than half are in tests. It would be good to hear from security-dev on the changes to the Apache Santuario code (in java.xml.crypto module) in case it would be better to contribute those upstream instead. Ditto for the Apache Xalan code (in the java.xml module) but it may be significantly forked already that it doesn't matter. I assume you can use JDK-8215014 rather than creating a new issue. This should be broken up to deal with the files in different functional areas under different bugids and PRs. Otherwise the cross-posting to so many lists is prohibitive. Files in different areas need to be reviewed by different teams. Thank you. I have in mind dozens of improvements all over the code like this one. I already did some further review and in most cases, those tiny changes go trough all codebase. I can create PR for every separate module, but in general, it would multiply x5 the number of PRs in total. If you think it's a better way to go, no problem, I can do that. Find a reasonable middle ground. You have around 14 mailing lists cc'd here, for changes on 150 files. It is very unlikely anyone will review all 150, and files in different areas are, by convention, reviewed by Reviewers for those areas. Even if people only look at a subset of files, communicating that to you effectively so you can figure out when all 150 have been reviewed, is not very practical. My initial breakdown would be: - build - desktop (awt/swing/2d) - serviceability/jmx (the SA and JVMTI changes) - security - core-libs Also note that while the original PR email went out to 14 lists, most people trying to reply-all won't be able to as they don't subscribe to all 14 lists, so the review threads will get very fragmented. Cheers, David - - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Thu, 10 Sep 2020 12:18:51 GMT, Kevin Rushforth wrote: >> This should be broken up to deal with the files in different functional >> areas under different bugids and PRs. Otherwise >> the cross-posting to so many lists is prohibitive. Files in different areas >> need to be reviewed by different teams. >> Thank you. > > @dholmes-ora raises a good point. Hopefully there is a balance point between > a dozen different bugs / pull requests > each targeting one area and one bug / PR targeting a dozen separate areas. I > don't have a good general rule to suggest. > Maybe @AlanBateman or @jddarcy can offer some advice? 14 cc'd mailing lists is unworkable. You need to be subscribed to lists to post, but even if you are a reply-all will be delayed due to all of the mails being held up for moderator approval due to: " Too many recipients to the message" I have a longer email coming once it gets through the moderator approval delay. :( - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Thu, 10 Sep 2020 11:21:28 GMT, David Holmes wrote: >> The code in java.base was updated to use String::isEmpty in JDK 12 >> (JDK-8215281). There was follow-up in JDK 13 to do >> the same in the java.desktop module (JDK-8223237). Changing the remaining >> usages make sense although I see that more >> more than half are in tests. It would be good to hear from security-dev on >> the changes to the Apache Santuario code >> (in java.xml.crypto module) in case it would be better to contribute those >> upstream instead. Ditto for the Apache Xalan >> code (in the java.xml module) but it may be significantly forked already >> that it doesn't matter. I assume you can use >> JDK-8215014 rather than creating a new issue. > > This should be broken up to deal with the files in different functional areas > under different bugids and PRs. Otherwise > the cross-posting to so many lists is prohibitive. Files in different areas > need to be reviewed by different teams. > Thank you. @dholmes-ora raises a good point. Hopefully there is a balance point between a dozen different bugs / pull requests each targeting one area and one bug / PR targeting a dozen separate areas. I don't have a good general rule to suggest. Maybe @AlanBateman or @jddarcy can offer some advice? - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Thu, 10 Sep 2020 11:21:28 GMT, David Holmes wrote: >> The code in java.base was updated to use String::isEmpty in JDK 12 >> (JDK-8215281). There was follow-up in JDK 13 to do >> the same in the java.desktop module (JDK-8223237). Changing the remaining >> usages make sense although I see that more >> more than half are in tests. It would be good to hear from security-dev on >> the changes to the Apache Santuario code >> (in java.xml.crypto module) in case it would be better to contribute those >> upstream instead. Ditto for the Apache Xalan >> code (in the java.xml module) but it may be significantly forked already >> that it doesn't matter. I assume you can use >> JDK-8215014 rather than creating a new issue. > > This should be broken up to deal with the files in different functional areas > under different bugids and PRs. Otherwise > the cross-posting to so many lists is prohibitive. Files in different areas > need to be reviewed by different teams. > Thank you. I have in mind dozens of improvements all over the code like this one. I already did some further review and in most cases, those tiny changes go trough all codebase. I can create PR for every separate module, but in general, it would multiply x5 the number of PRs in total. If you think it's a better way to go, no problem, I can do that. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Thu, 10 Sep 2020 10:40:15 GMT, Alan Bateman wrote: >> @kevinrushforth thanks. Done. >> >> Similar issues: >> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8215014 >> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8251246 >> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8223237 >> >> could be joined somehow? > > The code in java.base was updated to use String::isEmpty in JDK 12 > (JDK-8215281). There was follow-up in JDK 13 to do > the same in the java.desktop module (JDK-8223237). Changing the remaining > usages make sense although I see that more > more than half are in tests. It would be good to hear from security-dev on > the changes to the Apache Santuario code > (in java.xml.crypto module) in case it would be better to contribute those > upstream instead. Ditto for the Apache Xalan > code (in the java.xml module) but it may be significantly forked already that > it doesn't matter. I assume you can use > JDK-8215014 rather than creating a new issue. This should be broken up to deal with the files in different functional areas under different bugids and PRs. Otherwise the cross-posting to so many lists is prohibitive. Files in different areas need to be reviewed by different teams. Thank you. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Thu, 10 Sep 2020 08:47:35 GMT, Dmitriy Dumanskiy wrote: >> Before this Enhancement can be formally reviewed, you will need a JBS bug >> ID. If you are already working with a >> Committer or Reviewer in the `jdk` project who has agreed to sponsor your >> change, they can file the Enhancement >> request. Otherwise, you can file it using the web interface at >> [bugreport.java.com](https://bugreport.java.com/). Once >> you have a JBS bug ID, you need to edit the PR title to include that bug ID >> (without the `JDK-`) replacing >> "Improvement". Since this PR cuts across many functional areas (each gray >> label represents a functional area), you >> should expect a longer review process, since someone from each functional >> area will need to look at the changes in >> their area (like @mrserb started to do for the `2d` area). > > @kevinrushforth thanks. Done. > > Similar issues: > https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8215014 > https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8251246 > https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8223237 > > could be joined somehow? The code in java.base was updated to use String::isEmpty in JDK 12 (JDK-8215281). There was follow-up in JDK 13 to do the same in the java.desktop module (JDK-8223237). Changing the remaining usages make sense although I see that more more than half are in tests. It would be good to hear from security-dev on the changes to the Apache Santuario code (in java.xml.crypto module) in case it would be better to contribute those upstream instead. Ditto for the Apache Xalan code (in the java.xml module) but it may be significantly forked already that it doesn't matter. I assume you can use JDK-8215014 rather than creating a new issue. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252105: parallel heap inspection for ZCollectedHeap
On Thu, 10 Sep 2020 09:33:28 GMT, Per Lidén wrote: >> - enable parallel heap inspection for ZCollectedHeap >> - preliminary evaluation: >> Time of jmap histo on 8GB heap with ~5GB objects >> * before: 7.103s >> * after : 2.734s (with 4 parallel threads) > > Just looked at this briefly. My initial comment is that we need to avoid all > the code duplicated from ZHeapIterator and > isolate all this a bit better. Perhaps folding everything into one iterator, > that can be used by both object_iterate() > and parallel_object_iterator(). I'll take a closer look, and perhaps try some > alternatives, when I get a chance. Hi @pliden, Thanks for your comments, I will try to merge ZHeapIterator and ZHeapParIterator and update the pr then. -Lin - PR: https://git.openjdk.java.net/jdk/pull/103
Re: RFR: 8252105: parallel heap inspection for ZCollectedHeap
On Thu, 10 Sep 2020 02:28:43 GMT, Lin Zang wrote: > - enable parallel heap inspection for ZCollectedHeap > - preliminary evaluation: > Time of jmap histo on 8GB heap with ~5GB objects > * before: 7.103s > * after : 2.734s (with 4 parallel threads) Just looked at this briefly. My initial comment is that we need to avoid all the code duplicated from ZHeapIterator and isolate all this a bit better. Perhaps folding everything into one iterator, that can be used by both object_iterate() and parallel_object_iterator(). I'll take a closer look, and perhaps try some alternatives, when I get a chance. - PR: https://git.openjdk.java.net/jdk/pull/103
Re: RFR: 8252999: replace all String.equals("") usages with String.isEmpty()
On Wed, 9 Sep 2020 20:21:44 GMT, Kevin Rushforth wrote: >> @mrserb hello. Thanks for the review. Any further actions required from me? > > Before this Enhancement can be formally reviewed, you will need a JBS bug ID. > If you are already working with a > Committer or Reviewer in the `jdk` project who has agreed to sponsor your > change, they can file the Enhancement > request. Otherwise, you can file it using the web interface at > [bugreport.java.com](https://bugreport.java.com/). Once > you have a JBS bug ID, you need to edit the PR title to include that bug ID > (without the `JDK-`) replacing > "Improvement". Since this PR cuts across many functional areas (each gray > label represents a functional area), you > should expect a longer review process, since someone from each functional > area will need to look at the changes in > their area (like @mrserb started to do for the `2d` area). @kevinrushforth thanks. Done. Similar issues: https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8215014 https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8251246 https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8223237 could be joined somehow? - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: replace all String.equals("") usages with String.isEmpty()
On Wed, 9 Sep 2020 07:57:48 GMT, Dmitriy Dumanskiy wrote: >> @doom369 jcheck requires an associated issue > > @mrserb hello. Thanks for the review. Any further actions required from me? Before this Enhancement can be formally reviewed, you will need a JBS bug ID. If you are already working with a Committer or Reviewer in the `jdk` project who has agreed to sponsor your change, they can file the Enhancement request. Otherwise, you can file it using the web interface at [bugreport.java.com](https://bugreport.java.com/). Once you have a JBS bug ID, you need to edit the PR title to include that bug ID (without the `JDK-`) replacing "Improvement". Since this PR cuts across many functional areas (each gray label represents a functional area), you should expect a longer review process, since someone from each functional area will need to look at the changes in their area (like @mrserb started to do for the `2d` area). - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: replace all String.equals("") usages with String.isEmpty()
On Sun, 6 Sep 2020 13:57:19 GMT, Dmitriy Dumanskiy
wrote:
> **isEmpty** is faster + has less byte code + easier to read. Benchmarks could
> be found
>
> [here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416).
src/demo/share/java2d/J2DBench/src/j2dbench/tests/iio/InputImageTests.java line
187:
> 185: String format = spi.getFormatNames()[0].toLowerCase();
> 186: String suffix = spi.getFileSuffixes()[0].toLowerCase();
> 187: if (suffix == null || suffix.equals("")) {
This file intentionally maintains compatibility to jdk1.4, so equals("") should
be used.
src/demo/share/java2d/J2DBench/src/j2dbench/tests/iio/OutputImageTests.java
line 146:
> 144: String format = spi.getFormatNames()[0].toLowerCase();
> 145: String suffix = spi.getFileSuffixes()[0].toLowerCase();
> 146: if (suffix == null || suffix.equals("")) {
This file intentionally maintains compatibility to jdk1.4, so equals("") should
be used.
-
PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: replace all String.equals("") usages with String.isEmpty()
On Sun, 6 Sep 2020 18:08:15 GMT, thatsIch wrote: >> **isEmpty** is faster + has less byte code + easier to read. Benchmarks >> could be found >> >> [here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416). > > @doom369 jcheck requires an associated issue @mrserb hello. Thanks for the review. Any further actions required from me? - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: replace all String.equals("") usages with String.isEmpty()
On Sun, 6 Sep 2020 13:57:19 GMT, Dmitriy Dumanskiy wrote: > **isEmpty** is faster + has less byte code + easier to read. Benchmarks could > be found > > [here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416). @doom369 jcheck requires an associated issue - PR: https://git.openjdk.java.net/jdk/pull/29
RFR: 8252999: replace all String.equals("") usages with String.isEmpty()
**isEmpty** is faster + has less byte code + easier to read. Benchmarks could
be found
[here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416).
-
Commit messages:
- Merge branch 'master' of https://github.com/doom369/jdk into
reaplce_equals_with_is_empty
- revert change in classes that maintain jdk 1.4 compatibility
- Improvement: replace all occurrences of the .equals("") usages with
.isEmpty()
Changes: https://git.openjdk.java.net/jdk/pull/29/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=29=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8252999
Stats: 234 lines in 150 files changed: 0 ins; 0 del; 234 mod
Patch: https://git.openjdk.java.net/jdk/pull/29.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/29/head:pull/29
PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252406: Introduce Thread::as_Java_thread() convenience function [v8]
On Thu, 10 Sep 2020 03:18:23 GMT, David Holmes wrote:
>> This is a rather large but generally simple cleanup.
>>
>> We use a lot of raw C-style casts to "(JavaThread*)" typically after
>> checking "thread->is_Java_thread()". To simplify
>> this pattern, and make the code look somewhat cleaner we introduce a
>> convenience function Thread::as_Java_thread(),
>> which asserts is_Java_thread() and returns "this" cast to "JavaThread*". A
>> const version, as_const_Java_thread(), was
>> also added to allow use in cases where we start with "const Thread *".
>> Summary of changes:
>> - changed raw casts to use as_Java_thread()
>> - removed redundant checks of is_Java_thread() as it is now done in
>> as_Java_thread()
>> - Removed checks that are checking the type system e.g.
>> void foo(JavaThread* t) {
>> assert(t->is_Java_thread(), "must be")
>> the only way the assert can fire is if someone deliberately bypasses the
>> type system, and if we are going to worry
>> about that then we would need asserts like this on every method that expects
>> a JavaThread! The right place for the
>> assertion is at the head of any such call chain.
>> - Removed asserts that are already guaranteed in the caller.
>> - Use JavaThread::current() where appropriate.
>> - Use pre-existing "thread" variable where available rather than casting
>> THREAD
>>
>> Change locations found by grepping for variations of the cast syntax
>> "(JavaThread*)" - it is possible some may have
>> been missed.
>> Testing: tiers 1-3
>>
>> Thanks,
>> David
>
> David Holmes has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed serious bug in dependencies.cpp. The v1 change was wrong and got
> restored but
> not commited, then a later rollback to v1 lost the fix.
>
> Minor issues Dan spotted now fixed.
Marked as reviewed by kbarrett (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/37
