Skip to content

Manually Manage Direct Write Buffer in Frozen Cache Writes #70399

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

Conversation

original-brownbear
Copy link
Contributor

@original-brownbear original-brownbear commented Mar 15, 2021

Manually managing the buffer has a number of upsides for us and no direct downsides as far as I can see:

  • We limit the number of syscalls for large writes by a factor of up to 8 (in practice it's probably mostly 8 when writing larger amounts of data)
    • which also means up to 8x less bookkeeping for the fs to do things like coalescing outstanding writes and such
  • Since we limit the write thread count to 28 the amount of direct memory used is limited
  • We eliminate the copying from heap -> off-heap under the lock in the channel's positional write
  • We eliminate allocating ByteBuffer instances in a hot loop when writing large regions

Manually managing the buffer has a number of upsides for us and no direct downsides
as far as I can see.
* We limit the number of syscalls for large writes by a factor of up to `8` (in pratice it's probably mostly `8` when writing larger amounts of data)
* Since we limit the write thread count to 28 the amount of direct memory used is limited
* We eliminate the copying from heap -> off-heap under the lock in the channel's positional write
* We eliminate allocating `ByteBuffer` instances in a hot loop when writing large regions
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Mar 15, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

return fc.write(byteBuffer, start);
byteBuffer.flip();
int written = fc.write(byteBuffer, start);
byteBuffer.clear();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we clear buffer if not everything was written? Or would we have to assert that buffer.remaining() is 0 here and that written == old(buffer.remaining())?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just assert byteBuffer.hasRemaining() == false; is sufficient. We do assert that the sum of all writes matches up in the callers anyway and we don't have to add another variable just for the assertion here. I pushed 34b179e for that.

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@original-brownbear original-brownbear merged commit 6c08574 into elastic:master Mar 18, 2021
@original-brownbear original-brownbear deleted the aggregate-frozen-file-writes branch March 18, 2021 13:11
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Mar 18, 2021
…0399)

Manually managing the buffer has a number of upsides for us and no direct downsides
as far as I can see.
* We limit the number of syscalls for large writes by a factor of up to `8` (in practice it's probably mostly `8` when writing larger amounts of data)
* Since we limit the write thread count to 28 the amount of direct memory used is limited
* We eliminate the copying from heap -> off-heap under the lock in the channel's positional write
* We eliminate allocating `ByteBuffer` instances in a hot loop when writing large regions
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Mar 18, 2021
…0399)

Manually managing the buffer has a number of upsides for us and no direct downsides
as far as I can see.
* We limit the number of syscalls for large writes by a factor of up to `8` (in practice it's probably mostly `8` when writing larger amounts of data)
* Since we limit the write thread count to 28 the amount of direct memory used is limited
* We eliminate the copying from heap -> off-heap under the lock in the channel's positional write
* We eliminate allocating `ByteBuffer` instances in a hot loop when writing large regions
original-brownbear added a commit that referenced this pull request Mar 18, 2021
…70558)

Manually managing the buffer has a number of upsides for us and no direct downsides
as far as I can see.
* We limit the number of syscalls for large writes by a factor of up to `8` (in practice it's probably mostly `8` when writing larger amounts of data)
* Since we limit the write thread count to 28 the amount of direct memory used is limited
* We eliminate the copying from heap -> off-heap under the lock in the channel's positional write
* We eliminate allocating `ByteBuffer` instances in a hot loop when writing large regions
original-brownbear added a commit that referenced this pull request Mar 18, 2021
…70559)

Manually managing the buffer has a number of upsides for us and no direct downsides
as far as I can see.
* We limit the number of syscalls for large writes by a factor of up to `8` (in practice it's probably mostly `8` when writing larger amounts of data)
* Since we limit the write thread count to 28 the amount of direct memory used is limited
* We eliminate the copying from heap -> off-heap under the lock in the channel's positional write
* We eliminate allocating `ByteBuffer` instances in a hot loop when writing large regions
@original-brownbear original-brownbear restored the aggregate-frozen-file-writes branch April 18, 2023 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >non-issue Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.12.1 v7.13.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants