Improve serialization of translog operations #134243
Conversation
Tim-Brooks
added
>non-issue
:Distributed Indexing/CRUD
… recycler_byte_improvements
… recycler_byte_improvements
… recycler_byte_improvements
… recycler_byte_improvements
server/src/main/java/org/elasticsearch/index/translog/TranslogHeaderWriter.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/translog/TranslogHeaderWriter.java
Outdated
Show resolved
Hide resolved
|
|
||
| int variableLengthSize = (int) (buffer.position() - variableLengthStart); | ||
| int sizeOfOperation = FIXED_INDEX_HEADER_SIZE - Integer.BYTES + variableLengthSize + sourceLength + Integer.BYTES; | ||
| ByteUtils.writeIntBE(sizeOfOperation, bytes, off); |
There was a problem hiding this comment.
Should we assert that it is identical to a slow header, perhaps rarely?? You probably have tests for it though.
There was a problem hiding this comment.
I do have tests in TranslogHeaderWriterTests which runs 30 checks each run randomizing to one page and cross-page.
| buffer.writeString(indexOperation.id()); | ||
|
|
||
| int sourceLength = indexOperation.source().length(); | ||
| buffer.writeVInt(indexOperation.source().length()); |
There was a problem hiding this comment.
I need to check but am guessing we may not need this. I am ok to keep it though, but think a comment on why we have it is appropriate.
There was a problem hiding this comment.
Hmm. I'm not sure what you mean. We serialize it here to calculate the checksum. I actually don't love using a vInt here. It's more historical from how we serialize BytesReferences in stream outputs.
Fixed width values would be nicer but we already have variable length values for id and routing so I did not feel like it was worth it to change this.
| out.writeLong(seqNo); | ||
| out.writeLong(primaryTerm); | ||
| } else { | ||
| out.writeLong(version); |
There was a problem hiding this comment.
Can this and writeSlowIndexHeader share code?
There was a problem hiding this comment.
I combined these.
server/src/main/java/org/elasticsearch/index/translog/TranslogWriter.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/translog/TranslogTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/translog/TranslogTests.java
Outdated
Show resolved
Hide resolved
|
Buildkite benchmark this with tsdb-metricsgen-270m please |
|
Buildkite benchmark this with http_logs-1n-1g please |
|
When I ran the http logs it deleted the tsdb metrics gen comment. I also ran that: ⏳ Build in-progress Buildkite Build Result: https://esbench-metrics.kb.us-east-2.aws.elastic-cloud.com:9243/app/r/s/lwXvc |
💚 Build Succeeded
This build ran two http_logs-1n-1g benchmarks to evaluate performance impact of this PR. History |
|
Buildkite benchmark this with nyc_taxis-1n-8g please |
|
NYC Taxis 💚 Build Succeeded
This build ran two nyc_taxis-1n-8g benchmarks to evaluate performance impact of this PR. History |
|
TSDB Metricsgen p50 
NYC Taxi 
Http Logs are nearly identical between contender and main. CPUs pegged. I'll run the standard logs overnight and as long as that looks fine I'll commit tomorrow morning for a full nightly run. |
|
Buildkite benchmark this with elastic-logs please |
💚 Build Succeeded
This build ran two elastic-logs benchmarks to evaluate performance impact of this PR. History |
|
Logs |
Currently translog operations are serialized in a way that is relatively
inefficient. This commit improves performance by re-ordering the
operations to allow most of the header to be serialized without bounds
checks. Additionally, instead of incrementally calculating the checksum
we do a single pass at the end. Finally, we no longer copy the source
twice. Instead serialize it directly into the translog writer.