Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8271506: Add ResourceHashtable support for deleting selected entries #4938

Closed
wants to merge 2 commits into from

Conversation

@coleenp
Copy link
Contributor

@coleenp coleenp commented Jul 29, 2021

The ResourceHashtable doesn't have a way to delete selected entries based on things like their class has been unloaded, which is needed to replace Hashtable with ResourceHashtable.
The Nodes of the ResourceHashtable has a Key and Value of template types.
template<typename K, typename V>
class ResourceHashtableNode : public ResourceObj {
public:
unsigned _hash;
K _key;
V _value;
...
But there's no destructor so that ~K and ~V are not called (if I understand C++ correctly).

When instantiated with a value that's not a pointer, calling code does this:

SourceObjInfo src_info(ref, read_only, follow_mode);
bool created;
SourceObjInfo* p = _src_obj_table.put_if_absent(src_obj, src_info, &created);

So if SourceObjInfo has a destructor, it'll have to have a careful assignment operator so that the value copied into the hashtable doesn't get deleted.

In this patch, I assign the responsibility of deleting the Key and Value of the hashtable to the do_entry function, because it's simple. If we want to use more advanced unreadable C++ code, someone will have to suggest an alternate set of changes, because my C++ is not up to this.

Tested with tier1-3, gtest, and upcoming patch for JDK-8048190.


Progress

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

Issue

  • JDK-8271506: Add ResourceHashtable support for deleting selected entries

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4938/head:pull/4938
$ git checkout pull/4938

Update a local copy of the PR:
$ git checkout pull/4938
$ git pull https://git.openjdk.java.net/jdk pull/4938/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4938

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4938.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 29, 2021

👋 Welcome back coleenp! 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.

Loading

@openjdk openjdk bot added the rfr label Jul 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 29, 2021

@coleenp 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.

Loading

@openjdk openjdk bot added the hotspot label Jul 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 29, 2021

Webrevs

Loading

// called for each entry in the table. If do_entry() returns true,
// the entry is deleted.
template<class ITER>
void unlink(ITER* iter) const {
Copy link
Member

@iklam iklam Jul 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unlink() shouldn't be const as it modifies the table. However, you probably needed to declare it as such because it calls bucket_at(), which is const. And the reason that bucket_at() is const is because it's called by iterate(), which is const.

The use of const and const_cast in this class is getting a bit out of whack. I have created JDK-8271525 with a preliminary fix here: #4942

Maybe we should do that before this PR? it will also make the const_cast unnecessary for decrement_entries().

Loading

Copy link
Contributor Author

@coleenp coleenp Jul 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @tstuefe in your draft PR. I don't think get should be non-const. I haven't been able to parse through the monstrosity cast of lookup_node to know why and whether it should be changed.
I made unlink non-const and made the changes that he suggested there and that fixed the cast that I had to decrement the entries.

Loading

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Aug 2, 2021

I don't think this should be blocked by JDK-8271525 since there is still discussion on that approach, which doesn't really affect this change.

Loading

iklam
iklam approved these changes Aug 2, 2021
Copy link
Member

@iklam iklam left a comment

LGTM. I agree this shouldn't be blocked by JDK-8271525.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 2, 2021

@coleenp 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:

8271506: Add ResourceHashtable support for deleting selected entries

Reviewed-by: iklam, stuefe

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 28 new commits pushed to the master branch:

  • 0a85236: 8193559: ugly DO_JAVA_THREADS macro should be replaced
  • db950ca: 8271348: Add stronger sanity check of thread state when polling for safepoint/handshakes
  • 3e3051e: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside
  • 7a4c754: 8271611: Use SecurityConstants.ACCESS_PERMISSION in MethodHandles
  • e74537f: 8271605: Update JMH devkit to 1.32
  • 249d641: 8263561: Re-examine uses of LinkedList
  • 6a3f834: 8268113: Re-use Long.hashCode() where possible
  • 2536e43: 8270160: Remove redundant bounds check from AbstractStringBuilder.charAt()
  • 6c4c48f: 8266972: Use String.concat() in j.l.Class where invokedynamic-based String concatenation is not available
  • 72145f3: 8269665: Clean-up toString() methods of some primitive wrappers
  • ... and 18 more: https://git.openjdk.java.net/jdk/compare/d09b028407ff9d0e8c2dfd9cc5d0dca19c4497e3...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

Loading

@openjdk openjdk bot added the ready label Aug 2, 2021
tstuefe
tstuefe approved these changes Aug 2, 2021
Copy link
Member

@tstuefe tstuefe left a comment

Hi Coleen,

looks good to me. One minor remark, but I leave it up to you, this is good as it is.

Thanks, Thomas

Loading

while (*ptr != NULL) {
Node* node = *ptr;
// do_entry must clean up the key and value in Node.
bool clean = iter->do_entry(node->_key, node->_value);
Copy link
Member

@tstuefe tstuefe Aug 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I miss it, you don't enforce the constness here, no? So, nothing prevents the called hook from modifying key or value. You may do what Ioi did in #4942 and const-cast the parameters for the call.

Loading

Copy link
Contributor Author

@coleenp coleenp Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I answered this in email but I don't see it. I want the do_entry function to modify the arguments, like calling their destructor or free operators.

Loading

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Aug 3, 2021

Thanks Thomas and Ioi for the code reviews.

Loading

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Aug 3, 2021

Yes, Ioi, hopefully you can just move this unlink function in your changes.
/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 3, 2021

Going to push as commit f15d6cb.
Since your change was applied there have been 36 commits pushed to the master branch:

  • bdb50ca: 8270347: ZGC: Adopt release-acquire ordering for forwarding table access
  • b217a6c: 8271609: Misleading message for AbortVMOnVMOperationTimeoutDelay
  • c8add22: Merge
  • ada58d1: 8067223: [TESTBUG] Rename Whitebox API package
  • f8fb571: 8271150: Remove EA from JDK 17 version string starting with Initial RC promotion on Aug 5, 2021(B34)
  • 84f0231: 8271419: Refactor test code for modifying CDS archive contents
  • 0b95394: 8271624: Avoid unnecessary ThreadGroup.checkAccess calls when creating Threads
  • e621cff: 8271627: Use local field access in favor of Class.getClassLoader0
  • 0a85236: 8193559: ugly DO_JAVA_THREADS macro should be replaced
  • db950ca: 8271348: Add stronger sanity check of thread state when polling for safepoint/handshakes
  • ... and 26 more: https://git.openjdk.java.net/jdk/compare/d09b028407ff9d0e8c2dfd9cc5d0dca19c4497e3...master

Your commit was automatically rebased without conflicts.

Loading

@openjdk openjdk bot closed this Aug 3, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Aug 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 3, 2021

@coleenp Pushed as commit f15d6cb.

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

Loading

@coleenp coleenp deleted the unlink branch Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants