8253178: Replace LinkedList Impl in net.http.FilterFactory #4721

fdesu · 2021-07-08T10:38:33Z

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

The list created once per HttpClient and filled with upfront known values (3 of them in the jdk.internal.net.http.HttpClientImpl#initFilters: AuthenticationFilter.class, RedirectFilter.class and depending on the presence of a cookieHandler - a CookieFilter.class).

Progress

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

Issue

JDK-8253178: Replace LinkedList Impl in net.http.FilterFactory

Reviewers

Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4721

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

Using diff file

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

bridgekeeper · 2021-07-08T10:39:47Z

👋 Welcome back fdesu! 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 · 2021-07-08T10:42:55Z

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

mlbridge · 2021-07-08T10:46:05Z

Webrevs

fdesu · 2021-07-09T08:18:50Z

src/java.net.http/share/classes/jdk/internal/net/http/FilterFactory.java


    public void addFilter(Class<? extends HeaderFilter> type) {
        filterClasses.add(type);
    }

-    LinkedList<HeaderFilter> getFilterChain() {
-        LinkedList<HeaderFilter> l = new LinkedList<>();
+    List<HeaderFilter> getFilterChain() {


I was thinking about the ways how FilterChain could be simplified. I played around with the List<Supplier<HeaderFilter>> instead of List<Class<? extends HeaderFilter>> and it seems that such change might improve the FilterFactory:

List<HeaderFilter> getFilterChain() { return filterSuppliers.stream() .map(Supplier::get) .toList(); }

There is no constructor reflective access anymore, hence no exception handling.
Drawback here would be that HttpClientImpl in the initFilters will need to add suppliers vs classes i.e. filters.addFilter(AuthenticationFilter::new); vs addFilter(AuthenticationFilter.class);.

I think FilterFactory looks a bit simpler this way.
What do you think, should we go ahead with the change?

fdesu · 2021-07-19T14:35:25Z

Hi, could anyone please help me out with this patch? I am glad to fix problems that you might spot here, otherwise it would be awesome if someone could sponsor this change!

fdesu · 2021-08-16T16:08:57Z

Hi, it would be great if anyone would have time to take a quick look on this pull request. I hope this change is still relevant and I am happy to have any feedback from you. Thanks!

dfuch · 2021-08-17T12:31:12Z

Hi Sergei - I will have a look. It may take some time until I come back to you (my apologies).

dfuch

LGTM. I can sponsor once you have integrated
.

openjdk · 2021-08-23T15:18:40Z

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

8253178: Replace LinkedList Impl in net.http.FilterFactory

Reviewed-by: dfuchs

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

594e516: 8272778: Consolidate is_instance and is_instance_inlined in java_lang_String
d542745: 8267894: Skip work for empty regions in G1 Full GC
741f58c: 8272417: ZGC: fastdebug build crashes when printing ClassLoaderData
b7f75c0: 8271142: package help is not displayed for missing X11/extensions/Xrandr.h
e8a289e: 8272609: Add string deduplication support to SerialGC
b690f29: 8269687: pauth_aarch64.hpp include name is incorrect
f77a1a1: 8272472: StackGuardPages test doesn't build with glibc 2.34
04a806e: 8270344: Session resumption errors
d85560e: 8267161: Write automated test case for JDK-4479161
1ea437a: 8272720: Fix the implementation of loop unrolling heuristic with LoopPercentProfileLimit
... and 380 more: https://git.openjdk.java.net/jdk/compare/a8f15427156b8095ee815fbe6ed14c25c1d4b374...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) 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).

fdesu · 2021-08-23T15:19:49Z

/integrate

openjdk · 2021-08-23T15:21:13Z

@fdesu
Your change (at version 3991ea5) is now ready to be sponsored by a Committer.

dfuch · 2021-08-24T14:48:53Z

/sponsor

openjdk · 2021-08-24T14:50:12Z

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

94f5e44: 8271258: @param with non-ascii variable names produces incorrect results
7454306: 8272526: Cleanup ThreadStateTransition class
0597cde: 8221360: Eliminate Shared_DirtyCardQ_lock
928b972: 8271930: Simplify end_card calculation in G1BlockOffsetTablePart::verify
7f80683: 8272783: Epsilon: Refactor tests to improve performance
22ef4f0: 5015261: NPE may be thrown if JDesktopIcon is set to null on a JInternalFrame
9bc0232: 8269223: -Xcheck:jni WARNINGs working with fonts on Linux
2ff4c01: 8271600: C2: CheckCastPP which should closely follow Allocate is sunk of a loop
ad92033: 8272736: [JVMCI] Add API for reading and writing JVMCI thread locals
709b591: 8272553: several hotspot runtime/CommandLine tests don't check exit code
... and 391 more: https://git.openjdk.java.net/jdk/compare/a8f15427156b8095ee815fbe6ed14c25c1d4b374...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-08-24T14:50:31Z

@dfuch @fdesu Pushed as commit 2309b7d.

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

openjdk bot added the rfr Pull request is ready for review label Jul 8, 2021

openjdk bot added the net net-dev@openjdk.org label Jul 8, 2021

fdesu commented Jul 9, 2021

View reviewed changes

fdesu added 2 commits July 19, 2021 12:17

8253178: Replace LinkedList Impl in net.http.FilterFactory

c2ddac9

8253178: Trim the size for list with headers

3991ea5

fdesu force-pushed the remove-usages-of-linkedlist-in-netHttpFilterFactory branch from 31ab71b to 3991ea5 Compare July 19, 2021 10:18

dfuch approved these changes Aug 23, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 23, 2021

openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 23, 2021

openjdk bot closed this Aug 24, 2021

openjdk bot added integrated Pull request has been integrated and 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 Aug 24, 2021

fdesu deleted the remove-usages-of-linkedlist-in-netHttpFilterFactory branch August 24, 2021 15:48

8253178: Replace LinkedList Impl in net.http.FilterFactory #4721

8253178: Replace LinkedList Impl in net.http.FilterFactory #4721

fdesu commented Jul 8, 2021 •

edited by openjdk bot

bridgekeeper bot commented Jul 8, 2021

openjdk bot commented Jul 8, 2021

mlbridge bot commented Jul 8, 2021 •

edited

fdesu Jul 9, 2021

fdesu commented Jul 19, 2021

fdesu commented Aug 16, 2021

dfuch commented Aug 17, 2021

dfuch left a comment •

edited

openjdk bot commented Aug 23, 2021 •

edited

fdesu commented Aug 23, 2021

openjdk bot commented Aug 23, 2021

dfuch commented Aug 24, 2021

openjdk bot commented Aug 24, 2021

openjdk bot commented Aug 24, 2021

8253178: Replace LinkedList Impl in net.http.FilterFactory #4721

8253178: Replace LinkedList Impl in net.http.FilterFactory #4721

Conversation

fdesu commented Jul 8, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 8, 2021

openjdk bot commented Jul 8, 2021

mlbridge bot commented Jul 8, 2021 • edited

Webrevs

fdesu Jul 9, 2021

Choose a reason for hiding this comment

fdesu commented Jul 19, 2021

fdesu commented Aug 16, 2021

dfuch commented Aug 17, 2021

dfuch left a comment • edited

Choose a reason for hiding this comment

openjdk bot commented Aug 23, 2021 • edited

fdesu commented Aug 23, 2021

openjdk bot commented Aug 23, 2021

dfuch commented Aug 24, 2021

openjdk bot commented Aug 24, 2021

openjdk bot commented Aug 24, 2021

fdesu commented Jul 8, 2021 •

edited by openjdk bot

mlbridge bot commented Jul 8, 2021 •

edited

dfuch left a comment •

edited

openjdk bot commented Aug 23, 2021 •

edited