Fix contains in HttpFields name set and prove random access to HttpFields via EnumMap not worth it.

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

@@ -38,9 +38,16 @@
 /**
  * <p>An ordered collection of {@link HttpField}s that represent the HTTP headers
  * or HTTP trailers of an HTTP request or an HTTP response.</p>
+ *
  * <p>{@link HttpFields} is immutable and typically used in server-side HTTP requests
  * and client-side HTTP responses, while {@link HttpFields.Mutable} is mutable and
  * typically used in server-side HTTP responses and client-side HTTP requests.</p>
+ *
+ * <p>Access is always more efficient using {@link HttpHeader} keys rather than {@link String} field names.</p>
+ *
+ * <p>The primary implementations of {@code HttpFields} have been optimized assuming few
+ * lookup operations, thus typically if many {@link HttpField}s need to looked up, it may be
+ * better to use an {@link Iterator} to find multiple fields in a single iteration.</p>
  */
 public interface HttpFields extends Iterable<HttpField>, Supplier<HttpFields>
 {
@@ -350,10 +357,12 @@ default boolean contains(EnumSet<HttpHeader> headers)
     /**
      * <p>Returns whether this instance contains the given field name.</p>
      * <p>The comparison of field name is case-insensitive via
-     * {@link HttpField#is(String)}.
+     * {@link HttpField#is(String)}. If possible, it is more efficient to use
+     * {@link #contains(HttpHeader)}.
      *
      * @param name the case-insensitive field name to search for
      * @return whether this instance contains the given field name
+     * @see #contains(HttpHeader) 
      */
     default boolean contains(String name)
     {
@@ -412,14 +421,15 @@ default String getLast(HttpHeader header)
      * <p>Returns the encoded value of the first field with the given field name,
      * or {@code null} if no such field is present.</p>
      * <p>The comparison of field name is case-insensitive via
-     * {@link HttpField#is(String)}.</p>
+     * {@link HttpField#is(String)}. If possible, it is more efficient to use {@link #get(HttpHeader)}.</p>
      * <p>In case of multi-valued fields, the returned value is the encoded
      * value, including commas and quotes, as returned by {@link HttpField#getValue()}.</p>
      *
      * @param name the case-insensitive field name to search for
      * @return the raw value of the first field with the given field name,
      * or {@code null} if no such field is present
      * @see HttpField#getValue()
+     * @see #get(HttpHeader) 
      */
     default String get(String name)
     {
@@ -594,22 +604,23 @@ default Enumeration<String> getFieldNames()
      * <p>Returns a {@link Set} of the field names.</p>
      * <p>Case-sensitivity of the field names is preserved.</p>
      *
-     * @return a {@link Set} of the field names
+     * @return an immutable {@link Set} of the field names. Changes made to the
+     * {@code HttpFields} after this call are not reflected in the set.
      */
     default Set<String> getFieldNamesCollection()
     {
         Set<HttpHeader> seenByHeader = EnumSet.noneOf(HttpHeader.class);
-        Set<String> seenByName = null;
+        Set<String> buildByName = null;
         List<String> list = new ArrayList<>(size());
 
         for (HttpField f : this)
         {
             HttpHeader header = f.getHeader();
             if (header == null)
             {
-                if (seenByName == null)
-                    seenByName = new TreeSet<>(String::compareToIgnoreCase);
-                if (seenByName.add(f.getName()))
+                if (buildByName == null)
+                    buildByName = new TreeSet<>(String::compareToIgnoreCase);
+                if (buildByName.add(f.getName()))
                     list.add(f.getName());
             }
             else if (seenByHeader.add(header))
@@ -618,6 +629,8 @@ else if (seenByHeader.add(header))
             }
         }
 
+        Set<String> seenByName = buildByName;
+
         // use the list to retain a rough ordering
         return new AbstractSet<>()
         {
@@ -632,6 +645,14 @@ public int size()
             {
                 return list.size();
             }
+
+            @Override
+            public boolean contains(Object o)
+            {
+                if (o instanceof String s)
+                    return seenByName != null && seenByName.contains(s) || seenByHeader.contains(HttpHeader.CACHE.get(s));
+                return false;
+            }
         };
     }
 

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

@@ -20,12 +20,7 @@
 import java.util.stream.Stream;
 
 /**
- * HTTP Fields. A collection of HTTP header and or Trailer fields.
- *
- * <p>This class is not synchronized as it is expected that modifications will only be performed by a
- * single thread.
- *
- * <p>The cookie handling provided by this class is guided by the Servlet specification and RFC6265.
+ * An immutable implementation of {@link HttpFields}.
  */
 class ImmutableHttpFields implements HttpFields
 {
@@ -70,10 +65,9 @@ public boolean equals(Object o)
     {
         if (this == o)
             return true;
-        if (!(o instanceof org.eclipse.jetty.http.ImmutableHttpFields))
-            return false;
-
-        return isEqualTo((HttpFields)o);
+        if (o instanceof HttpFields httpFields)
+            return isEqualTo(httpFields);
+        return false;
     }
 
     @Override

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

@@ -67,7 +67,7 @@ protected MutableHttpFields(int capacity)
      */
     protected MutableHttpFields(HttpFields fields)
     {
-        if (fields instanceof ImmutableHttpFields immutable)
+        if (fields instanceof org.eclipse.jetty.http.ImmutableHttpFields immutable)
         {
             _immutable = true;
             _fields = immutable._fields;
@@ -180,7 +180,7 @@ else if (fields instanceof org.eclipse.jetty.http.MutableHttpFields mutable)
     public HttpFields asImmutable()
     {
         _immutable = true;
-        return new ImmutableHttpFields(_fields, _size);
+        return new org.eclipse.jetty.http.ImmutableHttpFields(_fields, _size);
     }
 
     private void copyImmutable()

jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java

@@ -25,6 +25,7 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.NoSuchElementException;
+import java.util.Set;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -352,16 +353,27 @@ public void testCaseInsensitive()
         assertThat(header.get("EXPECT"), is("100"));
         assertThat(header.get("eXpEcT"), is("100"));
         assertThat(header.get(HttpHeader.EXPECT), is("100"));
+        assertTrue(header.contains("expect"));
+        assertTrue(header.contains("Expect"));
+        assertTrue(header.contains("EXPECT"));
+        assertTrue(header.contains("eXpEcT"));
 
         assertThat(header.get("random"), is("value"));
         assertThat(header.get("Random"), is("value"));
         assertThat(header.get("RANDOM"), is("value"));
         assertThat(header.get("rAnDoM"), is("value"));
         assertThat(header.get("RaNdOm"), is("value"));
+        assertTrue(header.contains("random"));
+        assertTrue(header.contains("Random"));
+        assertTrue(header.contains("RANDOM"));
+        assertTrue(header.contains("rAnDoM"));
+        assertTrue(header.contains("RaNdOm"));
 
         assertThat(header.get("Accept-Charset"), is("UTF-8"));
         assertThat(header.get("accept-charset"), is("UTF-8"));
         assertThat(header.get(HttpHeader.ACCEPT_CHARSET), is("UTF-8"));
+        assertTrue(header.contains("Accept-Charset"));
+        assertTrue(header.contains("accept-charset"));
 
         assertThat(header.getValuesList("Accept-Charset"), contains("UTF-8", "UTF-16"));
         assertThat(header.getValuesList("accept-charset"), contains("UTF-8", "UTF-16"));
@@ -371,9 +383,19 @@ public void testCaseInsensitive()
         assertThat(header.get("Foo-Bar"), is("one"));
         assertThat(header.getValuesList("foo-bar"), contains("one", "two"));
         assertThat(header.getValuesList("Foo-Bar"), contains("one", "two"));
+        assertTrue(header.contains("foo-bar"));
+        assertTrue(header.contains("Foo-Bar"));
 
         // We know the order of the set is deterministic
-        assertThat(header.getFieldNamesCollection(), contains("expect", "RaNdOm", "Accept-Charset", "foo-bar"));
+        Set<String> names = header.getFieldNamesCollection();
+        assertThat(names, contains("expect", "RaNdOm", "Accept-Charset", "foo-bar"));
+        assertTrue(names.contains("expect"));
+        assertTrue(names.contains("Expect"));
+        assertTrue(names.contains("random"));
+        assertTrue(names.contains("accept-charset"));
+        assertTrue(names.contains("Accept-Charset"));
+        assertTrue(names.contains("foo-bar"));
+        assertTrue(names.contains("Foo-Bar"));
     }
 
     @ParameterizedTest

tests/jetty-jmh/src/main/java/org/eclipse/jetty/server/jmh/HashMapVsEnumMapBenchmark.java

@@ -0,0 +1,151 @@
+//
+// ========================================================================
+// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
+//
+// This program and the accompanying materials are made available under the
+// terms of the Eclipse Public License v. 2.0 which is available at
+// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
+// which is available at https://www.apache.org/licenses/LICENSE-2.0.
+//
+// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
+// ========================================================================
+//
+
+package org.eclipse.jetty.server.jmh;
+
+import java.util.ArrayList;
+import java.util.EnumMap;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.jetty.http.HttpField;
+import org.eclipse.jetty.http.HttpHeader;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OperationsPerInvocation;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.runner.Runner;
+import org.openjdk.jmh.runner.RunnerException;
+import org.openjdk.jmh.runner.options.Options;
+import org.openjdk.jmh.runner.options.OptionsBuilder;
+
+@State(Scope.Thread)
+@BenchmarkMode(Mode.Throughput)
+@Fork(1)
+@Warmup(iterations = 6, time = 2000, timeUnit = TimeUnit.MILLISECONDS)
+@Measurement(iterations = 3, time = 2000, timeUnit = TimeUnit.MILLISECONDS)
+public class HashMapVsEnumMapBenchmark
+{
+    private static final HttpHeader[] HEADERS = HttpHeader.values();
+    private static final HttpHeader[] HEADER_NAMES =
+    {
+        // These will be hits
+        HttpHeader.HOST,
+        HttpHeader.CONTENT_TYPE,
+        HttpHeader.CONTENT_LENGTH,
+        HttpHeader.ACCEPT,
+
+        // These will be misses
+        HttpHeader.TRANSFER_ENCODING,
+        HttpHeader.AUTHORIZATION
+    };
+
+    private List<HttpField> newHeaders()
+    {
+        List<HttpField> list = new ArrayList<>();
+        list.add(new HttpField(HttpHeader.HOST, "Localhost"));
+        list.add(new HttpField(HttpHeader.CONTENT_TYPE, "application/json"));
+        list.add(new HttpField(HttpHeader.CONTENT_LENGTH, "123"));
+        list.add(new HttpField(HttpHeader.USER_AGENT, "JMH Benchmark"));
+        list.add(new HttpField(HttpHeader.ACCEPT, "application/json"));
+        return list;
+    }
+
+    @Benchmark
+    @OperationsPerInvocation(5)
+    public long testListLookup()
+    {
+        // Build the HashMap
+        List<HttpField> list = newHeaders();
+
+        // Perform lookups
+        long result = 0;
+        for (HttpHeader header : HEADER_NAMES)
+        {
+            for (HttpField field : list)
+            {
+                if (field.getHeader() == header)
+                {
+                    result ^= field.getValue().hashCode();
+                    break;
+                }
+            }
+        }
+        return result;
+    }
+
+    @Benchmark
+    @OperationsPerInvocation(5)
+    public long testHashMapBuildAndLookup()
+    {
+        // Build the HashMap
+        List<HttpField> list = newHeaders();
+        Map<String, HttpField> hashMap = new HashMap<>();
+        for (HttpField field : list)
+        {
+            hashMap.put(field.getName(), field);
+        }
+
+        // Perform lookups
+        long result = 0;
+        for (HttpHeader header : HEADER_NAMES)
+        {
+            HttpField field = hashMap.get(header.asString());
+            if (field != null)
+                result ^= field.getValue().hashCode();
+        }
+        return result;
+    }
+
+    @Benchmark
+    @OperationsPerInvocation(5)
+    public long testEnumMapBuildAndLookup()
+    {
+        // Build the EnumMap
+        Map<HttpHeader, HttpField> enumMap = new EnumMap<>(HttpHeader.class);
+
+        List<HttpField> list = newHeaders();
+        for (HttpField field : list)
+        {
+            enumMap.put(field.getHeader(), field);
+        }
+
+        // Perform lookups
+        long result = 0;
+        for (HttpHeader header : HEADERS)
+        {
+            HttpField field = enumMap.get(header);
+            if (field != null)
+                result ^= field.getValue().hashCode();
+        }
+        return result;
+    }
+
+    public static void main(String[] args) throws RunnerException
+    {
+        Options opt = new OptionsBuilder()
+            .include(HashMapVsEnumMapBenchmark.class.getSimpleName())
+            // .addProfiler(GCProfiler.class)
+            .forks(1)
+            .build();
+
+        new Runner(opt).run();
+    }
+}