Skip to content

HDDS-9940. Use MappedByteBuffer in ChunkManagerDummyImpl#9649

Merged
adoroszlai merged 5 commits intoapache:masterfrom
Russole:HDDS-9940
Feb 11, 2026
Merged

HDDS-9940. Use MappedByteBuffer in ChunkManagerDummyImpl#9649
adoroszlai merged 5 commits intoapache:masterfrom
Russole:HDDS-9940

Conversation

@Russole
Copy link
Contributor

@Russole Russole commented Jan 19, 2026

What changes were proposed in this pull request?

  • Updated ChunkManagerDummyImpl to avoid allocating heap buffers on reads (previously ByteBuffer.allocate(len)), by creating a small backing file (default 1MB) and reading data from a reusable MappedByteBuffer.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9940

How was this patch tested?

All CI checks passed.
https://github.com/Russole/ozone/actions/runs/21101732901

@adoroszlai adoroszlai requested a review from szetszwo January 19, 2026 17:39
Copy link
Contributor

@Gargi-jais11 Gargi-jais11 left a comment

Choose a reason for hiding this comment

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

Thanks @Russole for the patch.
Left some review comments below.

Comment on lines 76 to 78
if (ch.size() < newSize) {
ch.truncate(newSize);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, the code uses ch.truncate(newSize) to expand the file when ch.size() < newSize, but truncate() only reduces file size - it cannot expand files.

According to Java API:> "If the given size is greater than or equal to the file's current size then the file is not modified."
When the file is smaller than newSize, truncate() does nothing, leaving the file unchanged. This causes silent failures when read requests exceed the current mapped buffer size.

  1. Scenario 1: File is Smaller, Need Smaller Size (< 1MB)

Example:

  • Current file: 500 KB
  • Request: Read 256 KB
  • newSize = max(1MB, 256KB) = 1MB (because of DEFAULT_MAP_SIZE)
Current: ch.size() = 500 KB
Need:    newSize = 1 MB
Check:   Is 500 KB < 1 MB? → YES 
Action:  ch.truncate(1 MB)
Result:  ❌ truncate() does NOTHING (can't expand)
         File stays 500 KB, but we need 1 MB!

Problem: File doesn't expand to 1 MB, mapping fails or maps wrong size.

  1. Scenario 2: File is Larger, Need Smaller Size (< 1MB)

Example:

  • Current file: 2 MB
  • Request: Read 256 KB
  • newSize = max(1MB, 256KB) = 1MB
Current: ch.size() = 2 MB
Need:    newSize = 1 MB
Check:   Is 2 MB < 1 MB? → NO ❌
Action:  Skip truncate (condition false)
Result:  File is already big enough (2 MB > 1 MB)
         But we're mapping only 1 MB, so it works!

Status: Works (but wastes space - file is 2 MB but we only use 1 MB)

We need to take care of these scenarios.

if (ch.size() < newSize) {
  // LOGIC:  Need to EXPAND file 
} else if (ch.size() > newSize) {
  // Need to SHRINK file
  ch.truncate(newSize);  // This works for shrinking!
}

@Gargi-jais11
Copy link
Contributor

Add some test coverage in TestChunkManagerDummyImpl for below scenarios:

  • Test when read size is exactly at current mapped size boundary
  • Test when read size larger than current mapped size (triggers resize)
  • Test Concurrent reads from multiple threads

@Russole
Copy link
Contributor Author

Russole commented Jan 24, 2026

Thanks @Gargi-jais11 for the detailed review. I’ll make the necessary updates based on the comments.

@jojochuang
Copy link
Contributor

realistically ChunkManagerDummyImpl is for perf test only not production code. IMO doesn't require super rigorous test corner case coverage. (Though it's nothing if AI generates the test code)

@Gargi-jais11
Copy link
Contributor

realistically ChunkManagerDummyImpl is for perf test only not production code. IMO doesn't require super rigorous test corner case coverage. (Though it's nothing if AI generates the test code)

Okay if that's the case then we can skip testing this. @Russole Just fix the above review comment.

@Russole
Copy link
Contributor Author

Russole commented Feb 4, 2026

Thanks @Gargi-jais11 for the clarification. I’ll focus on fixing the review comment mentioned above and update the patch accordingly.

@Russole Russole requested a review from Gargi-jais11 February 7, 2026 02:45
@Russole
Copy link
Contributor Author

Russole commented Feb 10, 2026

Hi @szetszwo, just sharing this PR for visibility as it follows up on the related JIRA. Thanks!

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

@Russole , thanks for working on this!

We could

  • create the tmp file in the begining,
  • initialize the MappedByteBuffer and make it readonly,
  • duplicate the buffer and set limit for each readChunk(..) call.

Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13080734/9649_review.patch

ChunkInfo info, DispatcherContext dispatcherContext)
throws StorageContainerException {

limitReadSize(info.getLen());
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keeping using limitReadSize(..). It limits the len <= OZONE_SCM_CHUNK_MAX_SIZE.

Then, we may create the backing file using the size OZONE_SCM_CHUNK_MAX_SIZE.

StandardOpenOption.READ, StandardOpenOption.WRITE)) {
long currentSize = ch.size();
if (currentSize < newSize) {
ch.position(newSize - 1L);
Copy link
Contributor

Choose a reason for hiding this comment

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

... A later attempt to write bytes at such a position will cause the file to be grown to accommodate the new bytes; the values of any bytes between the previous end-of-file and the newly-written bytes are unspecified.

From the FileChannel javadoc above, we should write zeros for the zero-filled buffer..

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

+1 the change looks good.

@adoroszlai adoroszlai merged commit 86f9fd6 into apache:master Feb 11, 2026
44 checks passed
@adoroszlai
Copy link
Contributor

Thanks @Russole for the patch, @Gargi-jais11, @szetszwo for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants