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

8253179: Replace LinkedList Impl in net.http.Http2Connection #431

Closed
wants to merge 2 commits into from

Conversation

c-cleary
Copy link
Contributor

@c-cleary c-cleary commented Sep 30, 2020

This patch replaces a LinkedList data structure used in the net.http.Http2Connection class with an ArrayList. This issue relates to JDK-8246048: Replace LinkedList with ArrayLists in java.net.

Some justifications for this change are as follows:

  • Sequential Access Times for ArrayLists are improved due to locality of reference (i.e ArrayList elements stored in same memory neighborhood)
  • Get(index) operations are O(1) time complexity for ArrayLists as opposed to worst-case O(N-1) for LinkedLists
  • While insertion operations can be expensive (O(N) in the worst case), these operations appear to be infrequent/non-existent in this case.

Additional justifications or challenges to those listed are welcome! The general idea is that ArrayLists out-perform LinkedLists in this scenario.


Progress

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

Issue

  • JDK-8253179: Replace LinkedList Impl in net.http.Http2Connection

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/431/head:pull/431
$ git checkout pull/431

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 30, 2020

👋 Welcome back ccleary! 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
Copy link

@openjdk openjdk bot commented Sep 30, 2020

@ccleary-oracle The following label will be automatically applied to this pull request:

  • net

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 net label Sep 30, 2020
@c-cleary c-cleary marked this pull request as ready for review Oct 1, 2020
@openjdk openjdk bot added the rfr label Oct 1, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 1, 2020

Webrevs

