Navigation Menu

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

8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V) #8259

Closed
wants to merge 8 commits into from

Conversation

liach
Copy link
Member

@liach liach commented Apr 15, 2022

Explicitly implement remove and replace in IdentityHashMap to compare values by identity. Updated API documentation of these two methods (Preview) to mention such behavior.


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
  • Change requires a CSR request to be approved

Issues

  • JDK-8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)
  • JDK-8284901: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V) (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8259

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

Using diff file

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

@liach
Copy link
Member Author

liach commented Apr 15, 2022

/csr

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 15, 2022

👋 Welcome back liach! 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 openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Apr 15, 2022
@openjdk
Copy link

openjdk bot commented Apr 15, 2022

@liach this pull request will not be integrated until the CSR request JDK-8284901 for issue JDK-8178355 has been approved.

@openjdk
Copy link

openjdk bot commented Apr 15, 2022

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

  • core-libs

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 core-libs core-libs-dev@openjdk.org label Apr 15, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 15, 2022

Webrevs

public static void main(String[] args) {
final String key = "key";
final String internedValue = "value";
final String constructedValue = new String(new char[]{'v', 'a', 'l', 'u', 'e'});

Choose a reason for hiding this comment

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

Using:

Suggested change
final String constructedValue = new String(new char[]{'v', 'a', 'l', 'u', 'e'});
final String constructedValue = new String(internedValue);

will allow the internal String.value array to be shared:

@IntrinsicCandidate
public String(String original) {
this.value = original.value;
this.coder = original.coder;
this.hash = original.hash;
this.hashIsZero = original.hashIsZero;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I know. This is just to get rid of intellij idea's warning.

@liach
Copy link
Member Author

liach commented Apr 19, 2022

@stuart-marks Mind peek at this given you are the assignee of the original JBS issue?

@stuart-marks
Copy link
Member

Thanks for working on this. Yes I can review and sponsor this change.

Sorry I got a bit distracted. I started looking at the test here, and this lead me to inquire about what other tests we have for IdentityHashMap, and the answer is: not enough. See JDK-8285295. (But that should be handled separately.)

@@ -1192,7 +1160,7 @@ public boolean contains(Object o) {
}
public boolean remove(Object o) {
return o instanceof Entry<?, ?> entry
&& removeMapping(entry.getKey(), entry.getValue());
&& IdentityHashMap.this.remove(entry.getKey(), entry.getValue());
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to keep the internal removeMapping method and have other methods call it, instead of calling a public method. In particular the EntrySet.remove() method here should call an internal method to perform the actual removal instead of calling the public remove() method, since that potentially exposes the "self-use" to subclasses. The the public remove() method on IDHM could call removeMapping.

return false;
i = nextKeyIndex(i, len);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately there's some mostly-duplicate code here. However, there's similar logic and code sprinkled throughout this class, so more duplication isn't necessarily the wrong thing to do. However, trying to unify this logic results in much more intrusive refactoring, which is harder to review, and which isn't backed up with tests (see JDK-8285295) which I wouldn't encourage pursuing right now. In other words, I'm ok with this duplicate logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

On intrusive logic: I planned address it in https://bugs.openjdk.java.net/browse/JDK-8277520 (#6532), and if this one is integrated first, it may be possible to fix it up there.

throw new AssertionError("Failed to remove value by identity");
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Overall comments on this test. It does test the right set of four cases (positive and negative calls to replace and remove). However, it's one test that tests the four cases, instead of four tests. Using the same state makes it hard to add or maintain test cases in the future. It also trusts the return value of the method calls and doesn't make any assertions over the actual contents of the map. Without assertions over the expected contents, a method could behavior incorrectly and cause unrelated and confusing errors downstream, or even cause errors to be missed. I'd thus recommend the following:

  • Convert this to a Test-NG test and use a @BeforeTest method to set up a test fixture consisting of a map containing the desired entries.
  • Make each test case into its own test method that performs the method call and then makes assertions over the return value and expected result state of the map. Individual test methods is probably fine; no need to use a data provider for this.
  • Probably add a "null hypothesis" test to ensure that the test fixture itself has the right properties, similar to the assertions at lines 38-44.
  • Instead of fiddling around with strings, which have additional complexity caused by interning of string literals, I'd suggest using the technique I used in JDK-8285295 and create a record Box(int i) { } class. With this it's easy to get multiple instances that are != but are equals.
  • After further thought, maybe additional cases are necessary. For example, tests of calls to remove and replace with a key that is equals() but != to a key in the map.

@liach
Copy link
Member Author

liach commented Apr 21, 2022

@stuart-marks Updated. In addition, for API docs change, should we add apiNote on object equality code, like call computeIfPresent?

@liach
Copy link
Member Author

liach commented May 6, 2022

@stuart-marks I have updated the tests to be based off that from JDK-8285295. Anything else that needs an update?

@liach
Copy link
Member Author

liach commented May 31, 2022

@stuart-marks Just curious, would this fix target 19 or 20 at the current state?

Comment on lines 1388 to 1392
* <p>More formally, if this map contains a mapping from a key
* {@code k} to a value {@code v} such that {@code key == k}
* and {@code value == v}, then this method removes the mapping
* for this key and returns {@code true}; otherwise it returns
* {@code false}.
Copy link
Member

@dfuch dfuch Jun 1, 2022

Choose a reason for hiding this comment

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

The API documentation of containsKey() and containsValue() should probably be updated to mention reference equality too. This doesn't need to be carried out in this PR, but maybe a new issue should be logged to double check the completeness of the IdentityHashMap API documentation and see where adding some similar text makes sense.

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 29, 2022

@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@liach
Copy link
Member Author

liach commented Jun 29, 2022

Could anyone take a look at this collections-related patch?

@stuart-marks
Copy link
Member

OK, I will sponsor this.

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 27, 2022

@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@liach
Copy link
Member Author

liach commented Jul 27, 2022

Is there any update that I should push to this patch?

@stuart-marks
Copy link
Member

No action needed from you at this time. This is mostly waiting for me to get some time to focus on this. Thanks for your patience.

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 29, 2022

@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@liach
Copy link
Member Author

liach commented Aug 31, 2022

Let's wait

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 28, 2022

@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@stuart-marks
Copy link
Member

stuart-marks commented Sep 28, 2022

I swear, I'll get to this soon! I even pulled over the patch and started looking at it.

@liach Actually one thing that would be helpful is if you could merge a recent master into the PR branch. Thanks.

@stuart-marks
Copy link
Member

stuart-marks commented Oct 5, 2022

@liach Thanks for merging in a recent master. I started looking at this now. The tests and implementation look good. I've run this through our internal build&test system and the results look good too.

Per @dfuch comment of June 1 I also looked at various method specs to see whether they would need to be updated to have reference-equality semantics. The specs are all in mostly pretty good shape, but I did add a few clauses here and there and I also added some cross-references, so I think the spec is overall much tighter now.

I've pushed a branch with my proposed spec changes. Please merge from

https://github.com/stuart-marks/jdk/tree/JDK-8178355-IdentityHashMap

which should be based on the current head of this PR branch.

The next step would be the CSR. I'd suggest updating in the (slightly) revised specs of remove() and replace() in the Specification section, but otherwise leaving them pretty much as they are, and not bothering with diffs for all the other tweaks I did. I'll generate a specdiff (internal tool, sorry, basically a structured HTML diff) and then post that to the CSR to provide a complete record of the changes.

@liach
Copy link
Member Author

liach commented Oct 5, 2022

I have updated the specification section of the CSR with the updated remove and replace spec (adding parentheses around equality expressions and replace "to" with "with") and pulled your branch.

@stuart-marks
Copy link
Member

OK, thanks for the updates. I've attached a specdiff and have tweaked the CSR in a few places and I've marked it reviewed. Go ahead and mark it Finalized.

(I could actually do this myself, but I'd have to reassign the CSR to myself, make it Finalized, then reassign it back to you. That would save a messaging round-trip, but it would generate more notification spam. Plus I figured I'd give you the pleasure of hitting the Finalize button. :-D )

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Oct 20, 2022
@stuart-marks
Copy link
Member

Now that the CSR is approved, this should be close to ready to go in. I just ran this through our CI system and there were a bunch of failures. Most likely they are unrelated, but I need to track this down and rerun the tests. I'll post an update when I get this figured out.

Copy link
Member

@stuart-marks stuart-marks left a comment

Choose a reason for hiding this comment

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

Sorry, the issues with the test failures were a distraction. I had used the wrong version of an internal test suite. Once I figured that out and used the right version, everything passes.

OK to integrate! I will sponsor this.

@openjdk
Copy link

openjdk bot commented Nov 2, 2022

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

8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)

Reviewed-by: smarks

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

  • fd60036: 8296188: Update style and header in JDWP Protocol spec and JVMTI spec
  • c7b95a8: 8296163: [aarch64] Cleanup Pre/Post addressing mode classes
  • f84b0ad: 8295670: Remove duplication in java/util/Formatter/Basic*.java
  • a124d8e: 8296115: Allow for compiling the JDK with strict standards conformance
  • 491d43c: 8289838: ZGC: OOM before clearing all SoftReferences
  • a1c349f: 8290063: IGV: Give the graphs a unique number in the outline
  • b807470: 8296235: IGV: Change shortcut to delete graph from ctrl+del to del
  • e15d241: 8295991: java/net/httpclient/CancelRequestTest.java fails intermittently
  • 16a041a: 8296142: CertAttrSet::(getName|getElements|delete) are mostly useless
  • 6626a29: 8294845: Make globals accessed by G1 young gen revising atomic
  • ... and 431 more: https://git.openjdk.org/jdk/compare/4fb424ba5f3ece1a47b8774defdbe6645cc72759...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 (@stuart-marks) 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 openjdk bot added the ready Pull request is ready to be integrated label Nov 2, 2022
@liach
Copy link
Member Author

liach commented Nov 3, 2022

Thanks!

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 3, 2022
@openjdk
Copy link

openjdk bot commented Nov 3, 2022

@liach
Your change (at version 6874dc8) is now ready to be sponsored by a Committer.

@stuart-marks
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Nov 3, 2022

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

  • 25dfcbd: 8289689: (fs) Re-examine the need for normalization to Unicode Normalization Format D (macOS)
  • b7442d1: 8295653: Add a graph of the sealed class hierarchy for marked classes
  • 59a13b1: 8295872: [PPC64] JfrGetCallTrace: Need pc == nullptr check before frame constructor
  • 2a79dfc: 8295774: Write a test to verify List sends ItemEvent/ActionEvent
  • cc3c5a1: 8296101: nmethod::is_unloading result unstable with concurrent unloading
  • d771abb: 8295970: Add vector api sanity tests in tier1
  • 7a85441: 8232933: Javac inferred type does not conform to equality constraint
  • 6ee8ccf: 8296168: x86: Add reasonable constraints between AVX and SSE
  • 1950747: 8279913: obsolete ExtendedDTraceProbes
  • 13b20e0: 8296262: Remove dead code from InstanceKlass::signature_name()
  • ... and 441 more: https://git.openjdk.org/jdk/compare/4fb424ba5f3ece1a47b8774defdbe6645cc72759...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 3, 2022
@openjdk openjdk bot closed this Nov 3, 2022
@openjdk 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 Nov 3, 2022
@openjdk
Copy link

openjdk bot commented Nov 3, 2022

@stuart-marks @liach Pushed as commit 53905e6.

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

@liach liach deleted the fix/identityhashmap-default branch May 7, 2023 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants