HDDS-14169. Reduce copying of maps in OmKeyArgs

[adoroszlai]

What changes were proposed in this pull request?

Reduce copying of maps in OmKeyArgs:

• inherit metadata map from WithMetadata and its builder
• use MapBuilder for tags in OmKeyArgs.Builder

Since metadata is now immutable, change BlockOutputStreamEntryPool and BlockDataStreamOutputEntryPool to keep an OmKeyArgs.Builder instead of OmKeyArgs.

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

How was this patch tested?

CI:
https://github.com/adoroszlai/ozone/actions/runs/20959833882

[adoroszlai]

CC @Russole

[Russole]

@adoroszlai Thanks for the ping and for picking this up.
I was blocked by the unit test failures longer than expected and appreciate you stepping in to resolve it.

[siddhantsangwan]

Do we need a null check on metadatamap before using the getter? metadatamap might be null. Alternatively use the underlying map that's being added to.

[adoroszlai]

Thanks, added a null check.

[siddhantsangwan]

Just trying to complete my understanding - how does this change reduce copying of maps?

[adoroszlai]

Just trying to complete my understanding - how does this change reduce copying of maps?

When OmKeyArgs is converted toBuilder() for modification, its properties are all set on the Builder. Currently, metadata and tags are stored in new HashMap, so that changes made to the Builder do not affect the original OmKeyArgs. However, this copying of the maps happens even if metadata/tags are not changed, e.g. here:

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ResolvedBucket.java

Lines 106 to 111 in fb097af

	public OmKeyArgs update(OmKeyArgs args) {
	return isLink()
	? args.toBuilder()
	.setVolumeName(realVolume())
	.setBucketName(realBucket())
	.build()

MapBuilder stores the original immutable map, and creates mutable copy only if and when the map is being modified (new items added, etc.).

[siddhantsangwan]

@adoroszlai your explanation sounds good. I have two remaining points to discuss:

1. Since the maps are now immutable, we need to be sure there isn't existing code that's getting and trying to change their contents. There will be runtime exceptions if there is.
2. Currently, BlockOutputStreamEntryPool (and its keyArgs) is constructed during the construction of KeyOutputStream and then the keyArgs object is used during allocateNewBlock etc. With this change, every time allocateNewBlock is called, a new OmKeyArgs object will be constructed. Can that increase GC churn significantly? I suppose this only matters for really big keys with multiple blocks. However I'm not that familiar with client side code.

[adoroszlai]

1. Since the maps are now immutable, we need to be sure there isn't existing code that's getting and trying to change their contents. There will be runtime exceptions if there is.

Hopefully existing tests cover this.

2. Currently, BlockOutputStreamEntryPool (and its keyArgs) is constructed during the construction of KeyOutputStream and then the keyArgs object is used during allocateNewBlock etc. With this change, every time allocateNewBlock is called, a new OmKeyArgs object will be constructed. Can that increase GC churn significantly? I suppose this only matters for really big keys with multiple blocks.

For multi-block keys allocateNewBlock happens for each 256MB (default). A new instance of OmKeyArgs is nothing compared to that, as it mostly just holds references to the same data structures as the builder.

[siddhantsangwan]

LGTM, thanks @adoroszlai for working on this.

[adoroszlai]

Thanks @siddhantsangwan for the review, @Russole for starting work on this.