@@ -700,7 +700,7 @@ void shutdown(Throwable t) {
Throwable initialCause = this.cause;
if (initialCause == null) this.cause = t;
client2.deleteConnection(this);
List<Stream<?>> c = new LinkedList<>(streams.values());
List<Stream<?>> c = new ArrayList<>(streams.values());
Copy link
Contributor

@shipilev shipilev Oct 1, 2020

Choose a reason for hiding this comment

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

Why can't we dispense with this copy completely, and instead just iterate streams.values()?

Copy link
Member

@dfuch dfuch Oct 1, 2020

Choose a reason for hiding this comment

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

Good point Aleksey. I guess that the original intent was to avoid ConcurrentModificationException. However I see that streams is a ConcurrentHashMap now. So maybe we could dispense of the copy:

private final Map<Integer,Stream<?>> streams = new ConcurrentHashMap<>();

Could be worth changing the type from Map<Integer,Stream<?>> to ConcurrentMap<Integer,Stream<?>> in the declaration above if we decide to get rid of the copy though.

Copy link
Contributor Author

@c-cleary c-cleary Oct 1, 2020

Choose a reason for hiding this comment

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

Taking a look at this presently

Copy link
Member

@pavelrappo pavelrappo Oct 1, 2020

Choose a reason for hiding this comment

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

Although Aleksey's proposal does not change the behavior, it raises a question (to be explored separately): can a stream be added to that map while Http2Connection.shutdown is in progress? If this is the case, then the method is racy.

Copy link
Member

@dfuch dfuch Oct 1, 2020

Choose a reason for hiding this comment

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

Even if that happened , it should not matter as the connection will be (or is) closed anyway. I don't believe that's the kind of scenario that the copy was trying to prevent. Closing a stream will definitely remove it from the map though, so my guess that is where the CME would happen if no precaution was taken.

@@ -700,8 +700,7 @@ void shutdown(Throwable t) {
Throwable initialCause = this.cause;
if (initialCause == null) this.cause = t;
client2.deleteConnection(this);
List<Stream<?>> c = new ArrayList<>(streams.values());
for (Stream<?> s : c) {
for (Stream<?> s : streams.values()) {
try {
Copy link
Contributor Author

@c-cleary c-cleary Oct 2, 2020

Choose a reason for hiding this comment

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

To address some of the feedback given, the streams are now directly addressed instead of making a copy. ConcurrentMap has also been used to initialise streams instead of Map. Seeking feedback/opinions on this if possible

@c-cleary c-cleary changed the title 8253179: Replace LinkedList Impl in net.http.Http2Connection 8253179: WIP Replace LinkedList Impl in net.http.Http2Connection Oct 2, 2020
@c-cleary c-cleary changed the title 8253179: WIP Replace LinkedList Impl in net.http.Http2Connection [WIP] 8253179: Replace LinkedList Impl in net.http.Http2Connection Oct 2, 2020
@openjdk openjdk bot removed the rfr label Oct 2, 2020
@c-cleary c-cleary changed the title [WIP] 8253179: Replace LinkedList Impl in net.http.Http2Connection 8253179: Replace LinkedList Impl in net.http.Http2Connection Oct 5, 2020
@openjdk openjdk bot added the rfr label Oct 5, 2020
dfuch
dfuch approved these changes Oct 6, 2020
Copy link
Member

@dfuch dfuch left a comment

Looks good to me Conor.
best regards

-- daniel

@openjdk
Copy link

@openjdk openjdk bot commented Oct 6, 2020

@ccleary-oracle 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 more details.

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

8253179: Replace LinkedList Impl in net.http.Http2Connection

Reviewed-by: dfuchs, prappo, chegar, shade

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

  • 54b340b: 8254081: java/security/cert/PolicyNode/GetPolicyQualifiers.java fails due to an expired certificate
  • 51fdb4c: 8254075: Shenandoah: Remove ShenandoahCodeRootsStyle diagnostic flag and related test
  • 77921b9: 8254080: fix for JDK-8204256 causes jlink test failures
  • 57493c1: 8253694: Remove Thread::muxAcquire() from ThreadCrashProtection()
  • d2b1dc6: 8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command
  • a34f48b: 8253832: CharsetDecoder : decode() mentioning CoderMalfunctionError behavior not as per spec
  • f397b60: 8251123: doclint warnings about missing javadoc tags and comments
  • c9d1dcc: 8253902: G1: Starting a new marking cycle before the conc mark thread fully completed causes assertion failure
  • 9199783: 8253565: PPC64: Fix duplicate if condition in vm_version_ppc.cpp
  • 1728547: 8254010: GrowableArrayView::print fails to compile
  • ... and 70 more: https://git.openjdk.java.net/jdk/compare/04775f11fe637029b6040b494bb495939c6d3fcf...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 (@dfuch, @pavelrappo, @ChrisHegarty, @shipilev) 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 label Oct 6, 2020
Copy link
Member

@pavelrappo pavelrappo left a comment

This looks good.

@c-cleary
Copy link
Contributor Author

@c-cleary c-cleary commented Oct 6, 2020

/integrate

@openjdk openjdk bot added the sponsor label Oct 6, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 6, 2020

@ccleary-oracle
Your change (at version fc3f574) is now ready to be sponsored by a Committer.

@shipilev
Copy link
Contributor

@shipilev shipilev commented Oct 6, 2020

I can sponsor this, but it is not clear what testing was done on this. At least tier1?

@c-cleary
Copy link
Contributor Author

@c-cleary c-cleary commented Oct 6, 2020

I can sponsor this, but it is not clear what testing was done on this. At least tier1?

tier1, tier2, jdk_net, test/jdk/java/net/httpclient tests were conducted with all passing.

@shipilev
Copy link
Contributor

@shipilev shipilev commented Oct 7, 2020

/sponsor

@openjdk openjdk bot closed this Oct 7, 2020
@openjdk openjdk bot added integrated and removed sponsor labels Oct 7, 2020
@openjdk openjdk bot removed ready rfr labels Oct 7, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 7, 2020

@shipilev @ccleary-oracle Since your change was applied there have been 86 commits pushed to the master branch:

  • 3973073: 8253977: More memory leaks in client-libs on macOS
  • 2a0389a: 8254095: remove jdk.test.lib.Utils::distro() method
  • ac772cd: 8253750: use build-stable default seed for Utils.RANDOM_GENERATOR
  • 6712f8c: 8254084: Remove TemplateTable::pd_initialize
  • 82fe023: 8254061: Missing space in flag description
  • bd50ccd: 8253735: Cleanup SearchIndexItem API
  • 54b340b: 8254081: java/security/cert/PolicyNode/GetPolicyQualifiers.java fails due to an expired certificate
  • 51fdb4c: 8254075: Shenandoah: Remove ShenandoahCodeRootsStyle diagnostic flag and related test
  • 77921b9: 8254080: fix for JDK-8204256 causes jlink test failures
  • 57493c1: 8253694: Remove Thread::muxAcquire() from ThreadCrashProtection()
  • ... and 76 more: https://git.openjdk.java.net/jdk/compare/04775f11fe637029b6040b494bb495939c6d3fcf...master

Your commit was automatically rebased without conflicts.

Pushed as commit 703b345.

💡 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
integrated net
5 participants