deepthi912 commented on code in PR #19596:
URL: https://github.com/apache/pinot/pull/19596#discussion_r4127328488
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -426,15 +434,15 @@ protected GenericRow doUpdateRecord(GenericRow record,
RecordInfo recordInfo) {
if (!recordInfo.isDeleteRecord()
&&
recordInfo.getComparisonValue().compareTo(recordLocation.getComparisonValue())
>= 0) {
IndexSegment currentSegment = recordLocation.getSegment();
- ThreadSafeMutableRoaringBitmap currentQueryableDocIds =
currentSegment.getQueryableDocIds();
int currentDocId = recordLocation.getDocId();
- if (currentQueryableDocIds == null ||
currentQueryableDocIds.contains(currentDocId)) {
+ // Read lock: currentSegment cannot be destroyed while LazyRow
reads its columns. A consuming segment needs
+ // no lock: it is destroyed only after
replaceSegment()/removeSegment() has moved or dropped every location
+ // pointing at it, and those run under the same per-key compute as
this read.
+ if (tryAcquireSegmentReadLock(currentSegment)) {
Review Comment:
Good call-out. Early numbers, with the caveat that the harness needs work
before I'd trust the CPU side.
**Structural.** One consumer thread per partition and the lock is per
segment, so the read lock is uncontended in steady state. The real contention
is with `destroy()` — it's a *non-fair* RRWL, so a queued writer parks new
readers while the consumer sits inside a `computeIfPresent` bin lock. My
harness doesn't produce that case yet.
**CPU.** JMH over a real 200K-row segment, previous-row read with vs.
without the guard. Only the 1-column point reproduced across runs: **+1.8–2.3
ns (~1.3%)**. At 4 and 8 columns the delta fell below the noise floor (4
columns came out *negative*), so I'm not quoting those. Guard in isolation: ~16
ns.
Two known fidelity gaps, pulling opposite ways: it calls
`ImmutableSegmentImpl#tryAcquireReadLock()` directly instead of
`tryAcquireSegmentReadLock(IndexSegment)`, missing the bimorphic dispatch
(under-reports); and the denominator omits `PartialUpsertHandler.merge(...)`,
the `queryableDocIds` check, the CHM bin lock and null handling (over-reports).
**Memory.** One RRWL per `ImmutableSegmentImpl`, measured at **121 bytes**
(2M allocations, compressed oops). ~1.2 MB at 10K segments — under 0.01% of
segment footprint. `_destroyed` fits existing padding. RRWL's
`ThreadLocalHoldCounter` is bounded by segments-per-partition, since only the
consumer and segment-replace threads acquire; query threads never do.
**Next.** Measure guard and merge as separate terms and report the ratio
instead of subtracting two large noisy numbers, route through the production
helper, and add a contended arm with a background `destroy()`. Will post
numbers when they're defensible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
