MutableHttpFields.asImmutable avoids copy #10651

gregw · 2023-10-02T12:02:52Z

Avoid a copy in MutableHttpFields.asImmutable if the mutable is never mutated again.
Other minor improvements are included:

Better iterator usage
Use StringUtil.asciiEqualsIgnoreCase for HttpHeader#is(String)
Case sensitive HttpMethod#is(String)

Avoid a copy in MutableHttpFields.asImmutable if the mutable is never mutated again.

Signed-off-by: Ludovic Orban <[email protected]>

joakime · 2023-10-02T16:22:47Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ImmutableHttpFields.java

@@ -59,7 +58,7 @@ public HttpFields asImmutable()
    public int hashCode()
    {
        int hash = 0;


shouldn't the hash function start with an arbitrary prime number? (not 0)

Agreed, since we do xor's in the loop, starting with a hash of 31 would improve the spreading.

@gregw this should be a prime number.

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ManagedSelector.java

lorban · 2023-10-02T16:37:16Z

@gregw FYI the testBasicAuthenticationWithAuthenticationRemoved test failures are caused by your MutableHttpFields.asImmutable() changes.

Avoid a copy in MutableHttpFields.asImmutable if the mutable is never mutated again.

Do not persist a defaulted charset used in the request. Throw UnsupportedEncodingException from getReader Improve performance with asciiEqualsIgnoreCase HttpMethod is case-insensitive

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpMethod.java

lorban · 2023-10-03T07:52:31Z

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java

+     * @return true if the string ends with the substring, ignoring {@link StandardCharsets#US_ASCII}  case differences.
+     * @deprecated Use {@link #asciiEndsWithIgnoreCase(String, String)}
+     */
+    @Deprecated


@Deprecated(since = "12.0.3") ?

I'm not against the since attribute, but either we are consistent, run through the code add it to all deprecations, and we have a policy, otherwise it's not so useful and inconsistent.

Signed-off-by: Ludovic Orban <[email protected]>

…lations EnumSet Signed-off-by: Ludovic Orban <[email protected]>

…cation Signed-off-by: Ludovic Orban <[email protected]>

joakime

Some open thoughts

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpMethod.java

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MutableHttpFields.java

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java

joakime

The hashCode() implementation in ImmutableHttpFields is non-ideal.
But that hints at a larger issue in the codebase.
Probably should review that across the codebase in a future PR.

joakime · 2023-10-04T21:11:56Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ImmutableHttpFields.java

@@ -59,7 +58,7 @@ public HttpFields asImmutable()
    public int hashCode()
    {
        int hash = 0;


@gregw this should be a prime number.

…/HttpFields_asImmutable

prime hash seeds

gregw · 2023-10-04T23:25:21Z

@joakime this is already a bit of an omnibus PR, so I have fixed the other hash calculations I can see that are based on 0.

sbordet · 2023-10-05T13:25:29Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MutableHttpFields.java

+        }
+        else if (fields != null)
+        {
+            _fields = new HttpField[fields.size() + INITIAL_SIZE];


Suggested change

_fields = new HttpField[fields.size() + INITIAL_SIZE];

_fields = new HttpField[fields.size() + SIZE_INCREMENT];

lorban · 2023-10-05T15:18:15Z

@gregw I have an extra set of small but potentially controversial changes worth 1-2% of perf in my perf/jetty-12-HttpURI-violations branch. Would you like me to push them here?

Signed-off-by: Ludovic Orban <[email protected]>

gregw · 2023-10-06T11:14:30Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java

@@ -479,10 +479,10 @@ private static class RetainedBucket
        private RetainedBucket(int capacity, int poolSize)
        {
            if (poolSize <= ConcurrentPool.OPTIMAL_MAX_SIZE)
-                _pool = new ConcurrentPool<>(ConcurrentPool.StrategyType.THREAD_ID, poolSize, true);
+                _pool = new ConcurrentPool<>(ConcurrentPool.StrategyType.THREAD_ID, poolSize, false);


@lorban I like this change as THREAD_ID and the ThreadLocal cache are kind of trying to do the same thing anyway.
However, I wonder if we could improve the THREAD_ID algorithm a little bit. Currently it is used as:

case THREAD_ID -> (int)(Thread.currentThread().getId() % size);

The problem here is that typically the threads in use will be consecutive IDs, so if there are 50 threads in use and the pool is 100 in size, it may be that 50 consecutive entries are all in use and that an acquire by a thread that already has used one buffer will now need to scan over 49 others to find an empty slot. Perhaps we should be treating it a little more like an initial hash and spreading the thread ID over the available size more. Perhaps the following would work a little bit better:

case THREAD_ID -> (int)((Thread.currentThread().getId() * 31) % size);

(31 selected because it is prime)

I modified the THREAD_ID strategy to multiply by 31, ran the benchmark, and measured no noticeable change.

Using the thread id as an index combined with the randomness of the OS' scheduler is already spreading the threads sufficiently well, it seems.

Still it might be best to keep the * 31. It is probably rare that all threads need buffers at exactly the same time, but I also think it could occur. It may be the benchmark is not the right pattern to make it happen. So you saw no cost to doing this, so let's keep it.

I wrote a micro benchmark to see exactly what's going on and the * 31 costs exactly two cheap instructions: shift left by 5, then subtract once.

No big deal, so I'm okay to keep this change.

Signed-off-by: Ludovic Orban <[email protected]>

gregw · 2023-10-11T20:57:33Z

@sbordet nudge

MutableHttpFields.asImmutable avoids copy

cf16e0e

Avoid a copy in MutableHttpFields.asImmutable if the mutable is never mutated again.

gregw requested review from sbordet and lorban October 2, 2023 12:03

lorban added 2 commits October 2, 2023 15:31

reduce instance creations needed for iterations

3a5ad7c

Signed-off-by: Ludovic Orban <[email protected]>

fix getField

a05631c

Signed-off-by: Ludovic Orban <[email protected]>

joakime reviewed Oct 2, 2023

View reviewed changes

lorban reviewed Oct 2, 2023

View reviewed changes

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ManagedSelector.java Outdated Show resolved Hide resolved

gregw added 4 commits October 3, 2023 09:20

Merge branch 'jetty-12.0.x' into experiment/12/HttpFields_asImmutable

822a439

MutableHttpFields.asImmutable avoids copy

f55fb7e

Avoid a copy in MutableHttpFields.asImmutable if the mutable is never mutated again.

Fix for issue with #10563

03c4b6b

Do not persist a defaulted charset used in the request. Throw UnsupportedEncodingException from getReader Improve performance with asciiEqualsIgnoreCase HttpMethod is case-insensitive

Merge branch 'jetty-12.0.x' into experiment/12/HttpFields_asImmutable

a038a51

lorban reviewed Oct 3, 2023

View reviewed changes

lorban and others added 4 commits October 3, 2023 10:01

make _requestAttributeListeners list lazy

8c228f9

Signed-off-by: Ludovic Orban <[email protected]>

make violations set lazy + add a quicker way to allocate an empty vio…

033c399

…lations EnumSet Signed-off-by: Ludovic Orban <[email protected]>

revert iteration change in updateKeys() as that doesn't save any allo…

970d242

…cation Signed-off-by: Ludovic Orban <[email protected]>

Merge branch 'jetty-12.0.x' into experiment/12/HttpFields_asImmutable

8c03343

gregw requested review from lorban and joakime October 3, 2023 12:27

joakime reviewed Oct 3, 2023

View reviewed changes

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java Show resolved Hide resolved

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java Outdated Show resolved Hide resolved

lorban approved these changes Oct 3, 2023

View reviewed changes

sbordet requested changes Oct 3, 2023

View reviewed changes

updates from review

803df61

lorban previously approved these changes Oct 4, 2023

View reviewed changes

gregw requested review from sbordet and joakime October 4, 2023 12:57

joakime previously approved these changes Oct 4, 2023

View reviewed changes

gregw added 2 commits October 5, 2023 10:05

Merge remote-tracking branch 'origin/jetty-12.0.x' into experiment/12…

82535be

…/HttpFields_asImmutable

updates from review

296b7b4

prime hash seeds

sbordet requested changes Oct 5, 2023

View reviewed changes

gregw and others added 3 commits October 6, 2023 08:12

updates from review

0b262a3

simplify HttpStreamOverHTTP1 ids

901cfbc

Signed-off-by: Ludovic Orban <[email protected]>

Disable thread-local cache

63dd228

Signed-off-by: Ludovic Orban <[email protected]>

gregw commented Oct 6, 2023

View reviewed changes

multiply thread ID by a prime to make spreading more random

dc97245

Signed-off-by: Ludovic Orban <[email protected]>

gregw requested a review from sbordet October 10, 2023 01:46

sbordet approved these changes Oct 13, 2023

View reviewed changes

gregw dismissed stale reviews from joakime and lorban via dc97245 October 13, 2023 13:29

Merge branch 'jetty-12.0.x' into experiment/12/HttpFields_asImmutable

5de619e

gregw merged commit ffe80cd into jetty-12.0.x Oct 16, 2023
2 checks passed

gregw deleted the experiment/12/HttpFields_asImmutable branch October 16, 2023 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MutableHttpFields.asImmutable avoids copy #10651

MutableHttpFields.asImmutable avoids copy #10651

gregw commented Oct 2, 2023 •

edited

Loading

joakime Oct 2, 2023 •

edited

Loading

lorban Oct 2, 2023

joakime Oct 4, 2023

lorban commented Oct 2, 2023

lorban Oct 3, 2023

sbordet Oct 3, 2023

joakime left a comment

joakime left a comment

joakime Oct 4, 2023

gregw commented Oct 4, 2023

sbordet Oct 5, 2023

lorban commented Oct 5, 2023

gregw Oct 6, 2023

lorban Oct 6, 2023

gregw Oct 6, 2023

lorban Oct 9, 2023 •

edited

Loading

gregw commented Oct 11, 2023

	_fields = new HttpField[fields.size() + INITIAL_SIZE];
	_fields = new HttpField[fields.size() + SIZE_INCREMENT];

MutableHttpFields.asImmutable avoids copy #10651

MutableHttpFields.asImmutable avoids copy #10651

Conversation

gregw commented Oct 2, 2023 • edited Loading

joakime Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorban commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime left a comment

Choose a reason for hiding this comment

joakime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw commented Oct 4, 2023

Choose a reason for hiding this comment

lorban commented Oct 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorban Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

gregw commented Oct 11, 2023

gregw commented Oct 2, 2023 •

edited

Loading

joakime Oct 2, 2023 •

edited

Loading

lorban Oct 9, 2023 •

edited

Loading