8290833: Remove ConstantPoolCache::walk_entries_for_initialization() #9759

iklam · 2022-08-04T23:22:25Z

Background:

ConstantPoolCache::walk_entries_for_initialization() is called by ConstantPoolCache::remove_unshareable_info() to restore the CpCache to the state immediately after the class has been rewritten by the Rewriter (which happens during the class linking phase). In most part, this means the ConstantPoolCacheEntry's need to be zeroed. However, the _f2 fields of some of the entries are initialized to be non-zero by the Rewriter and must be preserved.

The reason walk_entries_for_initialization() exists is that after Rewriter::rewrite() has finished, some information about what is stored inside the CpCache is discarded (e.g., Rewriter::_invokedynamic_references_map). As a result, we cannot easily determine which entries has a _f2 field that need to be preserved. We must walk all the bytecodes in all the methods of this class to recompute this information.

This is awkward and time consuming. It also needs to be updated if the Rewriter ever changes.

Also, for future optimizations, we may need to pre-resolve a subset of the CpCache entries during CDS dump time. Trying to make that work alongside walk_entries_for_initialization() seems too complicated.

Fix:

Store a copy of the CpCache after rewritting. Use this to revert the CpCache's state inside ConstantPoolCache::remove_unshareable_info().

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8290833: Remove ConstantPoolCache::walk_entries_for_initialization()

Reviewers

Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to eb99170b
Calvin Cheung (@calvinccheung - Reviewer) ⚠️ Review applies to dcf1c54c

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9759/head:pull/9759
$ git checkout pull/9759

Update a local copy of the PR:
$ git checkout pull/9759
$ git pull https://git.openjdk.org/jdk pull/9759/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9759

View PR using the GUI difftool:
$ git pr show -t 9759

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9759.diff

bridgekeeper · 2022-08-04T23:24:13Z

👋 Welcome back iklam! 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 · 2022-08-04T23:27:22Z

@iklam 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 pull request command.

iklam · 2022-08-04T23:41:34Z

/label remove hotspot
/label add hotspot-runtime

openjdk · 2022-08-04T23:42:26Z

@iklam
The hotspot label was successfully removed.

openjdk · 2022-08-04T23:44:11Z

@iklam
The hotspot-runtime label was successfully added.

mlbridge · 2022-08-04T23:51:28Z

Webrevs

src/hotspot/share/cds/dumpTimeClassInfo.hpp

coleenp

This looks like a nice improvement, not having to figure out how to reinitialize the cpCache properly.

coleenp · 2022-08-05T23:04:34Z

src/hotspot/share/cds/dumpTimeClassInfo.cpp

+
+  // The saved cp entries array is not modified by the ArchiveBuilder, so there's
+  // no need to make a deep copy. It will be freed when the holder class is removed.
+  _saved_cpcache_entries = src._saved_cpcache_entries;


If you're copying these entries, should you make src._saved_cpcache_entries null so you don't double delete them? Or just for safety in case someone adds deleting it to the destructor later.

Actually, the _saved_cpcache_entries has a different lifecycle than the DumpTimeClassInfo. We set the _saved_cpcache_entries after the cpCache is allocated, and it's never changed afterwards. It's freed when the cpCache is deallocated.

I can see why the current code is confusing. I'll try to move it to a new table of cpCache* -> ConstantPoolEntry* which will make the code easier to understand and maintain.

src/hotspot/share/cds/dumpTimeClassInfo.hpp

openjdk · 2022-08-05T23:12:53Z

@iklam This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290833: Remove ConstantPoolCache::walk_entries_for_initialization()

Reviewed-by: coleenp, ccheung

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

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

calvinccheung

Updates look good.

coleenp

I think this might need some more comments.

src/hotspot/share/classfile/systemDictionaryShared.cpp

coleenp · 2022-08-09T16:50:27Z

src/hotspot/share/classfile/systemDictionaryShared.cpp

+  ConstantPoolCache* cpc = k->constants()->cache();
+  if (cpc != NULL) {
+    remove_saved_cpcache_entries_locked(cpc);
+  }


Why did you need the separate table?

There are two reasons:

The old table was indexed by InstanceKlass*. However, with class redefinition, a second ConstantPoolCache may be created for a class. Trying to keep this one-to-many relationship with the old table is doable but awkward.

The old table was cloned when the archive is dumped. However, the saved_cpcache_entries don't need to be cloned because they are never modified. I don't want to have to explain why parts of the DumpTimeClassInfo are not deep-copied.

coleenp · 2022-08-09T17:54:04Z

src/hotspot/share/oops/cpCache.cpp

-  InstanceKlass* ik = constant_pool()->pool_holder();
-  ConstantPoolCacheEntry* saved = SystemDictionaryShared::get_saved_cpcache_entries_locked(ik);
+  ConstantPoolCache* orig_cpc = ArchiveBuilder::current()->get_src_obj(this);
+  ConstantPoolCacheEntry* saved = SystemDictionaryShared::get_saved_cpcache_entries_locked(orig_cpc);


What does this do? get_src_obj looks like it just returns the address with a cast to the type T.

I was confused with the template. get_src_obj() gets in-memory (current) version of the metadata object. It has a mapping to the copy in the archive. right?
"this" is the copy in the archive. The comment helps.

coleenp

Maybe a couple more comments, otherwise, looks good. Approving again.

coleenp · 2022-08-10T20:04:27Z

src/hotspot/share/oops/cpCache.cpp

-  InstanceKlass* ik = constant_pool()->pool_holder();
-  ConstantPoolCacheEntry* saved = SystemDictionaryShared::get_saved_cpcache_entries_locked(ik);
+  ConstantPoolCache* orig_cpc = ArchiveBuilder::current()->get_src_obj(this);
+  ConstantPoolCacheEntry* saved = SystemDictionaryShared::get_saved_cpcache_entries_locked(orig_cpc);


I was confused with the template. get_src_obj() gets in-memory (current) version of the metadata object. It has a mapping to the copy in the archive. right?
"this" is the copy in the archive. The comment helps.

coleenp · 2022-08-10T20:07:14Z

src/hotspot/share/cds/archiveBuilder.hpp

@@ -200,6 +200,7 @@ class ArchiveBuilder : public StackObj {
  SourceObjList _rw_src_objs;                 // objs to put in rw region
  SourceObjList _ro_src_objs;                 // objs to put in ro region
  ResizeableResourceHashtable<address, SourceObjInfo, ResourceObj::C_HEAP, mtClassShared> _src_obj_table;
+  ResizeableResourceHashtable<address, address, ResourceObj::C_HEAP, mtClassShared> _dumped_to_src_obj_table;


I was trying to sort out these tables. Can you have a comment above these tables what they are mapping? The names are somewhat opaque.

I think people will first reach these functions when reading the code, so I added some comments there:

// + When creating a CDS archive, we first load Java classes and create metadata // objects as usual. These are call "source" objects. // + We then copy the source objects into the output buffer at "dumped addrsses". // // The following functions translate between these two (non-overlapping) spaces. // (The API should be renamed to be less confusing!) address get_dumped_addr(address src_obj) const; address get_src_obj(address dumped_addr) const;

I have filed an RFE JDK-8292225 to clean up the names to make the core easier to understand.

Ok, yes thanks. but addrsses is missing an 'e' above. Thanks for filing an additional bug so that one can guess what the tables mean even without knowing all the context.

…addresses (will be properly fixed in JDK-8292225)

…remove-walk_entries_for_initialization

iklam · 2022-08-11T21:47:14Z

Thanks @coleenp and @calvinccheung for the review.
/integrate

openjdk · 2022-08-11T21:48:47Z

Going to push as commit bd58553.

openjdk · 2022-08-11T21:49:02Z

@iklam Pushed as commit bd58553.

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

8290833: Remove ConstantPoolCache::walk_entries_for_initialization()

96bf7b1

openjdk bot added the hotspot hotspot-dev@openjdk.org label Aug 4, 2022

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Aug 4, 2022

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Aug 4, 2022

fixed minimal build

5f2756a

iklam marked this pull request as ready for review August 4, 2022 23:45

openjdk bot added the rfr Pull request is ready for review label Aug 4, 2022

calvinccheung reviewed Aug 5, 2022

View reviewed changes

src/hotspot/share/cds/dumpTimeClassInfo.hpp Outdated Show resolved Hide resolved

coleenp approved these changes Aug 5, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 5, 2022

iklam added 2 commits August 8, 2022 11:33

review comments by @calvinccheung and @coleenp

44aa76d

Use a separate _saved_cpcache_entries_table

dcf1c54

calvinccheung approved these changes Aug 8, 2022

View reviewed changes

coleenp reviewed Aug 9, 2022

View reviewed changes

@coleenp review comments; added asserts

eb99170

coleenp approved these changes Aug 10, 2022

View reviewed changes

iklam added 4 commits August 10, 2022 21:21

added some comments about the translation between "src" and "dumped" …

36c6cfd

…addresses (will be properly fixed in JDK-8292225)

Merge branch 'master' of https://github.com/openjdk/jdk into 8290833-…

cb56a3f

…remove-walk_entries_for_initialization

Merge branch 'master' of https://github.com/openjdk/jdk into 8290833-…

a66c3b6

…remove-walk_entries_for_initialization

fixed typo

c061f2b

openjdk bot added the integrated Pull request has been integrated label Aug 11, 2022

openjdk bot closed this Aug 11, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 11, 2022

This was referenced Aug 15, 2022

8292318: Memory corruption in remove_dumptime_info #9883

Closed

8292318: Memory corruption in remove_dumptime_info #9887

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8290833: Remove ConstantPoolCache::walk_entries_for_initialization() #9759

8290833: Remove ConstantPoolCache::walk_entries_for_initialization() #9759

iklam commented Aug 4, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

iklam commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

mlbridge bot commented Aug 4, 2022 •

edited

Loading

coleenp left a comment

coleenp Aug 5, 2022

iklam Aug 8, 2022

openjdk bot commented Aug 5, 2022 •

edited

Loading

calvinccheung left a comment

coleenp left a comment

coleenp Aug 9, 2022

iklam Aug 9, 2022

coleenp Aug 9, 2022

coleenp Aug 10, 2022

coleenp left a comment

coleenp Aug 10, 2022

coleenp Aug 10, 2022

iklam Aug 11, 2022

coleenp Aug 11, 2022

iklam commented Aug 11, 2022

openjdk bot commented Aug 11, 2022

openjdk bot commented Aug 11, 2022

8290833: Remove ConstantPoolCache::walk_entries_for_initialization() #9759

8290833: Remove ConstantPoolCache::walk_entries_for_initialization() #9759

Conversation

iklam commented Aug 4, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

iklam commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

mlbridge bot commented Aug 4, 2022 • edited Loading

Webrevs

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Aug 5, 2022 • edited Loading

calvinccheung left a comment

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iklam commented Aug 11, 2022

openjdk bot commented Aug 11, 2022

openjdk bot commented Aug 11, 2022

iklam commented Aug 4, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 4, 2022 •

edited

Loading

openjdk bot commented Aug 5, 2022 •

edited

Loading