Skip to content
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

FileChannelJournalSegmentReader throws half buffers away #4248

Closed
Zelldon opened this issue Apr 7, 2020 · 0 comments · Fixed by #4372
Closed

FileChannelJournalSegmentReader throws half buffers away #4248

Zelldon opened this issue Apr 7, 2020 · 0 comments · Fixed by #4372
Labels
area/performance Marks an issue as performance related kind/bug Categorizes an issue or PR as a bug kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. scope/broker Marks an issue or PR to appear in the broker section of the changelog
Milestone

Comments

@Zelldon
Copy link
Member

Zelldon commented Apr 7, 2020

Description

During our investigation of the engine latency we found out that the atomix journal is a big hotspot performance wise. We had a deeper look at the code and found the readNext method.

We have to say before that per Zeebe default the maxEntrySize is 4 MB. The reader will allocate a buffer which is 8MB big. Our assumption is that it wants to do a read a head, but actually it doesn't because it clears the buffer positions etc.

  /** Reads the next entry in the segment. */
  @SuppressWarnings("unchecked")
  private void readNext() {
    // Compute the index of the next entry in the segment.
    final long index = getNextIndex();

    try {
      // Read more bytes from the segment if necessary.
      if (memory.remaining() < maxEntrySize) {
        final long position = channel.position() + memory.position();
        channel.position(position);
        memory.clear(); // <=== this resets to position 0, limit to capacity
        channel.read(memory);  // <=== we read now again 8 MB
        channel.position(position);
        memory.flip();
      }

      // Mark the buffer so it can be reset if necessary.
      memory.mark();

      try {
        // Read the length of the entry.
        final int length = memory.getInt();

        // If the buffer length is zero then return.
        if (length <= 0 || length > maxEntrySize) {
          memory.reset().limit(memory.position());
          nextEntry = null;
          return;
        }

        // Read the checksum of the entry.
        final long checksum = memory.getInt() & 0xFFFFFFFFL;

        // Compute the checksum for the entry bytes.
        final Checksum crc32 = new CRC32();
        crc32.update(memory.array(), memory.position(), length);

        // If the stored checksum equals the computed checksum, return the entry.
        if (checksum == crc32.getValue()) {
          final int limit = memory.limit();
          memory.limit(memory.position() + length);
          final E entry = namespace.deserialize(memory);
          memory.limit(limit);
          nextEntry = new Indexed<>(index, entry, length);
        } else {
          memory.reset().limit(memory.position());
          nextEntry = null;
        }
      } catch (final BufferUnderflowException e) {
        memory.reset().limit(memory.position());
        nextEntry = null;
      }
    } catch (final IOException e) {
      throw new StorageException(e);
    }
  }

This means we will read 8 MB process from these ~4 MBuntil our remaining is less then maxEntrySize, so after we reached half of the buffer. Then we will read again 8MB where the other remaining is thrown away and will end at the beginning of the buffer, because it is re-read.

This problem is more an issue when the maxEntrySize and realEntrySize is very different, which is our case. We using 4MB to support big deployments but actually most of our records are less then 1 kb. If the sizes would be nearer then we would probably also read most of the buffer, but this is not our use case so we should fix this!

@Zelldon Zelldon added kind/bug Categorizes an issue or PR as a bug kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. scope/broker Marks an issue or PR to appear in the broker section of the changelog area/performance Marks an issue as performance related labels Apr 7, 2020
@Zelldon Zelldon changed the title FileChannelJournalSegmentReader seems to work in an unexpected way FileChannelJournalSegmentReader throws half buffers away Apr 7, 2020
@Zelldon Zelldon added this to the Maintenance milestone Apr 7, 2020
zeebe-bors bot added a commit that referenced this issue Apr 29, 2020
4372: fix(atomix): consume read buffer correctly r=Zelldon a=Zelldon

## Description

Fix issue where the read buffer was consumed only half and the rest was
 thrown away.

Refactor `FileChannelJournalSegmentReader#readNext` to improve readability and hopefully maintainability.
<!-- Please explain the changes you made here. -->

I run also a benchmark with these changes (#4358 was also part of the branch I run a benchmark with)

![metrics](https://user-images.githubusercontent.com/2758593/80199113-45202a00-8621-11ea-9595-e8c65130ec97.png)


## Related issues

<!-- Which issues are closed by this PR or are related -->

closes #4248 

#

Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
@zeebe-bors zeebe-bors bot closed this as completed in bf5d2fa May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance Marks an issue as performance related kind/bug Categorizes an issue or PR as a bug kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. scope/broker Marks an issue or PR to appear in the broker section of the changelog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants