[
https://issues.apache.org/jira/browse/HDDS-16183?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Chi-Hsuan Huang reassigned HDDS-16183:
--------------------------------------
Assignee: Chi-Hsuan Huang
> Avoid the per-group list copy in OMKeyRequest.sumBlockLengths
> -------------------------------------------------------------
>
> Key: HDDS-16183
> URL: https://issues.apache.org/jira/browse/HDDS-16183
> Project: Apache Ozone
> Issue Type: Improvement
> Components: OM
> Reporter: Chi-Hsuan Huang
> Assignee: Chi-Hsuan Huang
> Priority: Minor
>
> h3. Problem
> {{OMKeyRequest.sumBlockLengths}} \({{OMKeyRequest.java:887\-897}}\) totals
> the length of every block a key points to, but reaches those blocks through
> {{OmKeyLocationInfoGroup.getLocationList\(\)}}, which builds a flattened copy
> on every call:
> {code}
> public static long sumBlockLengths\(OmKeyInfo omKeyInfo\) {
> long bytesUsed = 0;
> for \(OmKeyLocationInfoGroup group: omKeyInfo.getKeyLocationVersions\(\)\) {
> for \(OmKeyLocationInfo locationInfo : group.getLocationList\(\)\) {
> bytesUsed \+= QuotaUtil.getReplicatedSize\(
> locationInfo.getLength\(\), omKeyInfo.getReplicationConfig\(\)\);
> }
> }
> return bytesUsed;
> }
> {code}
> {{getLocationList\(\)}} carries an explicit warning from its author
> \({{OmKeyLocationInfoGroup.java:103\-112}}\):
> {quote}
> Use this expensive method only when absolutely needed\! It creates a new list
> so it is not an O\(1\) operation. Use getLocationLists\(\) instead.
> {quote}
> It is implemented as
> {{locationVersionMap.values\(\).stream\(\).flatMap\(List::stream\).collect\(...\)}},
> so each call allocates a stream pipeline and a new {{ArrayList}} per version
> group. Summing lengths does not need a flattened list;
> {{getLocationLists\(\)}} returns {{locationVersionMap.values\(\)}} directly
> and an extra nested loop would visit exactly the same block objects.
> h3. Where it is called
> {{sumBlockLengths}} is used to compute released quota in
> {{OMKeyDeleteRequest.java:165}}, {{OMKeyDeleteRequestWithFSO.java:162}},
> {{OMKeysDeleteRequest.java:322}}, {{OmKeysDeleteRequestWithFSO.java:115,156}}
> and {{OMKeyCommitRequest.java:358}}, which are all per\-request.
> Two callers are not per\-request, and they are where this matters:
> {{KeyManagerImpl.java:868}}, inside the loop that builds the reclaimable key
> list for {{KeyDeletingService}}. That one runs per pending\-delete key on
> every service iteration, so the allocation is repeated across the whole scan.
> {{OMDirectoriesPurgeRequestWithFSO.java:191}}, inside the loop over
> {{path.getDeletedSubFilesList\(\)}}, itself nested in a loop over purge
> paths. That request is produced in batches by {{DirectoryDeletingService}},
> so one request can carry many sub\-files. An earlier revision of this
> description listed it as per\-request, which was wrong.
> h3. Measurement
> An isolated measurement of {{sumBlockLengths}} alone was taken with a
> throwaway JUnit harness \(not committed; no new dependency\), counting
> per\-call thread allocation with {{ThreadMXBean.getThreadAllocatedBytes}}
> after warmup. Two runs agreed closely. Blocks are 256 MB, replication
> RATIS/THREE.
> ||key shape||before||after||saved||
> |1 group x 1 block|129 ns / 469 B|14 ns / 0 B|469 B|
> |1 group x 4 blocks|115 ns / 448 B|14 ns / 0 B|448 B|
> |1 group x 32 blocks|252 ns / 784 B|54 ns / 0 B|784 B|
> |4 groups x 4 blocks|339 ns / 1792 B|64 ns / 0 B|1792 B|
> |16 groups x 8 blocks|1579 ns / 7168 B|366 ns / 0 B|7168 B|
> The nested\-loop form allocates nothing: {{getLocationLists\(\)}} returns the
> live collection and the iterators are scalarized by escape analysis. The
> current form pays roughly 470 B and 130 ns even for a single\-block key,
> which is the fixed cost of the stream pipeline.
> With {{ozone.key.deleting.limit.per.task}} at its default of 50000, and a
> typical key of one version group with four blocks, that is on the order of 22
> MB of garbage and 6 ms per {{KeyDeletingService}} scan.
> h3. What this does and does not show
> The allocation column is a direct measurement and is the solid part. The
> timing column is indicative only: this is a plain harness, not JMH, so it has
> no dead\-code or constant\-folding guards.
> More importantly, this measures the method in isolation, not the
> {{KeyDeletingService}} scan end to end. It does not establish what share of
> that scan the change is worth, and 6 ms against 50000 RocksDB reads and
> {{OmKeyInfo}} deserializations is very likely still noise. The original
> caveat therefore stands, and no speedup is being claimed: the case for the
> change is that it follows the accessor's documented contract and removes
> provably unnecessary allocation, not that it makes deletion faster.
> h3. Original verification note
> This is an allocation observation, not a measurement. Nothing here has been
> benchmarked, and the effect may be too small to observe next to the RocksDB
> reads and the {{OmKeyInfo}} deserialization that dominate the same loop.
> Profiling or a benchmark of the {{KeyDeletingService}} scan should come
> before or with the change, so that the issue is closed on evidence rather
> than on the shape of the code.
> h3. Notes
> Behavior is unchanged either way: both accessors expose the same block
> objects, so the sum is identical, and the existing delete and purge tests
> cover it.
> HDDS\-16195 covers a separate defect at the {{KeyManagerImpl}} caller: that
> loop walks the same blocks twice and recomputes the replicated size,
> independently of which accessor {{sumBlockLengths}} uses. The two changes do
> not conflict and can land in either order.
> Noticed while reviewing HDDS\-16127, which added a second caller of
> {{sumBlockLengths}} in the quota repair recount. That caller only runs for
> keys retaining more than one version, so it is not the motivation here.
> Pinned source commit 4766aa8609. Analysis assisted by AI tooling \(Claude
> Code, Opus 5\).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
