8253278: Refactor/cleanup oopDesc::*_klass_addr #221

shipilev · 2020-09-17T08:37:26Z

Right now this one has the comment mentioning CMS:

Klass** oopDesc::klass_addr(HeapWord* mem) {
  // Only used internally and with CMS and will not work with
  // UseCompressedOops
  assert(!UseCompressedClassPointers, "only supported with uncompressed klass pointers");
  ByteSize offset = byte_offset_of(oopDesc, _metadata._klass);
  return (Klass**) (((char*)mem) + in_bytes(offset));
}

But that is not true, it is used on the generic paths.

The rest are the snowballing changes:

in load_klass_raw and set_klass: compressed_klass_addr and klass_addr are used where the direct access to _metadata can be done, avoiding these methods
the leftover use is in set_klass_release, where we can simplify things by inlining _addr methods directly
CHECK_SET_KLASS macro seems too much ceremony for just two asserts

Note that new code uses the universal klass_offset_in_bytes, that is technically offset_of(..., _metadata._klass). This is different from the previous code, but it matches what other code is doing -- it trusts that both _metadata._klass and _metadata._compressed_klass are on the same offsets. I thought to add a very paranoid assert there, but decided not to.

Testing:

Linux x86_64 fastdebug hotspot_gc_shenandoah
Linux x86_64 release hotspot_gc_shenandoah
Linux x86_64 fastdebug tier1
Linux x86_64 fastdebug tier2
Linux x86_64 fastdebug tier1, -XX:+UseShenandoahGC
Linux x86_64 fastdebug tier2, -XX:+UseShenandoahGC

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8253278: Refactor/cleanup oopDesc::*_klass_addr

Reviewers

Coleen Phillimore (@coleenp - Reviewer)
Stefan Karlsson (@stefank - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/221/head:pull/221
$ git checkout pull/221

bridgekeeper · 2020-09-17T08:38:26Z

👋 Welcome back shade! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2020-09-17T08:39:59Z

@shipilev The following label will be automatically applied to this pull request: hotspot.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label (add|remove) "label" command.

mlbridge · 2020-09-17T08:44:08Z

Webrevs

00: Full (c9c2ae9)

shipilev · 2020-09-17T08:54:09Z

/test tier1

openjdk · 2020-09-17T08:54:40Z

@shipilev you need to get approval to run the tests in tier1 for commits up until c9c2ae9

shipilev · 2020-09-17T10:04:46Z

objdumped MemAllocator::finish that uses oop::set_klass_release, and it has no differences. Looks like this before/after:

0000000000000000 <_ZNK12MemAllocator6finishEPP12HeapWordImpl>:
       0:       f3 0f 1e fa             endbr64 
       4:       48 8b 0d 00 00 00 00    mov    0x0(%rip),%rcx        # b <_ZNK12MemAllocator6finishEPP12HeapWordImpl+0xb>
       b:       48 89 f0                mov    %rsi,%rax
       e:       ba 01 00 00 00          mov    $0x1,%edx
      13:       80 39 00                cmpb   $0x0,(%rcx)
      16:       74 0b                   je     23 <_ZNK12MemAllocator6finishEPP12HeapWordImpl+0x23>
      18:       48 8b 57 10             mov    0x10(%rdi),%rdx
      1c:       48 8b 92 b8 00 00 00    mov    0xb8(%rdx),%rdx
      23:       48 8b 0d 00 00 00 00    mov    0x0(%rip),%rcx        # 2a <_ZNK12MemAllocator6finishEPP12HeapWordImpl+0x2a>
      2a:       48 89 10                mov    %rdx,(%rax)
      2d:       48 8b 57 10             mov    0x10(%rdi),%rdx
      31:       80 39 00                cmpb   $0x0,(%rcx)
      34:       74 1a                   je     50 <_ZNK12MemAllocator6finishEPP12HeapWordImpl+0x50>
      36:       48 8b 0d 00 00 00 00    mov    0x0(%rip),%rcx        # 3d <_ZNK12MemAllocator6finishEPP12HeapWordImpl+0x3d>
      3d:       48 2b 11                sub    (%rcx),%rdx
      40:       8b 49 08                mov    0x8(%rcx),%ecx
      43:       48 d3 ea                shr    %cl,%rdx
      46:       89 50 08                mov    %edx,0x8(%rax)
      49:       c3                      retq   
      4a:       66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)
      50:       48 89 50 08             mov    %rdx,0x8(%rax)
      54:       c3                      retq   
      55:       90                      nop
      56:       66 2e 0f 1f 84 00 00    nopw   %cs:0x0(%rax,%rax,1)
      5d:       00 00 00

coleenp

Nice cleanup!! I'm happy to see klass_addr go. It took me a while to reconcile that klass_offset_in_bytes points to _metadata._compressed_klass but I believe it's correct, and used everywhere else so it better be.

openjdk · 2020-09-17T11:48:14Z

@shipilev This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8253278: Refactor/cleanup oopDesc::*_klass_addr

Reviewed-by: coleenp, stefank

If you would like to add a summary, use the /summary command.
To credit additional contributors, use the /contributor command.
To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 19 commits pushed to the master branch:

73c9088: 8249451: Unconditional exceptions clearing logic in compiler code should honor Async Exceptions.
3ef2efb: 8252721: Nested classes in Swing APIs rely on default constructors
fd380d7: 8253274: The CycleDMImagetest brokes the system
2c3a37c: 8253314: precompiled.hpp missing from vmIntrinsics.cpp
d4269fd: 8253028: SA core file tests still time out on OSX with "java.io.IOException: App waiting timeout"
6c3e483: 8253313: xmlstream.hpp missing from vmIntrinsics.cpp
0a1dc50: 8253271: vm_version_x86.hpp should not include globals_extension.hpp
1e39649: 8243066: Move VM_INTRINSICS_DO into its own vmIntrinsics.hpp file
12dfe1c: 8253262: Allocate in DumpRegion is not thread safe
3570f5a: 8252041: G1: Fix incorrect uses of HeapRegionManager::max_length
... and 9 more: https://git.openjdk.java.net/jdk/compare/b87a1599674b8a73f489330decf051d1be9e9fba...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 73c9088b81775c579789d96069c2f59364841e0b.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

shipilev · 2020-09-18T05:40:47Z

/reviewers 2

shipilev · 2020-09-18T05:41:00Z

It is a sensitive code, I'd like another reviewer.

openjdk · 2020-09-18T05:41:24Z

@shipilev
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

shipilev · 2020-09-18T08:22:24Z

/integrate

openjdk · 2020-09-18T08:23:24Z

@shipilev Since your change was applied there have been 19 commits pushed to the master branch:

73c9088: 8249451: Unconditional exceptions clearing logic in compiler code should honor Async Exceptions.
3ef2efb: 8252721: Nested classes in Swing APIs rely on default constructors
fd380d7: 8253274: The CycleDMImagetest brokes the system
2c3a37c: 8253314: precompiled.hpp missing from vmIntrinsics.cpp
d4269fd: 8253028: SA core file tests still time out on OSX with "java.io.IOException: App waiting timeout"
6c3e483: 8253313: xmlstream.hpp missing from vmIntrinsics.cpp
0a1dc50: 8253271: vm_version_x86.hpp should not include globals_extension.hpp
1e39649: 8243066: Move VM_INTRINSICS_DO into its own vmIntrinsics.hpp file
12dfe1c: 8253262: Allocate in DumpRegion is not thread safe
3570f5a: 8252041: G1: Fix incorrect uses of HeapRegionManager::max_length
... and 9 more: https://git.openjdk.java.net/jdk/compare/b87a1599674b8a73f489330decf051d1be9e9fba...master

Your commit was automatically rebased without conflicts.

Pushed as commit 11c4ea9.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

coleenp · 2020-09-18T12:32:43Z

It is a sensitive code, I'd like another reviewer.

Yes, I didn't say it was trivial so yes, you needed another reviewer, which I see you got. Also, there should be an approximate 24 hr turnaround for PRs so that all the timezones have a chance to review it.

shipilev · 2020-09-18T12:35:02Z

It is a sensitive code, I'd like another reviewer.

Yes, I didn't say it was trivial so yes, you needed another reviewer, which I see you got. Also, there should be an approximate 24 hr turnaround for PRs so that all the timezones have a chance to review it.

Asked Skara folks to implemented this recently: https://bugs.openjdk.java.net/browse/SKARA-678

shipilev added 3 commits September 17, 2020 09:43

8253278: Refactor/cleanup oopDesc::*_klass_addr

d353261

Simplify it even more

6d1b5d4

Whitespace

c9c2ae9

openjdk bot added the rfr Pull request is ready for review label Sep 17, 2020

openjdk bot added the hotspot hotspot-dev@openjdk.org label Sep 17, 2020

openjdk bot added the test-request label Sep 17, 2020

coleenp approved these changes Sep 17, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 17, 2020

openjdk bot removed the ready Pull request is ready to be integrated label Sep 18, 2020

stefank approved these changes Sep 18, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 18, 2020

openjdk bot closed this Sep 18, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated labels Sep 18, 2020

openjdk bot removed the rfr Pull request is ready for review label Sep 18, 2020

shipilev deleted the JDK-8253278-cleanup-klass-addr branch September 18, 2020 08:55

hns mentioned this pull request Jan 13, 2021

JDK-8259283: use new HtmlId and HtmlIds classes #1951

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8253278: Refactor/cleanup oopDesc::*_klass_addr #221

8253278: Refactor/cleanup oopDesc::*_klass_addr #221

shipilev commented Sep 17, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 17, 2020

openjdk bot commented Sep 17, 2020

mlbridge bot commented Sep 17, 2020

shipilev commented Sep 17, 2020

openjdk bot commented Sep 17, 2020

shipilev commented Sep 17, 2020 •

edited

Loading

coleenp left a comment

openjdk bot commented Sep 17, 2020 •

edited

Loading

shipilev commented Sep 18, 2020

shipilev commented Sep 18, 2020

openjdk bot commented Sep 18, 2020

shipilev commented Sep 18, 2020

openjdk bot commented Sep 18, 2020

coleenp commented Sep 18, 2020

shipilev commented Sep 18, 2020

8253278: Refactor/cleanup oopDesc::*_klass_addr #221

8253278: Refactor/cleanup oopDesc::*_klass_addr #221

Conversation

shipilev commented Sep 17, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 17, 2020

openjdk bot commented Sep 17, 2020

mlbridge bot commented Sep 17, 2020

Webrevs

shipilev commented Sep 17, 2020

openjdk bot commented Sep 17, 2020

shipilev commented Sep 17, 2020 • edited Loading

coleenp left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 17, 2020 • edited Loading

shipilev commented Sep 18, 2020

shipilev commented Sep 18, 2020

openjdk bot commented Sep 18, 2020

shipilev commented Sep 18, 2020

openjdk bot commented Sep 18, 2020

coleenp commented Sep 18, 2020

shipilev commented Sep 18, 2020

shipilev commented Sep 17, 2020 •

edited by openjdk bot

Loading

shipilev commented Sep 17, 2020 •

edited

Loading

openjdk bot commented Sep 17, 2020 •

edited

Loading