[
https://issues.apache.org/jira/browse/TEZ-4745?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18107350#comment-18107350
]
Raghav Aggarwal commented on TEZ-4745:
--------------------------------------
Can I pick this up?
> Optimize some write paths in UnorderedPartitionedKVWriter
> ---------------------------------------------------------
>
> Key: TEZ-4745
> URL: https://issues.apache.org/jira/browse/TEZ-4745
> Project: Apache Tez
> Issue Type: Improvement
> Reporter: László Bodor
> Priority: Major
>
> UnorderedPartitionedKVWriter.write() is the hot path invoked once per KV pair
> emitted by every unordered partitioned output in Tez (used by all shuffle
> producers whose edge does not require a sort). writePartition() is called for
> every record when a partition is drained during a spill. Both methods spend a
> large fraction of their time on bookkeeping around the on-heap meta-buffer,
> and profiling shows a few mechanical inefficiencies that add up per record:
> https://github.com/apache/tez/blob/dd8137f4c13f71f55b975445d02140686b931664/tez-runtime-library/src/main/java/org/apache/tez/runtime/library/common/writers/UnorderedPartitionedKVWriter.java#L409-L479
> Repeated field access through {{currentBuffer}} / {{wrappedBuffer}}. Both
> methods dereference {{currentBuffer.nextPosition}},
> {{currentBuffer.availableSize}}, {{currentBuffer.metaBuffer}},
> {{currentBuffer.buffer}}, etc. many times per invocation.
> The JVM's C2 JIT is normally happy to load a field once and reuse it — that's
> just loop-invariant code motion. But it can only do that if it can prove
> nothing between the two reads could have changed the field. currentBuffer is
> a mutable field on an object reachable from other threads (spill callbacks
> run concurrently), and every method call in the loop body —
> keySerializer.serialize(...), valSerializer.serialize(...), setupNextBuffer()
> — is opaque to the JIT unless it inlines all the way through. If the JIT
> can't prove those calls don't reassign currentBuffer, it has to re-read the
> field after each one. Assigning currentBuffer to a local variable makes the
> reuse explicit — the JIT no longer needs to prove anything.
> Integer divide / modulo on a compile-time constant. {{INT_SIZE}} is {{4}},
> yet the code uses {{position % INT_SIZE}} and {{metaStart / INT_SIZE}}. javac
> does not fold {{% / /}} on {{int}} to shifts at the bytecode level, so an
> {{idiv}} instruction is emitted; the JIT sometimes lowers this to a shift,
> but not reliably (especially in interpreted / C1 tiers, and across VM
> versions). Replacing with {{position & (INT_SIZE - 1)}} (i.e. {{& 3}}) and
> {{metaStart >>> 2}} makes it a guaranteed single-cycle op.
> Redundant subtractions. The metadata-update block re-derives {{nextPosition -
> (metaStart + META_SIZE)}} in three separate places (key length, record bytes,
> and the per-partition size update). Compute it once as a local and reuse.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
