deepthi912 commented on code in PR #19596:
URL: https://github.com/apache/pinot/pull/19596#discussion_r4163216624
##########
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:
**Benchmark: partial-upsert ingestion, before vs after**
Single consuming thread, so these are **per-core** numbers. 16 real
immutable segments, 16K primary keys, PARTIAL upsert mode. Same benchmark
source compiled against both commits.
**Ingestion** — one op = one full record through
`MutableSegmentImpl.index()`: primary key extraction, partial-upsert merge,
metadata update, and indexing into dictionaries and forward indexes. This is
what the consuming thread does per stream message.
| | `a3af29e9` (before) | `c0495fe0` (after) |
|---|---:|---:|
| throughput | ~235,000 records/sec/core | ~235,000 records/sec/core |
| per record | ~3,835 ns | ~3,835 ns |
**No measurable difference** — run-to-run variance exceeded any gap between
the builds.
**Merge only** — one op = one `updateRecord()` call, roughly 4% of a
record's cost. This is where the guard sits, so it is the only place the effect
is visible:
| | before | after |
|---|---:|---:|
| throughput | 6,082,667 ops/sec | 5,922,819 ops/sec |
| per call | 164.4 ns | 168.8 ns |
| added | | +4.4 ns |
The 4.4 ns is two CAS plus two volatile reads, executed once per record —
about 0.1% of a 3,835 ns record.
**Contention: zero.** `destroy()` was never called during these runs. In
production it is the only writer of this lock, runs 2–3 times per day per
partition, and holds it for a single boolean store, so effectively every read
is uncontended.
**Environment:** Apple M2 Max (8P + 4E, no CPU pinning available on macOS),
32 GB, Temurin JDK 25.0.3, `-Xms2G -Xmx8G`, load average ~17 during the runs.
**Caveat:** three repeat runs did not reproduce the +4.4 ns — two showed
"after" nominally faster, which is not physically possible. Within-build
scatter was ~21%. Treat the merge figure as indicative; the defensible claim is
the ingestion row, **no regression detectable**.
--
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]
