From 15471e6020f1803938521fa20eabacda64c68922 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 Sep 2020 14:28:45 -0400 Subject: [PATCH 01/10] Speed up writeVInt This speeds up `StreamOutput#writeVInt` quite a bit which is nice because it is *very* commonly called when serializing aggregations. Well, when serializing anything. All "collections" serialize their size as a vint. Anyway, I was examining the serialization speeds of `StringTerms` and this saves about 30% of the write time for that. I expect it'll be useful other places. --- benchmarks/README.md | 1 - .../StringTermsSerializationBenchmark.java | 77 +++++++++++++++++++ .../common/io/stream/StreamOutput.java | 61 +++++++++++++-- .../common/io/stream/BytesStreamsTests.java | 11 ++- 4 files changed, 141 insertions(+), 9 deletions(-) create mode 100644 benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java diff --git a/benchmarks/README.md b/benchmarks/README.md index 1be31c4a38c48..1e7b08547dc1a 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -78,7 +78,6 @@ cd fcml* make cd example/hsdis make -cp .libs/libhsdis.so.0.0.0 sudo cp .libs/libhsdis.so.0.0.0 /usr/lib/jvm/java-14-adoptopenjdk/lib/hsdis-amd64.so ``` diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java new file mode 100644 index 0000000000000..ac1bc8f16c090 --- /dev/null +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java @@ -0,0 +1,77 @@ +package org.elasticsearch.benchmark.search.aggregations.bucket.terms; + +import org.apache.lucene.util.BytesRef; +import org.elasticsearch.Version; +import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.io.stream.BytesStreamOutput; +import org.elasticsearch.common.io.stream.DelayableWriteable; +import org.elasticsearch.common.io.stream.NamedWriteableRegistry; +import org.elasticsearch.search.DocValueFormat; +import org.elasticsearch.search.aggregations.BucketOrder; +import org.elasticsearch.search.aggregations.InternalAggregation; +import org.elasticsearch.search.aggregations.InternalAggregations; +import org.elasticsearch.search.aggregations.bucket.terms.StringTerms; +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.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.TimeUnit; + +@Fork(2) +@Warmup(iterations = 10) +@Measurement(iterations = 5) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.MILLISECONDS) +@State(Scope.Benchmark) +public class StringTermsSerializationBenchmark { + private static final NamedWriteableRegistry REGISTRY = new NamedWriteableRegistry( + List.of(new NamedWriteableRegistry.Entry(InternalAggregation.class, StringTerms.NAME, StringTerms::new)) + ); + @Param(value = { "1000" }) + private int buckets; + + private DelayableWriteable results; + + @Setup + public void initResults() { + results = DelayableWriteable.referencing(InternalAggregations.from(List.of(newTerms(true)))); + } + + private StringTerms newTerms(boolean withNested) { + List resultBuckets = new ArrayList<>(buckets); + for (int i = 0; i < buckets; i++) { + InternalAggregations inner = withNested ? InternalAggregations.from(List.of(newTerms(false))) : InternalAggregations.EMPTY; + resultBuckets.add(new StringTerms.Bucket(new BytesRef("test" + i), i, inner, false, 0, DocValueFormat.RAW)); + } + return new StringTerms( + "test", + BucketOrder.key(true), + BucketOrder.key(true), + buckets, + 1, + null, + DocValueFormat.RAW, + buckets, + false, + 100000, + resultBuckets, + 0 + ); + } + + @Benchmark + public DelayableWriteable serialize() { + return results.asSerialized(InternalAggregations::readFrom, REGISTRY); + } +} diff --git a/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java b/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java index 8d1866506e267..44fc07d3d44e5 100644 --- a/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java +++ b/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java @@ -218,14 +218,61 @@ public void writeInt(int i) throws IOException { * using {@link #writeInt} */ public void writeVInt(int i) throws IOException { - final byte[] buffer = scratch.get(); - int index = 0; - while ((i & ~0x7F) != 0) { - buffer[index++] = ((byte) ((i & 0x7f) | 0x80)); - i >>>= 7; + /* + * Pick the number of bytes that we need based on the value and then + * encode the int, unrolling the loops by hand. This allows writing + * small numbers to use `writeByte` which is simple and fast. The + * unrolling saves a few comparisons and bitwise operations. All + * together this saves quite a bit of time compared to a naive + * implementation. + */ + if (i < 0x7f) { + if (i >= 0) { + writeByte((byte) i); + return; + } + byte[] buffer = scratch.get(); + buffer[0] = (byte) (i & 0x7f | 0x80); + buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); + buffer[2] = (byte) ((i >>> 14) & 0x7f | 0x80); + buffer[3] = (byte) ((i >>> 21) & 0x7f | 0x80); + buffer[4] = (byte) (i >>> 28); + assert buffer[4] <= 0x7f; + writeBytes(buffer, 0, 5); + return; } - buffer[index++] = ((byte) i); - writeBytes(buffer, 0, index); + byte[] buffer = scratch.get(); + if (i < 0x3fff) { + buffer[0] = (byte) (i & 0x7f | 0x80); + buffer[1] = (byte) (i >>> 7); + assert buffer[1] <= 0x7f; + writeBytes(buffer, 0, 2); + return; + } + if (i < 0x1f_ffff) { + buffer[0] = (byte) (i & 0x7f | 0x80); + buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); + buffer[2] = (byte) (i >>> 14); + assert buffer[2] <= 0x7f; + writeBytes(buffer, 0, 3); + return; + } + if (i < 0x0fff_ffff) { + buffer[0] = (byte) (i & 0x7f | 0x80); + buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); + buffer[2] = (byte) ((i >>> 14) & 0x7f | 0x80); + buffer[3] = (byte) (i >>> 21); + assert buffer[3] <= 0x7f; + writeBytes(buffer, 0, 4); + return; + } + buffer[0] = (byte) ((i & 0x7f) | 0x80); + buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); + buffer[2] = (byte) ((i >>> 14) & 0x7f | 0x80); + buffer[3] = (byte) ((i >>> 21) & 0x7f | 0x80); + buffer[4] = (byte) (i >>> 28); + assert buffer[4] <= 0x7f; + writeBytes(buffer, 0, 5); } /** diff --git a/server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java b/server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java index e91bf3a146ca8..f2aa39c8ee6df 100644 --- a/server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java +++ b/server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java @@ -61,7 +61,7 @@ import static org.hamcrest.Matchers.sameInstance; /** - * Tests for {@link BytesStreamOutput} paging behaviour. + * Tests for {@link StreamOutput}. */ public class BytesStreamsTests extends ESTestCase { public void testEmpty() throws Exception { @@ -829,6 +829,15 @@ public void testVInt() throws IOException { output.writeVInt(value); StreamInput input = output.bytes().streamInput(); assertEquals(value, input.readVInt()); + + BytesStreamOutput simple = new BytesStreamOutput(); + int i = value; + while ((i & ~0x7F) != 0) { + simple.writeByte(((byte) ((i & 0x7f) | 0x80))); + i >>>= 7; + } + simple.writeByte((byte) i); + assertEquals(simple.bytes().toBytesRef().toString(), output.bytes().toBytesRef().toString()); } public void testVLong() throws IOException { From c65a0b150e7e90a40fe9f398b3f1b614c8c685d1 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 Sep 2020 18:02:53 -0400 Subject: [PATCH 02/10] License headers --- .../StringTermsSerializationBenchmark.java | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java index ac1bc8f16c090..408eacf7fbb50 100644 --- a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java @@ -1,9 +1,25 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + package org.elasticsearch.benchmark.search.aggregations.bucket.terms; import org.apache.lucene.util.BytesRef; -import org.elasticsearch.Version; -import org.elasticsearch.common.bytes.BytesReference; -import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.DelayableWriteable; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.search.DocValueFormat; @@ -23,7 +39,6 @@ import org.openjdk.jmh.annotations.State; import org.openjdk.jmh.annotations.Warmup; -import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; From 5cd3531e65ce0f61a2fb3a41dcf3db7b8910bcd4 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 Sep 2020 20:12:12 -0400 Subject: [PATCH 03/10] Switch?! --- .../StringTermsSerializationBenchmark.java | 17 +++ .../common/io/stream/StreamOutput.java | 112 +++++++++++------- .../common/io/stream/BytesStreamsTests.java | 8 +- 3 files changed, 91 insertions(+), 46 deletions(-) diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java index 408eacf7fbb50..775c0741c4b84 100644 --- a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java @@ -20,6 +20,9 @@ package org.elasticsearch.benchmark.search.aggregations.bucket.terms; import org.apache.lucene.util.BytesRef; +import org.elasticsearch.Version; +import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.DelayableWriteable; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.search.DocValueFormat; @@ -39,6 +42,7 @@ import org.openjdk.jmh.annotations.State; import org.openjdk.jmh.annotations.Warmup; +import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; @@ -89,4 +93,17 @@ private StringTerms newTerms(boolean withNested) { public DelayableWriteable serialize() { return results.asSerialized(InternalAggregations::readFrom, REGISTRY); } + + @Benchmark + public BytesReference serializeVint() throws IOException { + try (BytesStreamOutput buffer = new BytesStreamOutput()) { + buffer.setVersion(Version.CURRENT); + for (int i = 0; i < 1000000; i++) { + buffer.writeVInt(i * 31); + buffer.reset(); + } + return buffer.bytes(); + } + } + } diff --git a/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java b/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java index 44fc07d3d44e5..601d083aa10e8 100644 --- a/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java +++ b/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java @@ -226,53 +226,77 @@ public void writeVInt(int i) throws IOException { * together this saves quite a bit of time compared to a naive * implementation. */ - if (i < 0x7f) { - if (i >= 0) { + switch (Integer.numberOfLeadingZeros(i)) { + case 32: + case 31: + case 30: + case 29: + case 28: + case 27: + case 26: + case 25: writeByte((byte) i); return; - } - byte[] buffer = scratch.get(); - buffer[0] = (byte) (i & 0x7f | 0x80); - buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); - buffer[2] = (byte) ((i >>> 14) & 0x7f | 0x80); - buffer[3] = (byte) ((i >>> 21) & 0x7f | 0x80); - buffer[4] = (byte) (i >>> 28); - assert buffer[4] <= 0x7f; - writeBytes(buffer, 0, 5); - return; - } - byte[] buffer = scratch.get(); - if (i < 0x3fff) { - buffer[0] = (byte) (i & 0x7f | 0x80); - buffer[1] = (byte) (i >>> 7); - assert buffer[1] <= 0x7f; - writeBytes(buffer, 0, 2); - return; - } - if (i < 0x1f_ffff) { - buffer[0] = (byte) (i & 0x7f | 0x80); - buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); - buffer[2] = (byte) (i >>> 14); - assert buffer[2] <= 0x7f; - writeBytes(buffer, 0, 3); - return; - } - if (i < 0x0fff_ffff) { - buffer[0] = (byte) (i & 0x7f | 0x80); - buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); - buffer[2] = (byte) ((i >>> 14) & 0x7f | 0x80); - buffer[3] = (byte) (i >>> 21); - assert buffer[3] <= 0x7f; - writeBytes(buffer, 0, 4); - return; + case 24: + case 23: + case 22: + case 21: + case 20: + case 19: + case 18: + byte[] buffer = scratch.get(); + buffer[0] = (byte) (i & 0x7f | 0x80); + buffer[1] = (byte) (i >>> 7); + assert buffer[1] <= 0x7f; + writeBytes(buffer, 0, 2); + return; + case 17: + case 16: + case 15: + case 14: + case 13: + case 12: + case 11: + buffer = scratch.get(); + buffer[0] = (byte) (i & 0x7f | 0x80); + buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); + buffer[2] = (byte) (i >>> 14); + assert buffer[2] <= 0x7f; + writeBytes(buffer, 0, 3); + return; + case 10: + case 9: + case 8: + case 7: + case 6: + case 5: + case 4: + buffer = scratch.get(); + buffer[0] = (byte) (i & 0x7f | 0x80); + buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); + buffer[2] = (byte) ((i >>> 14) & 0x7f | 0x80); + buffer[3] = (byte) (i >>> 21); + assert buffer[3] <= 0x7f; + writeBytes(buffer, 0, 4); + return; + case 3: + case 2: + case 1: + case 0: + buffer = scratch.get(); + buffer[0] = (byte) (i & 0x7f | 0x80); + buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); + buffer[2] = (byte) ((i >>> 14) & 0x7f | 0x80); + buffer[3] = (byte) ((i >>> 21) & 0x7f | 0x80); + buffer[4] = (byte) (i >>> 28); + assert buffer[4] <= 0x7f; + writeBytes(buffer, 0, 5); + return; + default: + throw new UnsupportedOperationException( + "Can't encode [" + i + "]. Missing case for [" + Integer.numberOfLeadingZeros(i) + "]?" + ); } - buffer[0] = (byte) ((i & 0x7f) | 0x80); - buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); - buffer[2] = (byte) ((i >>> 14) & 0x7f | 0x80); - buffer[3] = (byte) ((i >>> 21) & 0x7f | 0x80); - buffer[4] = (byte) (i >>> 28); - assert buffer[4] <= 0x7f; - writeBytes(buffer, 0, 5); } /** diff --git a/server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java b/server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java index f2aa39c8ee6df..4470d224c979b 100644 --- a/server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java +++ b/server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java @@ -19,6 +19,8 @@ package org.elasticsearch.common.io.stream; +import com.carrotsearch.randomizedtesting.annotations.Repeat; + import org.apache.lucene.store.AlreadyClosedException; import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.Constants; @@ -823,12 +825,11 @@ public void testReadNegativeArraySize() throws IOException { } } + @Repeat(iterations=1000) public void testVInt() throws IOException { final int value = randomInt(); BytesStreamOutput output = new BytesStreamOutput(); output.writeVInt(value); - StreamInput input = output.bytes().streamInput(); - assertEquals(value, input.readVInt()); BytesStreamOutput simple = new BytesStreamOutput(); int i = value; @@ -838,6 +839,9 @@ public void testVInt() throws IOException { } simple.writeByte((byte) i); assertEquals(simple.bytes().toBytesRef().toString(), output.bytes().toBytesRef().toString()); + + StreamInput input = output.bytes().streamInput(); + assertEquals(value, input.readVInt()); } public void testVLong() throws IOException { From 929297f3a72d6d80e26b9a415e1204b413c52079 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 Sep 2020 20:34:05 -0400 Subject: [PATCH 04/10] drop --- .../StringTermsSerializationBenchmark.java | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java index 775c0741c4b84..408eacf7fbb50 100644 --- a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java @@ -20,9 +20,6 @@ package org.elasticsearch.benchmark.search.aggregations.bucket.terms; import org.apache.lucene.util.BytesRef; -import org.elasticsearch.Version; -import org.elasticsearch.common.bytes.BytesReference; -import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.DelayableWriteable; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.search.DocValueFormat; @@ -42,7 +39,6 @@ import org.openjdk.jmh.annotations.State; import org.openjdk.jmh.annotations.Warmup; -import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; @@ -93,17 +89,4 @@ private StringTerms newTerms(boolean withNested) { public DelayableWriteable serialize() { return results.asSerialized(InternalAggregations::readFrom, REGISTRY); } - - @Benchmark - public BytesReference serializeVint() throws IOException { - try (BytesStreamOutput buffer = new BytesStreamOutput()) { - buffer.setVersion(Version.CURRENT); - for (int i = 0; i < 1000000; i++) { - buffer.writeVInt(i * 31); - buffer.reset(); - } - return buffer.bytes(); - } - } - } From ca6cd67ccd78ea75546606e160ca7ffc5deb86c7 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 Sep 2020 20:45:16 -0400 Subject: [PATCH 05/10] Repeat is forbidden --- .../org/elasticsearch/common/io/stream/BytesStreamsTests.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java b/server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java index 4470d224c979b..2f0a6184976f6 100644 --- a/server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java +++ b/server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java @@ -19,8 +19,6 @@ package org.elasticsearch.common.io.stream; -import com.carrotsearch.randomizedtesting.annotations.Repeat; - import org.apache.lucene.store.AlreadyClosedException; import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.Constants; @@ -825,7 +823,6 @@ public void testReadNegativeArraySize() throws IOException { } } - @Repeat(iterations=1000) public void testVInt() throws IOException { final int value = randomInt(); BytesStreamOutput output = new BytesStreamOutput(); From 5beaac827ced891220a6e8427136cfe1d605dc57 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 Sep 2020 21:16:18 -0400 Subject: [PATCH 06/10] Revert "drop" This reverts commit 929297f3a72d6d80e26b9a415e1204b413c52079. --- .../StringTermsSerializationBenchmark.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java index 408eacf7fbb50..775c0741c4b84 100644 --- a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java @@ -20,6 +20,9 @@ package org.elasticsearch.benchmark.search.aggregations.bucket.terms; import org.apache.lucene.util.BytesRef; +import org.elasticsearch.Version; +import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.DelayableWriteable; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.search.DocValueFormat; @@ -39,6 +42,7 @@ import org.openjdk.jmh.annotations.State; import org.openjdk.jmh.annotations.Warmup; +import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; @@ -89,4 +93,17 @@ private StringTerms newTerms(boolean withNested) { public DelayableWriteable serialize() { return results.asSerialized(InternalAggregations::readFrom, REGISTRY); } + + @Benchmark + public BytesReference serializeVint() throws IOException { + try (BytesStreamOutput buffer = new BytesStreamOutput()) { + buffer.setVersion(Version.CURRENT); + for (int i = 0; i < 1000000; i++) { + buffer.writeVInt(i * 31); + buffer.reset(); + } + return buffer.bytes(); + } + } + } From ec402169487cf53b8965c60b2aa4ef94da3a6db3 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 15 Sep 2020 08:16:15 -0400 Subject: [PATCH 07/10] Simplify --- .../StringTermsSerializationBenchmark.java | 2 +- .../common/io/stream/StreamOutput.java | 81 +++---------------- 2 files changed, 12 insertions(+), 71 deletions(-) diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java index 775c0741c4b84..71dcf52a8431c 100644 --- a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java @@ -99,7 +99,7 @@ public BytesReference serializeVint() throws IOException { try (BytesStreamOutput buffer = new BytesStreamOutput()) { buffer.setVersion(Version.CURRENT); for (int i = 0; i < 1000000; i++) { - buffer.writeVInt(i * 31); + buffer.writeVInt(i); buffer.reset(); } return buffer.bytes(); diff --git a/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java b/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java index 601d083aa10e8..e0099ca496e25 100644 --- a/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java +++ b/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java @@ -226,77 +226,18 @@ public void writeVInt(int i) throws IOException { * together this saves quite a bit of time compared to a naive * implementation. */ - switch (Integer.numberOfLeadingZeros(i)) { - case 32: - case 31: - case 30: - case 29: - case 28: - case 27: - case 26: - case 25: - writeByte((byte) i); - return; - case 24: - case 23: - case 22: - case 21: - case 20: - case 19: - case 18: - byte[] buffer = scratch.get(); - buffer[0] = (byte) (i & 0x7f | 0x80); - buffer[1] = (byte) (i >>> 7); - assert buffer[1] <= 0x7f; - writeBytes(buffer, 0, 2); - return; - case 17: - case 16: - case 15: - case 14: - case 13: - case 12: - case 11: - buffer = scratch.get(); - buffer[0] = (byte) (i & 0x7f | 0x80); - buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); - buffer[2] = (byte) (i >>> 14); - assert buffer[2] <= 0x7f; - writeBytes(buffer, 0, 3); - return; - case 10: - case 9: - case 8: - case 7: - case 6: - case 5: - case 4: - buffer = scratch.get(); - buffer[0] = (byte) (i & 0x7f | 0x80); - buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); - buffer[2] = (byte) ((i >>> 14) & 0x7f | 0x80); - buffer[3] = (byte) (i >>> 21); - assert buffer[3] <= 0x7f; - writeBytes(buffer, 0, 4); - return; - case 3: - case 2: - case 1: - case 0: - buffer = scratch.get(); - buffer[0] = (byte) (i & 0x7f | 0x80); - buffer[1] = (byte) ((i >>> 7) & 0x7f | 0x80); - buffer[2] = (byte) ((i >>> 14) & 0x7f | 0x80); - buffer[3] = (byte) ((i >>> 21) & 0x7f | 0x80); - buffer[4] = (byte) (i >>> 28); - assert buffer[4] <= 0x7f; - writeBytes(buffer, 0, 5); - return; - default: - throw new UnsupportedOperationException( - "Can't encode [" + i + "]. Missing case for [" + Integer.numberOfLeadingZeros(i) + "]?" - ); + if (Integer.numberOfLeadingZeros(i) >= 25) { + writeByte((byte) i); + return; } + byte[] buffer = scratch.get(); + int index = 0; + do { + buffer[index++] = ((byte) ((i & 0x7f) | 0x80)); + i >>>= 7; + } while ((i & ~0x7F) != 0); + buffer[index++] = ((byte) i); + writeBytes(buffer, 0, index); } /** From e35e1c5e94ce2aa8cd4c9a5b6f3017d9f0fe1fbd Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 15 Sep 2020 08:39:02 -0400 Subject: [PATCH 08/10] Explain differently --- .../elasticsearch/common/io/stream/StreamOutput.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java b/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java index e0099ca496e25..67d8b521640fc 100644 --- a/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java +++ b/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java @@ -219,12 +219,12 @@ public void writeInt(int i) throws IOException { */ public void writeVInt(int i) throws IOException { /* - * Pick the number of bytes that we need based on the value and then - * encode the int, unrolling the loops by hand. This allows writing - * small numbers to use `writeByte` which is simple and fast. The - * unrolling saves a few comparisons and bitwise operations. All - * together this saves quite a bit of time compared to a naive - * implementation. + * Shortcut writing single byte because it is very, very common and + * can skip grabbing the scratch buffer. This is marginally slower + * than hand unrolling the entire encoding loop but hand unrolling + * the encoding loop blows out the method size so it can't be inlined. + * In that case benchmarks of the method itself are faster but + * benchmarks of methods that use this method are slower. */ if (Integer.numberOfLeadingZeros(i) >= 25) { writeByte((byte) i); From 4f6d4d8bc489c83898516777ae54085ba49cd34e Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 15 Sep 2020 08:48:54 -0400 Subject: [PATCH 09/10] moar comment --- .../java/org/elasticsearch/common/io/stream/StreamOutput.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java b/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java index 67d8b521640fc..428468b06b115 100644 --- a/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java +++ b/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java @@ -225,6 +225,8 @@ public void writeVInt(int i) throws IOException { * the encoding loop blows out the method size so it can't be inlined. * In that case benchmarks of the method itself are faster but * benchmarks of methods that use this method are slower. + * This is philosophically in line with vint in general - it biases + * twoards being simple and fast for smaller numbers. */ if (Integer.numberOfLeadingZeros(i) >= 25) { writeByte((byte) i); From 4d1d7bd0599b311558f33be2582a0ad79e427320 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 15 Sep 2020 13:03:50 -0400 Subject: [PATCH 10/10] drop example benchmark. It was good to look at, but we don't need to commit it. --- .../StringTermsSerializationBenchmark.java | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java index 71dcf52a8431c..408eacf7fbb50 100644 --- a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java @@ -20,9 +20,6 @@ package org.elasticsearch.benchmark.search.aggregations.bucket.terms; import org.apache.lucene.util.BytesRef; -import org.elasticsearch.Version; -import org.elasticsearch.common.bytes.BytesReference; -import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.DelayableWriteable; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.search.DocValueFormat; @@ -42,7 +39,6 @@ import org.openjdk.jmh.annotations.State; import org.openjdk.jmh.annotations.Warmup; -import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; @@ -93,17 +89,4 @@ private StringTerms newTerms(boolean withNested) { public DelayableWriteable serialize() { return results.asSerialized(InternalAggregations::readFrom, REGISTRY); } - - @Benchmark - public BytesReference serializeVint() throws IOException { - try (BytesStreamOutput buffer = new BytesStreamOutput()) { - buffer.setVersion(Version.CURRENT); - for (int i = 0; i < 1000000; i++) { - buffer.writeVInt(i); - buffer.reset(); - } - return buffer.bytes(); - } - } - }