Skip to content

[GEODE-10646] Reduce up-front buffer allocation in the Lucene file output stream - #8075

Open
JinwooHwang wants to merge 2 commits into
apache:developfrom
JinwooHwang:feature/GEODE-10646
Open

[GEODE-10646] Reduce up-front buffer allocation in the Lucene file output stream#8075
JinwooHwang wants to merge 2 commits into
apache:developfrom
JinwooHwang:feature/GEODE-10646

Conversation

@JinwooHwang

Copy link
Copy Markdown
Contributor

Reduce up-front buffer allocation in the Lucene file output stream

For all changes, please confirm:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
  • Has your PR been rebased against the latest commit within the target branch (typically develop)?
  • Is your initial contribution a single, squashed commit?
  • Does gradlew build run cleanly?
  • Have you written or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

@sboorlagadda sboorlagadda left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The chunking behavior looks preserved, and the existing large-write test plus
the new round-trip test provide useful coverage. Before approving the memory
optimization, could you add a test that detects reintroducing the eager 1 MiB
allocation and share a base/head allocation or GC comparison for small and
larger outputs? A fresh stream reaching 1 MiB now allocates 2040 KiB of buffer
arrays and recopies 1016 KiB during growth, although it saves substantially for
small files. If this is intended to address the integration-test OOM, please
also link evidence connecting that failure to these buffers; the existing
ticket discusses ClassGraph and ByteBuffersDirectory, a separate storage path.

@JinwooHwang

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @sboorlagadda.

Added FileOutputStreamJUnitTest.testSmallFileAllocatesLessThanChunkSize, which measures thread allocation while writing a 100-byte file. It fails against develop's FileOutputStream (1,055,576 bytes allocated) and passes on this branch.

Base/head allocation per stream in bytes (JDK 17, ThreadMXBean, best of 7, 1000-byte writes, open through close):

Written     develop      GEODE-10646
1,000 B     1,055,896       15,480
64 KiB      1,120,432      194,920
512 KiB     1,579,184    1,571,392
1 MiB       2,103,472    3,144,328
5 MiB       6,322,864    7,363,784

This is consistent with your numbers: outputs reaching 1 MiB allocate about 1 MiB more per stream, from buffer growth.

This PR isn't intended to address the integration-test OOM.

To verify the effect with many streams open at once, I opened 452 streams on a map-backed FileSystem and wrote 2 KiB to each, using a 768 MiB heap. On develop each stream reserves a full 1 MiB buffer at open, and the 452 streams did not fit in the heap. On this branch the same streams retained about 6 MB.

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.

2 participants