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

8049228: Improve multithreaded scalability of InetAddress cache #70

Closed
wants to merge 3 commits into from

Conversation

lusou-zhangquan
Copy link

@lusou-zhangquan lusou-zhangquan commented Jun 1, 2022

Backport JDK-8049228 to improve multithreaded scalability of InetAddress cache

Backport-of 250fbb065a789a303d692d698c9b69117bf6cd2c


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issues

  • JDK-8049228: Improve multithreaded scalability of InetAddress cache
  • JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime for more precise and accurate

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev pull/70/head:pull/70
$ git checkout pull/70

Update a local copy of the PR:
$ git checkout pull/70
$ git pull https://git.openjdk.org/jdk8u-dev pull/70/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 70

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/70.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 1, 2022

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

@lusou-zhangquan lusou-zhangquan force-pushed the port_8049228 branch 2 times, most recently from a98fb5c to 799a4f3 Compare June 20, 2022 07:32
@lusou-zhangquan lusou-zhangquan changed the title 8u backport of JDK-8049228 8288718: Requesting backport of JDK-8049228 to JDK 8 Jun 20, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 20, 2022
@mlbridge
Copy link

mlbridge bot commented Jun 20, 2022

Webrevs

@jerboaa
Copy link
Contributor

jerboaa commented Jun 20, 2022

Please change the PR title to Backport 250fbb065a789a303d692d698c9b69117bf6cd2c and close the manual bug you've created for this backport. The original issue will be used instead once you use the said PR title: https://bugs.openjdk.org/browse/JDK-8049228

@lusou-zhangquan lusou-zhangquan changed the title 8288718: Requesting backport of JDK-8049228 to JDK 8 Backport 250fbb065a789a303d692d698c9b69117bf6cd2c Jun 20, 2022
@openjdk openjdk bot changed the title Backport 250fbb065a789a303d692d698c9b69117bf6cd2c 8049228: Improve multithreaded scalability of InetAddress cache Jun 20, 2022
@openjdk
Copy link

openjdk bot commented Jun 20, 2022

This backport pull request has now been updated with issues from the original commit.

@openjdk openjdk bot added the backport label Jun 20, 2022
@lusou-zhangquan
Copy link
Author

lusou-zhangquan commented Jun 20, 2022

@jerboaa PR title has been changed as your advise, but I have no idea about how to close the manual bug JDK-8288718 I created. Could you please provide any suggestions?

@openjdk-notifier
Copy link

@lusou-zhangquan Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

@jerboaa
Copy link
Contributor

jerboaa commented Jun 20, 2022

@jerboaa PR title has been changed as your advise, but I have no idea about how to close the manual bug JDK-8288718 I created. Could you please provide any suggestions?

I've closed it as a duplicate for you now.

Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

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

Looks fine, but please run at least tier1 tests.

Backport-of 250fbb065a789a303d692d698c9b69117bf6cd2c
@openjdk-notifier
Copy link

@lusou-zhangquan Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

@lusou-zhangquan
Copy link
Author

@jerboaa PR title has been changed as your advise, but I have no idea about how to close the manual bug JDK-8288718 I created. Could you please provide any suggestions?

I've closed it as a duplicate for you now.

Thanks a lot. Won't make similar mistakes next time.

@lusou-zhangquan
Copy link
Author

Looks fine, but please run at least tier1 tests.

Re-run the gate and passed. Please take a look again.

@lusou-zhangquan
Copy link
Author

@phohensee Please take a look at this PR again, thanks a lot.

Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

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

Lgtm.

@openjdk
Copy link

openjdk bot commented Jul 18, 2022

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

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

8049228: Improve multithreaded scalability of InetAddress cache
7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime for more precise and accurate

Reviewed-by: phh, andrew

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

  • d0ad242: 8285497: Add system property for Java SE specification maintenance version
  • e3251a2: 8232950: SUNPKCS11 Provider incorrectly check key length for PSS Signatures.
  • 41c7d2d: 8039955: [TESTBUG] jdk/lambda/LambdaTranslationTest1 - java.lang.AssertionError: expected [d:1234.000000] but found [d:1234,000000]
  • a4d4973: 8254318: Remove .hgtags
  • 8ad9018: 8285400: Add '@APinote' to the APIs defined in Java SE 8 MR 3
  • 3a5b2cd: 8201793: (ref) Reference object should not support cloning
  • 1b52b01: 8183107: PKCS11 regression regarding checkKeySize
  • e633df1: 8175797: (ref) Reference::enqueue method should clear the reference object before enqueuing
  • 5323ef6: 8254178: Remove .hgignore
  • 4cc462a: 8214427: probable bug in logic of ConcurrentHashMap.addCount()
  • ... and 33 more: https://git.openjdk.org/jdk8u-dev/compare/83e90957bef15267bed792ee5d8d65899a2487e8...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 (@phohensee, @gnu-andrew) 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 Jul 18, 2022
@lusou-zhangquan
Copy link
Author

/integrate

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

openjdk bot commented Jul 19, 2022

@lusou-zhangquan
Your change (at version 0846705) is now ready to be sponsored by a Committer.

@jerboaa
Copy link
Contributor

jerboaa commented Jul 19, 2022

@lusou-zhangquan Please only issue /integrate once you have jdk8u-fix-yes on the bug. See "Contributing" section in https://wiki.openjdk.org/display/jdk8u In particular steps 10 and 11.

@phohensee
Copy link
Member

@ lusou-zhangquan, I strongly suspect the copyright date conflict in InetAddressCachePolicy.java would cause a merge failure if I sponsored this. Once you fix it I'll re-review and sponsor.

@openjdk openjdk bot removed the sponsor Pull request is ready to be sponsored label Aug 16, 2022
@lusou-zhangquan
Copy link
Author

lusou-zhangquan commented Aug 16, 2022

I've approved this one, but as I commented on the bug, it would be nice to know the reasoning for this backport. Approvals are primarily about risk assessment in the context of the whole JDK.

In this case, it's been in later JDKs for years, and is realistically borderline between being an enhancement and a bug fix (see https://bugs.openjdk.org/browse/JDK-8171346). I think it would be preferable to deal with any issues in the new code rather than the current outdated code, which is doing its own locking that is prone to error.

This bug is reproduced in our DNS service, which is running on jdk8 and hard to be upgraded to newer versions. So we hope to backport it and eliminate this potential risk for all jdk8 users.

@lusou-zhangquan
Copy link
Author

@ lusou-zhangquan, I strongly suspect the copyright date conflict in InetAddressCachePolicy.java would cause a merge failure if I sponsored this. Once you fix it I'll re-review and sponsor.

Copyright date has been updated in latest commit, thanks for your suggestion.

Copy link
Member

@gnu-andrew gnu-andrew left a comment

Choose a reason for hiding this comment

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

Wait, what? Why are we changing the copyright dates? This is not part of the original patch. Please revert this change.

@gnu-andrew
Copy link
Member

/reviewers 2 reviewer

@openjdk
Copy link

openjdk bot commented Aug 16, 2022

@gnu-andrew
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Aug 16, 2022
@phohensee
Copy link
Member

That was my bad. Somehow I thought the original patch updated the copyright date. Please revert to the previous version.

@gnu-andrew
Copy link
Member

That was my bad. Somehow I thought the original patch updated the copyright date. Please revert to the previous version.

No worries. I just couldn't work out the issue you were referring to and indeed there is no 2022 bump in the original patch.

Copy link
Member

@gnu-andrew gnu-andrew left a comment

Choose a reason for hiding this comment

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

Ok, let's try again :)

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Aug 22, 2022
@phohensee
Copy link
Member

Just need an integrate comment. :)

@lusou-zhangquan
Copy link
Author

/summary
8049228: Improve multithreaded scalability of InetAddress cache
7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime for more precise and accurate

Reviewed-by: phh, andrew

@openjdk
Copy link

openjdk bot commented Aug 24, 2022

@lusou-zhangquan Setting summary to:

8049228: Improve multithreaded scalability of InetAddress cache
7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime for more precise and accurate

Reviewed-by: phh, andrew

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Aug 24, 2022
@lusou-zhangquan
Copy link
Author

lusou-zhangquan commented Aug 24, 2022

/summary

@lusou-zhangquan
Copy link
Author

/summary 8049228: Improve multithreaded scalability of InetAddress cache 7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime for more precise and accurate

Reviewed-by: phh, andrew

I intended to update summary of this PR with this summary command before integrate, but it looks like not working as expected. And now jcheck failed with this error message:

Exception occurred during jcheck - the operation will be retried

Incorrectly formatted commit message: 8049228: Improve multithreaded scalability of InetAddress cache
7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime for more precise and accurate

8049228: Improve multithreaded scalability of InetAddress cache
7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime for more precise and accurate

Reviewed-by: phh, andrew

Reviewed-by: phh, Andrew

Is this my operation fault or jcheck bug? Could anybody help to solve this problem. Thanks a lot.

@jerboaa
Copy link
Contributor

jerboaa commented Aug 24, 2022

/summary 8049228: Improve multithreaded scalability of InetAddress cache 7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime for more precise and accurate
Reviewed-by: phh, andrew

I intended to update summary of this PR with this summary command before integrate, but it looks like not working as expected. And now jcheck failed with this error message:

Exception occurred during jcheck - the operation will be retried

Incorrectly formatted commit message: 8049228: Improve multithreaded scalability of InetAddress cache
7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime for more precise and accurate

8049228: Improve multithreaded scalability of InetAddress cache
7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime for more precise and accurate

Reviewed-by: phh, andrew

Reviewed-by: phh, Andrew

Is this my operation fault or jcheck bug? Could anybody help to solve this problem. Thanks a lot.

Don't set the commit message like that using /summary. What you want is /issue add. Reviewed-by fields are set by the bots based on the approving reviewers, so it's no surprise it didn't like that. That being said, for all intents an purposes what the bot suggested in #70 (comment) pretty much matches what you want, no? What is it that you want to achieve exactly?

@jerboaa
Copy link
Contributor

jerboaa commented Aug 24, 2022

/summary
Remove me

@jerboaa
Copy link
Contributor

jerboaa commented Aug 24, 2022

@lusou-zhangquan It looks like that has broken the bots. Best to file a SKARA issue here: https://bugs.openjdk.org/browse/SKARA if you can't create issues, I can help with that.

@gnu-andrew
Copy link
Member

It's probably simpler to just open a fresh PR.

@lusou-zhangquan
Copy link
Author

Close this PR and open a fresh one because of broken bots.

@openjdk
Copy link

openjdk bot commented Aug 25, 2022

@lusou-zhangquan Removing existing summary

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Aug 25, 2022
@openjdk
Copy link

openjdk bot commented Aug 25, 2022

@jerboaa Only the author (@lusou-zhangquan) is allowed to issue the /summary command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport ready Pull request is ready to be integrated rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

4 participants