[
https://issues.apache.org/jira/browse/HDDS-16183?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Chi-Hsuan Huang updated HDDS-16183:
-----------------------------------
Description:
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\).
was:
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. Verification needed first
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\).
> 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
> 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]
