8329194: Cleanup Type::cmp definition and usage by jaskarth · Pull Request #18533 · openjdk/jdk

jaskarth · 2024-03-28T15:13:48Z

Hi all, this patch aims to cleanup Type::cmp by changing it from returning a 0 when types are equal and 1 when they are not, to it returning a boolean denoting equality. This makes its usages at various callsites more intuitive. However, as it is passed to the type dictionary as a comparator, a lambda is needed to map the boolean to a comparison value.

I was also considering changing the name to Type::equals as it's not really returning a comparison value anymore, but I felt it would be too similar to Type::eq. If this would be preferred though, I can change it.

Tier 1 testing passes on my machine. Reviews and thoughts would be appreciated!

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-8329194: Cleanup Type::cmp definition and usage (Enhancement - P5)

Reviewers

Damon Fenacci (@dafedafe - Committer) ⚠️ Review applies to 63c17d53
Christian Hagedorn (@chhagedorn - Reviewer)
Quan Anh Mai (@merykitty - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18533

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-28T15:14:34Z

👋 Welcome back jkarthikeyan! 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 · 2024-03-28T15:15:09Z

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

8329194: Cleanup Type::cmp definition and usage

Reviewed-by: dfenacci, chagedorn, qamai

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

e6118ce: 8331018: Clean up non-standard use of /** comments in jdk.jshell
a44ac02: 8330853: Add missing checks for ConnectionGraph::can_reduce_cmp() call
8a8d928: 8330611: AES-CTR vector intrinsic may read out of bounds (x86_64, AVX-512)
0014e0e: 8331061: Serial: Missed BOT update in TenuredGeneration::expand_and_allocate
ea3909a: 8331054: C2 MergeStores: assert failed: unexpected basic type after JDK-8318446 and JDK-8329555
7b2560b: 8330849: Add test to verify memory usage with recursive locking
25871af: 8328896: Fontmetrics for large Fonts has zero width
1d06170: 8300148: Consider using a StoreStore barrier instead of Release barrier on ctor exit
f1d0e71: 8324577: [REDO] - [IMPROVE] OPEN_MAX is no longer the max limit on macOS >= 10.6 for RLIMIT_NOFILE
74b11cc: 8331004: Serial: Remove unused GenClosure
... and 395 more: https://git.openjdk.org/jdk/compare/907e30ff00abd6cd4935987810d282f46ec07704...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dafedafe, @chhagedorn, @merykitty) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-03-28T15:15:34Z

@jaskarth The following label will be automatically applied to this pull request:

hotspot-compiler

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.

mlbridge · 2024-03-28T15:18:40Z

Webrevs

01: Full - Incremental (46a4f3fa)
00: Full (63c17d53)

dafedafe · 2024-04-11T07:37:19Z

src/hotspot/share/opto/type.cpp

+
+  // Map the boolean result of Type::cmp into a comparator result that CmpKey expects.
+  auto type_cmp = [](const void* t1, const void* t2) -> int32_t {
+    return Type::cmp((Type*) t1, (Type*) t2) ? 0 : 1;


Wouldn't return !Type::cmp((Type*) t1, (Type*) t2); be enough? (though it might actually result in the same compiled code)

I think this would work too, but I wanted to avoid the implicit boolean to integer conversion. I can make this change if it would be fine, though.

Fair point.

dafedafe

Looks good (I also tested tier1-5).

jaskarth · 2024-04-12T14:03:54Z

Great, thank you for the review and testing!

chhagedorn

Overall a good improvement and makes it more intuitive. A few comments.

src/hotspot/cpu/x86/x86.ad

src/hotspot/share/opto/node.cpp

src/hotspot/share/opto/type.cpp

chhagedorn · 2024-04-15T07:24:59Z

src/hotspot/share/opto/type.hpp

  // Create a new hash-consd type
  static const Type *make(enum TYPES);
  // Test for equivalence of types
-  static int cmp( const Type *const t1, const Type *const t2 );


Was it required to remove the consts here?

Since the second const is referring to the t1/t2 it means that the parameters can't be modified, which IIRC in the header would be functionally the same as not having the additional const. I changed it in type.cpp as well because looking at other parts of the code const Type* val is used a lot more often than const Type* const val for arguments.

eme64 · 2024-04-15T09:53:11Z

src/hotspot/share/opto/type.hpp

  static const Type *make(enum TYPES);
  // Test for equivalence of types
-  static int cmp( const Type *const t1, const Type *const t2 );
+  static bool cmp(const Type* t1, const Type* t2);


I wonder if this will be a bit confusing to people now. Usually, cmp methods tend to have an output that allows sorting, i.e. 0 for equals, and either larger or smaller than 0, depending on which one is larger.

For a bool return, the name equals would be more adequate. But this would also require lots of changes in the code-base, and maybe make backports harder.

Actually, I'd be worried about backports now in general:
Imagine someone now writes if (Type::cmp(...)). And someone else backports this, since the patch seems to cleanly apply. if (Type::cmp(...)) is also correct C++ before your change.

But the semantics now have changed: with your patch, the if succeeds if the types are equal. Without your patch, the if succeeds if the types are not equal. Yikes. This will lead to some very subtle bugs and annoying debugging in old JDK versions.

Hence, I would think you have to rename the method to equals and change all usages accordingly.

What do you think?

That's actually a good point about the backports. I was also wondering if equals is better due to the cmp convention of using ints but thought it's okay. But with backports in mind, it gives a stronger case to actually go with equals - so, I agree with your suggestion @eme64.

This is a good point, I hadn't thought about backports! In the top comment I was also thinking about naming it Type::equals, but I worried that it'd be too close to Type::eq. But with this detail I think it should be renamed as well, because of the change of semantics. That way at least it'll cause a compile error instead of silently breaking at runtime, which would be bad.

jaskarth · 2024-04-16T02:17:47Z

Thanks for the comments @chhagedorn and @eme64! I've pushed a commit that should address the points brought up in review, and renamed the function to Type::equals.

eme64 · 2024-04-16T10:56:56Z

src/hotspot/share/opto/type.hpp


  static int uhash( const Type *const t );
-  // Structural equality check.  Assumes that cmp() has already compared
+  // Structural equality check.  Assumes that equals() has already compared


Is this even correct? Because we use eq inside equals, (before your patch, we used eq inside cmp).
Should this not rather mean that we should have already done the ==?
What do you think?

I think it's still correct, because from my understanding it's referring to this line in equals:

if (t1->_base != t2->_base) { return false; // Missed badly }

equals only calls eqafter this, so eq knows that the underlying type of both classes are the same. So TypeInt::eq, for example, is able to safely cast the incoming Type* t to a TypeInt* to do a structural equality check.

eme64 · 2024-04-16T11:01:08Z

@jaskarth this looks good. I am running testing again now.

@merykitty do you have an opinion on this? You have done quite some work on types.

merykitty

I think it is a really nice change. It is confusing to have a cmp function on a type with no order.

Cheers,
Quan Anh

jaskarth · 2024-04-19T03:01:23Z

Thanks for taking another look @eme64, and for the review @merykitty :)

chhagedorn

Looks good, thanks for the updates!

jaskarth · 2024-04-24T23:06:49Z

Thanks for taking another look @chhagedorn!

/integrate

openjdk · 2024-04-24T23:08:12Z

@jaskarth
Your change (at version 46a4f3f) is now ready to be sponsored by a Committer.

TobiHartmann · 2024-04-25T13:11:23Z

/sponsor

openjdk · 2024-04-25T13:13:27Z

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

c944201: 8331031: unify os::dont_yield and os::naked_yield across Posix platforms
8031dab: 8331094: ZGC: GTest fails due to incompatible Windows version
c9f8d0e: 8331050: Serial: Remove unused _saved_mark_word in DefNewGeneration and TenuredGeneration
b1b953c: 8330972: Serial: Inline Generation::max_contiguous_available
87e864b: 8328703: Illegal accesses in Java_jdk_internal_org_jline_terminal_impl_jna_linux_CLibraryImpl_ioctl0
ef745a6: 8331097: Tests build is broken after pr/18914
d32f109: 8329797: Shenandoah: Default case invoked for: "MaxL" (bad AD file)
ccc0d0f: 8325373: Improve StackCounter error reporting for bad switch cases
e818ab6: 8330815: Use pattern matching for instanceof in KeepAliveCache
d43654e: 8331030: langtools/tools/javac/tree tests fail with SOE with fastdebug and -Xcomp
... and 406 more: https://git.openjdk.org/jdk/compare/907e30ff00abd6cd4935987810d282f46ec07704...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-25T13:13:35Z

@TobiHartmann @jaskarth Pushed as commit b9927aa.

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

Cleanup Type::cmp

63c17d5

openjdk bot added the rfr Pull request is ready for review label Mar 28, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 28, 2024

dafedafe reviewed Apr 11, 2024

View reviewed changes

dafedafe approved these changes Apr 12, 2024

View reviewed changes

chhagedorn reviewed Apr 15, 2024

View reviewed changes

eme64 reviewed Apr 15, 2024

View reviewed changes

Rename to Type::equals, changes from code review

46a4f3f

eme64 reviewed Apr 16, 2024

View reviewed changes

merykitty approved these changes Apr 16, 2024

View reviewed changes

chhagedorn approved these changes Apr 24, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 24, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 24, 2024

openjdk bot added the integrated Pull request has been integrated label Apr 25, 2024

openjdk bot closed this Apr 25, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 25, 2024

Conversation

jaskarth commented Mar 28, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 28, 2024

Uh oh!

openjdk bot commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 28, 2024

Uh oh!

mlbridge bot commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dafedafe left a comment

Choose a reason for hiding this comment

Uh oh!

jaskarth commented Apr 12, 2024

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaskarth commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eme64 commented Apr 16, 2024

Uh oh!

merykitty left a comment

Choose a reason for hiding this comment

Uh oh!

jaskarth commented Apr 19, 2024

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

jaskarth commented Apr 24, 2024

Uh oh!

openjdk bot commented Apr 24, 2024

Uh oh!

TobiHartmann commented Apr 25, 2024

Uh oh!

openjdk bot commented Apr 25, 2024

Uh oh!

openjdk bot commented Apr 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

jaskarth commented Mar 28, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 28, 2024 •

edited

Loading

mlbridge bot commented Mar 28, 2024 •

edited

Loading

jaskarth commented Apr 16, 2024 •

edited

Loading