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

8276789: Support C++ lambda in ResourceHashtable::iterate #8761

Closed

Conversation

iklam
Copy link
Member

@iklam iklam commented May 17, 2022

I added two new template functions to reduce the boilerplate code when walking the entries in a ResourceHashtable

  • template<typename F> void ResourceHashtable::iterate(F f)
  • template<typename F> void ResourceHashtable::iterate_all(F f)

I also modified a couple of places in systemDictionaryShared.cpp to use the new functionality.

Testing with tiers 1-4.


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-8276789: Support C++ lambda in ResourceHashtable::iterate

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8761

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented May 17, 2022

👋 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.

@iklam
Copy link
Member Author

iklam commented May 17, 2022

GCC 10.3.0 generates identical code for the following before and after this PR. So there should be no impact with existing code that uses the old style iterator.

class MyIter : StackObj {
public:
  int _count;
  MyIter() : _count(0) {}
  bool do_entry(InstanceKlass* k, DumpTimeClassInfo& info) {
    _count ++;
    return true; // keep on iterating
  }
};

int doit(DumpTimeSharedClassTable* dumptime_table) {
  MyIter myiter;
  dumptime_table->iterate(&myiter);
  return myiter._count;
}

@openjdk
Copy link

openjdk bot commented May 17, 2022

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

@openjdk openjdk bot added the hotspot label May 17, 2022
@iklam iklam marked this pull request as ready for review May 17, 2022
@openjdk openjdk bot added the rfr label May 17, 2022
@mlbridge
Copy link

mlbridge bot commented May 17, 2022

Webrevs

bool do_entry(InstanceKlass* k, DumpTimeClassInfo& info) {
template<typename F>
void DumpTimeSharedClassTable::iterate(F f) const {
auto g = [=] (InstanceKlass* k, DumpTimeClassInfo& info) {
Copy link
Member

@dholmes-ora dholmes-ora May 18, 2022

Choose a reason for hiding this comment

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

The style guide states we should try to only use [&] for capture by reference.

Copy link
Member Author

@iklam iklam May 18, 2022

Choose a reason for hiding this comment

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

Fixed.

@@ -36,21 +36,15 @@
#if INCLUDE_CDS

// For safety, only iterate over a class if it loader is alive.
Copy link
Member

@dougxc dougxc May 18, 2022

Choose a reason for hiding this comment

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

"if it loader" -> "if its loader is"

auto check_for_exclusion = [&] (InstanceKlass* k, DumpTimeClassInfo& info) {
SystemDictionaryShared::check_for_exclusion(k, &info);
};
_dumptime_table->iterate_all(check_for_exclusion);
Copy link
Member

@stefank stefank May 24, 2022

Choose a reason for hiding this comment

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

Can't this be changed to just:
_dumptime_table->iterate_all(SystemDictionaryShared::check_for_exclusion);

or maybe even:
_dumptime_table->iterate_all(check_for_exclusion);

Copy link
Member Author

@iklam iklam May 24, 2022

Choose a reason for hiding this comment

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

The second parameter to SystemDictionaryShared::check_for_exclusion() is a DumpTimeClassInfo* because NULL is allowed. However, the iterator requires a DumpTimeClassInfo&.

Copy link
Member

@stefank stefank May 25, 2022

Choose a reason for hiding this comment

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

OK

template<class ITER>
void iterate(ITER* iter) const {
auto function = [&] (K& k, V& v) {
return iter->do_entry(k, v);
};
iterate(function);
}
Copy link
Member

@stefank stefank May 24, 2022

Choose a reason for hiding this comment

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

Do you still have any usages of this? I'm a bit surprised that the compiler don't complain about not being able to disambiguate calls to this vs the new lambda enabled function.

Copy link
Member Author

@iklam iklam May 24, 2022

Choose a reason for hiding this comment

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

This is still needed as there are many places that use the old style iteration. Also, when the body of the iteration is complex, it might be more readable to do it as a separate class. E.g., here in systemDictionaryShared.cpp:

    UnregisteredClassesDuplicationChecker dup_checker;
    _dumptime_table->iterate(&dup_checker);

I think there's no ambiguity because iter->do_entry() cannot be substituted with the other template, and function(node->_key, node->_value) in the other template cannot be substituted here. So by SFINAE the C++ compiler can pick the correct template.

Copy link
Member

@stefank stefank May 25, 2022

Choose a reason for hiding this comment

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

Interesting. I got problems when I converted BitMap to support both closures and lambdas. The problem might have been that the iterate function took an specific closure class, and didn't use a template.

Choose a reason for hiding this comment

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

[Not a review, just a drive-by comment]
SFINAE operates on arguments and signatures; it doesn't reach into the bodies
of (potential) callees like that. The disambiguation is happening because one
argument (the Closure object) is passed as a pointer, while the other is a
lambda object (not a pointer). In the pointer to closure object case the ITER*
overload is a better match, so is selected. In the lambda case, it's not a
pointer so only the non-pointer overload is applicable.

Choose a reason for hiding this comment

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

(And SFINAE can't reach into the body like that, since the body might not be available.)

Copy link
Member Author

@iklam iklam Jun 3, 2022

Choose a reason for hiding this comment

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

@kimbarrett Thanks for the explanation. So we're lucky because we use ITER* in the old API. If it were ITER& then the C++ compiler would complain about ambiguous overloading.

auto check_for_exclusion = [&] (InstanceKlass* k, DumpTimeClassInfo& info) {
SystemDictionaryShared::check_for_exclusion(k, &info);
};
_dumptime_table->iterate_all(check_for_exclusion);
Copy link
Member

@stefank stefank May 25, 2022

Choose a reason for hiding this comment

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

OK

template<class ITER>
void iterate(ITER* iter) const {
auto function = [&] (K& k, V& v) {
return iter->do_entry(k, v);
};
iterate(function);
}
Copy link
Member

@stefank stefank May 25, 2022

Choose a reason for hiding this comment

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

Interesting. I got problems when I converted BitMap to support both closures and lambdas. The problem might have been that the iterate function took an specific closure class, and didn't use a template.

@openjdk
Copy link

openjdk bot commented May 25, 2022

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

8276789: Support C++ lambda in ResourceHashtable::iterate

Reviewed-by: stefank, coleenp

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.

@openjdk openjdk bot added the ready label May 25, 2022
Copy link
Contributor

@coleenp coleenp left a comment

I'm a bit baffled by the layers of lambda expressions.

@@ -209,8 +209,15 @@ class DumpTimeSharedClassTable: public DumpTimeSharedClassTableBaseType
}
Copy link
Contributor

@coleenp coleenp Jun 1, 2022

Choose a reason for hiding this comment

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

This file has a blank first line, can you fix this?

Copy link
Contributor

@coleenp coleenp Jun 1, 2022

Choose a reason for hiding this comment

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

And no new line between these here:
#define SHARED_CDS_DUMPTIMESHAREDCLASSINFO_HPP
#include "cds/archiveBuilder.hpp"

Copy link
Member Author

@iklam iklam Jun 2, 2022

Choose a reason for hiding this comment

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

Fixed.

auto function = [&] (InstanceKlass* k, DumpTimeClassInfo& v) {
return iter->do_entry(k, v);
};
iterate(function);
Copy link
Contributor

@coleenp coleenp Jun 1, 2022

Choose a reason for hiding this comment

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

I don't understand this wrapper level. Why doesn't this call into the resourceHashtable::iterate function directly? Same with the iterate_all function?

Copy link
Member Author

@iklam iklam Jun 2, 2022

Choose a reason for hiding this comment

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

I renamed the functions to DumpTimeSharedClassTable::iterate_all_live_classes() to disambiguate them from the ResourceHashtable::iterate/_all functions.

void SystemDictionaryShared::check_excluded_classes() {
assert(no_class_loading_should_happen(), "sanity");
assert_lock_strong(DumpTimeTable_lock);

if (DynamicDumpSharedSpaces) {
// Do this first -- if a base class is excluded due to duplication,
// all of its subclasses will also be excluded by ExcludeDumpTimeSharedClasses
// all of its subclasses will also be excluded.
ResourceMark rm;
UnregisteredClassesDuplicationChecker dup_checker;
_dumptime_table->iterate(&dup_checker);
Copy link
Contributor

@coleenp coleenp Jun 1, 2022

Choose a reason for hiding this comment

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

Which iterate function does this call?

Copy link
Member Author

@iklam iklam Jun 2, 2022

Choose a reason for hiding this comment

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

I changed it to _dumptime_table->iterate_all_live_classes()

coleenp
coleenp approved these changes Jun 3, 2022
Copy link
Contributor

@coleenp coleenp left a comment

Looks good. Thanks for the help and discussion offline. Now it makes sense.

// instead.
template<class ITER> void iterate(ITER* iter) const;
template<typename Function> void iterate(Function function) const;
template<typename Function> void iterate_all(Function function) const;
Copy link
Contributor

@coleenp coleenp Jun 3, 2022

Choose a reason for hiding this comment

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

This helps a lot and thanks for the comment, which describes why there's a wrapper for this class.

@iklam
Copy link
Member Author

iklam commented Jun 3, 2022

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

@openjdk
Copy link

openjdk bot commented Jun 3, 2022

Going to push as commit b544b8b.

@openjdk openjdk bot added the integrated label Jun 3, 2022
@openjdk openjdk bot closed this Jun 3, 2022
@openjdk openjdk bot removed ready rfr labels Jun 3, 2022
@openjdk
Copy link

openjdk bot commented Jun 3, 2022

@iklam Pushed as commit b544b8b.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot integrated
7 participants