-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Big arrays sliced from netty buffers (double) #90745
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
Changes from all commits
6ac0d35
b5ecbe3
9b07968
7f9f084
d77167e
94c75f3
8c41e92
ae57e3f
4c144f4
cb2887c
d54ee6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,9 @@ | |
|
|
||
| import org.apache.lucene.util.ArrayUtil; | ||
| import org.apache.lucene.util.RamUsageEstimator; | ||
| import org.elasticsearch.common.io.stream.StreamOutput; | ||
|
|
||
| import java.io.IOException; | ||
| import java.lang.invoke.MethodHandles; | ||
| import java.lang.invoke.VarHandle; | ||
| import java.nio.ByteOrder; | ||
|
|
@@ -24,6 +26,12 @@ | |
| */ | ||
| final class BigDoubleArray extends AbstractBigArray implements DoubleArray { | ||
|
|
||
| static { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we actually need this here as well? Maybe we can just make a static method for this somewhere at least since we really only use it once? Or put this in a bootstrap check? Seems strange to duplicate this check doesn't it?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I like this idea. I will attempt this in a followup pr.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've opened this pr for this change: #91801 |
||
| if (ByteOrder.nativeOrder() != ByteOrder.LITTLE_ENDIAN) { | ||
| throw new Error("The deserialization assumes this class is written with little-endian numbers."); | ||
| } | ||
| } | ||
|
|
||
| private static final BigDoubleArray ESTIMATOR = new BigDoubleArray(0, BigArrays.NON_RECYCLING_INSTANCE, false); | ||
|
|
||
| static final VarHandle VH_PLATFORM_NATIVE_DOUBLE = MethodHandles.byteArrayViewVarHandle(double[].class, ByteOrder.nativeOrder()); | ||
|
|
@@ -123,4 +131,21 @@ public static long estimateRamBytes(final long size) { | |
| public void set(long index, byte[] buf, int offset, int len) { | ||
| set(index, buf, offset, len, pages, 3); | ||
| } | ||
|
|
||
| @Override | ||
| public void writeTo(StreamOutput out) throws IOException { | ||
| int size = (int) this.size; | ||
| out.writeVInt(size * Double.BYTES); | ||
| int lastPageEnd = size % DOUBLE_PAGE_SIZE; | ||
| if (lastPageEnd == 0) { | ||
| for (byte[] page : pages) { | ||
| out.write(page); | ||
| } | ||
| return; | ||
| } | ||
| for (int i = 0; i < pages.length - 1; i++) { | ||
| out.write(pages[i]); | ||
| } | ||
| out.write(pages[pages.length - 1], 0, lastPageEnd * Double.BYTES); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should share this code with the int/long/whatever other types. It's nearly the same code. 🤷 sounds like a good follow up change. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| package org.elasticsearch.common.util; | ||
|
|
||
| import org.apache.lucene.util.RamUsageEstimator; | ||
| import org.elasticsearch.common.bytes.ReleasableBytesReference; | ||
| import org.elasticsearch.common.io.stream.StreamInput; | ||
| import org.elasticsearch.common.io.stream.StreamOutput; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| class ReleasableDoubleArray implements DoubleArray { | ||
| private static final long SHALLOW_SIZE = RamUsageEstimator.shallowSizeOfInstance(ReleasableDoubleArray.class); | ||
|
|
||
| private final ReleasableBytesReference ref; | ||
|
|
||
| ReleasableDoubleArray(StreamInput in) throws IOException { | ||
| ref = in.readReleasableBytesReference(); | ||
| } | ||
|
|
||
| @Override | ||
| public void writeTo(StreamOutput out) throws IOException { | ||
| out.writeBytesReference(ref); | ||
| } | ||
|
|
||
| @Override | ||
| public long size() { | ||
| return ref.length() / Long.BYTES; | ||
| } | ||
|
|
||
| @Override | ||
| public double get(long index) { | ||
| if (index > Integer.MAX_VALUE / Long.BYTES) { | ||
| // We can't serialize messages longer than 2gb anyway | ||
| throw new ArrayIndexOutOfBoundsException(); | ||
| } | ||
| return ref.getDoubleLE((int) index * Long.BYTES); | ||
| } | ||
|
|
||
| @Override | ||
| public double set(long index, double value) { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| @Override | ||
| public double increment(long index, double inc) { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| @Override | ||
| public void fill(long fromIndex, long toIndex, double value) { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| @Override | ||
| public void set(long index, byte[] buf, int offset, int len) { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| @Override | ||
| public long ramBytesUsed() { | ||
| /* | ||
| * If we return the size of the buffer that we've sliced | ||
| * we're likely to double count things. | ||
| */ | ||
| return SHALLOW_SIZE; | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| ref.decRef(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this is obvious, but I'm not used to looking at this part of the code - where's the corresponding
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You'll love this. It's in the ctor - in |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,7 +160,34 @@ public void testGetIntLE() { | |
| assertThat(comp.getIntLE(2), equalTo(0x02010012)); | ||
| assertThat(comp.getIntLE(3), equalTo(0x03020100)); | ||
| assertThat(comp.getIntLE(4), equalTo(0x04030201)); | ||
| Exception e = expectThrows(ArrayIndexOutOfBoundsException.class, () -> comp.getIntLE(5)); | ||
| assertThat(e.getMessage(), equalTo("Index 4 out of bounds for length 4")); | ||
| // The jvm can optimize throwing ArrayIndexOutOfBoundsException by reusing the same exception, | ||
| // but these reused exceptions have no message or stack trace. This sometimes happens when running this test case. | ||
| // We can assert the exception message if -XX:-OmitStackTraceInFastThrow is set in gradle test task. | ||
| expectThrows(ArrayIndexOutOfBoundsException.class, () -> comp.getIntLE(5)); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The additional the This reproduces when running: And also failed in PR CI. Running with We either need to ensure
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it is, nah. For what it's worth, painless does set this because it really does want to assert messages. And we set it in production because it can make debugging some issues impossible. |
||
| } | ||
|
|
||
| public void testGetDoubleLE() { | ||
| // first double = 1.2, second double = 1.4, third double = 1.6 | ||
| // tag::noformat | ||
| byte[] data = new byte[] { | ||
| 0x33, 0x33, 0x33, 0x33, 0x33, 0x33, -0xD, 0x3F, | ||
| 0x66, 0x66, 0x66, 0x66, 0x66, 0x66, -0xA, 0x3F, | ||
| -0x66, -0x67, -0x67, -0x67, -0x67, -0x67, -0x7, 0x3F}; | ||
| // end::noformat | ||
|
|
||
| List<BytesReference> refs = new ArrayList<>(); | ||
| int bytesPerChunk = randomFrom(4, 16); | ||
| for (int offset = 0; offset < data.length; offset += bytesPerChunk) { | ||
| int length = Math.min(bytesPerChunk, data.length - offset); | ||
| refs.add(new BytesArray(data, offset, length)); | ||
| } | ||
| BytesReference comp = CompositeBytesReference.of(refs.toArray(BytesReference[]::new)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| assertThat(comp.getDoubleLE(0), equalTo(1.2)); | ||
| assertThat(comp.getDoubleLE(8), equalTo(1.4)); | ||
| assertThat(comp.getDoubleLE(16), equalTo(1.6)); | ||
| // The jvm can optimize throwing ArrayIndexOutOfBoundsException by reusing the same exception, | ||
| // but these reused exceptions have no message or stack trace. This sometimes happens when running this test case. | ||
| // We can assert the exception message if -XX:-OmitStackTraceInFastThrow is set in gradle test task. | ||
| expectThrows(IndexOutOfBoundsException.class, () -> comp.getDoubleLE(17)); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we want to do it in this PR or in a follow up, but we'll want this same bit twiddling for the
longversion, and might as well make this reusable for that case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I will add
getLongLEmethod to this class and the interface and a unit test for it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pushed: 94c75f3