Skip to content

HDDS-16257. Avoid the flatten-copy in OmMetadataManagerImpl.getExpiredOpenKeys' hsync branch - #11116

Open
shuan1026 wants to merge 2 commits into
apache:masterfrom
shuan1026:HDDS-16257
Open

HDDS-16257. Avoid the flatten-copy in OmMetadataManagerImpl.getExpiredOpenKeys' hsync branch#11116
shuan1026 wants to merge 2 commits into
apache:masterfrom
shuan1026:HDDS-16257

Conversation

@shuan1026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

OmMetadataManagerImpl.getExpiredOpenKeys's hsync-commit branch called OmKeyLocationInfoGroup.createLocationList() on the key's latest version group just to stream it once while building the commit KeyArgs protobuf, allocating a throwaway flattened List per matched hsync key.

This PR swaps the method reference for the zero-copy getLocationLists() accessor and inserts a flatMap(List::stream) to keep the rest of the chain (.map(getProtobuf).forEach(addKeyLocations)) unchanged.

Additionally, a throwaway (not committed) JUnit micro-benchmark measured per-KeyArgs-construction thread allocation for both implementations (JDK 21.0.12, 50,000-iteration warmup then 500,000 measured iterations per shape, output KeyArgs protobufs asserted equal, volatile sink to block JIT elimination):

Key Shape Before After Saved
1 version list x 1 block 352 ns / 1372 B 177 ns / 1154 B 218 B
1 version list x 4 blocks 367 ns / 2496 B 387 ns / 2296 B 200 B
1 version list x 32 blocks 2155 ns / 13264 B 2086 ns / 12728 B 536 B
4 version lists x 4 blocks 1144 ns / 7432 B 1117 ns / 7048 B 384 B
16 version lists x 8 blocks 8176 ns / 51280 B 8005 ns / 49088 B 2192 B

What is the link to the Apache JIRA

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

How was this patch tested?

  • TestOpenKeyCleanupService#testCommitExpiredHsyncKeys
  • TestOpenKeyCleanupService#testCleanupExpiredOpenKeys
  • TestOpenKeyCleanupService#testIgnoreExpiredRecoverhsyncKeys

@sarvekshayr sarvekshayr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @shuan1026 for working on this.

Minor nits not related to your changes.

@@ -1510,9 +1504,10 @@ public ExpiredOpenKeys getExpiredOpenKeys(Duration expireThreshold,
.setKeyName(openKeyInfo.getKeyName())
.setDataSize(info.getDataSize());
java.util.Optional.ofNullable(info.getLatestVersionLocations())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Add the import for Optional at the top.

Suggested change
java.util.Optional.ofNullable(info.getLatestVersionLocations())
Optional.ofNullable(info.getLatestVersionLocations())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here as well.

@sravani-revuri sravani-revuri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

agree with @sarvekshayr on the import. Otherwise LGTM.

@shuan1026

Copy link
Copy Markdown
Contributor Author

Done, thanks @sarvekshayr @sravani-revuri 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.

3 participants