From eda162a38b4a4df10064891b3bf57570a7ce0417 Mon Sep 17 00:00:00 2001 From: afrind Date: Wed, 15 Apr 2026 22:02:55 -0400 Subject: [PATCH] feat(ranking): bidirectional int64_t seq so past-active tracks rank above never-active MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When two tracks tie on a synthetic property value, a participant who was recently active and went quiet should rank above one that joined and never contributed. Early session join should not grant priority over an established contributor at the same level. Two counters manage arrivalSeq (int64_t): - nextSeqUp_ (non-negative, incrementing): stamped on registration and every value increase. Whoever rose to a given value first holds the lowest Up seq and wins ties — prior registration order is irrelevant. - nextSeqDown_ (negative, decrementing): stamped on every value decrease. Negative seqs always sort below non-negative ones, so any track that has decreased to a value outranks tracks that registered at or rose to that value. Rising again clears the negative history by stamping a fresh Up seq. Known trade-off: a track that decreased to a value beats any track that later rose to the same value even if the riser arrived more recently. Acceptable because the property is a synthetic application metric, so exact-value ties at active levels are rare in practice. Co-Authored-By: Claude Sonnet 4.6 --- docs/dev/track-filter-ranking.md | 11 ++++- src/relay/PropertyRanking.cpp | 11 +++-- src/relay/PropertyRanking.h | 12 ++++-- test/PropertyRankingBaseTest.cpp | 71 ++++++++++++++++++++++++++++++++ 4 files changed, 98 insertions(+), 7 deletions(-) diff --git a/docs/dev/track-filter-ranking.md b/docs/dev/track-filter-ranking.md index 157fba25..8d924d22 100644 --- a/docs/dev/track-filter-ranking.md +++ b/docs/dev/track-filter-ranking.md @@ -22,7 +22,16 @@ The split between `onValueChanged` and `onActivity` matters: value updates drive ## PropertyRanking — The Ranking Engine -`PropertyRanking` maintains a `std::map>` — a descending-sorted map of all registered tracks. `RankKey` is `(value, arrivalSeq)` where higher value wins and lower `arrivalSeq` breaks ties (earlier arrivals win). +`PropertyRanking` maintains a `std::map>` — a descending-sorted map of all registered tracks. `RankKey` is `(value, arrivalSeq)` where higher value wins. `arrivalSeq` (`int64_t`) breaks ties: lower value wins. + +Two counters manage `arrivalSeq`: + +- **Up-counter** (`nextSeqUp_`, non-negative, incrementing): assigned on registration and on every value *increase*. Whoever rose to a given value first holds the lowest Up seq and wins ties at that level — early session join does not grant priority over an established contributor. +- **Down-counter** (`nextSeqDown_`, negative, decrementing): assigned on every value *decrease*. Negative seqs always sort below non-negative ones, so any track that has decreased to a value permanently outranks tracks that registered at or rose to that value. + +The intended consequence: a participant who was recently active and has since gone quiet ranks above one that joined and never contributed. Among past-active tracks at the same value, the most recently decreased (most-negative seq) ranks highest. Rising again clears the negative history by stamping a fresh Up seq. + +A known side-effect of this two-tier structure: a track that decreased to a value will beat any track that later rose to that same value, even if the riser arrived more recently. This trade-off is acceptable because the property is expected to be a synthetic application metric (not a raw signal), so exact-value ties at active levels are rare in practice. A parallel `F14FastMap` provides O(1) lookup by name and caches each track's integer rank. diff --git a/src/relay/PropertyRanking.cpp b/src/relay/PropertyRanking.cpp index b487e297..b6394a02 100644 --- a/src/relay/PropertyRanking.cpp +++ b/src/relay/PropertyRanking.cpp @@ -37,7 +37,7 @@ void PropertyRanking::registerTrack( } uint64_t value = initialPropertyValue.value_or(0); - RankKey key{value, nextSeq_++}; + RankKey key{value, nextSeqUp_++}; publisherTrackCount_[publisher.get()]++; @@ -94,8 +94,13 @@ void PropertyRanking::updateSortValue(const moxygen::FullTrackName& ftn, uint64_ return; } - // Construct new key after the early-exit check - RankKey newKey{value, oldKey.arrivalSeq}; + // Construct new key after the early-exit check. + // Rise: fresh nextSeqUp_++ so whoever reached this level first keeps priority + // (early registration no longer displaces an established talker). + // Fall: nextSeqDown_-- (negative) so past-active tracks rank above + // never-active or just-rising tracks (non-negative) at the same value. + int64_t newSeq = (value > oldKey.value) ? nextSeqUp_++ : nextSeqDown_--; + RankKey newKey{value, newSeq}; // Capture old rank before modifying the map (oldKey disappears after erase) uint64_t oldRank = getCachedRank(ftn); diff --git a/src/relay/PropertyRanking.h b/src/relay/PropertyRanking.h index a611f9c4..c8070907 100644 --- a/src/relay/PropertyRanking.h +++ b/src/relay/PropertyRanking.h @@ -26,8 +26,13 @@ namespace openmoq::moqx { * Uses std::greater<> comparator for descending order (highest value first). */ struct RankKey { - uint64_t value; // Property value (higher = better rank) - uint64_t arrivalSeq; // Tie-breaker (lower = earlier = wins) + uint64_t value; // Property value (higher = better rank) + int64_t arrivalSeq; // Tie-breaker: lower = wins. + // Non-negative (0,1,2…): assigned on registration or value increase. + // Whoever rose to this value first holds the lowest Up seq and wins. + // Negative (-1,-2,-3…): assigned on value decrease. + // Past-active tracks (negative) always outrank never-active or + // just-rising tracks (non-negative) at the same value. bool operator<(const RankKey& other) const { if (value != other.value) { @@ -370,7 +375,8 @@ class PropertyRanking { // map never holds zombie entries. folly::F14FastMap publisherTrackCount_; - uint64_t nextSeq_{0}; + int64_t nextSeqUp_{0}; + int64_t nextSeqDown_{-1}; uint64_t selectionThreshold_{0}; std::vector sortedThresholds_; diff --git a/test/PropertyRankingBaseTest.cpp b/test/PropertyRankingBaseTest.cpp index 8d94a782..5af8c9af 100644 --- a/test/PropertyRankingBaseTest.cpp +++ b/test/PropertyRankingBaseTest.cpp @@ -701,4 +701,75 @@ TEST_F(PropertyRankingBaseTest, SweepIdle_ActiveTracksNotEvicted) { EXPECT_EQ(group->trackStates.at(ftn("b")), TrackState::Selected); } +// --------------------------------------------------------------------------- +// Bidirectional seq counter: past-active tracks rank above never-active +// --------------------------------------------------------------------------- + +// A joins first and never talks. B joins later, talks, then goes silent. +// Both end up at value=0. B should rank above A because its seq was stamped +// with the down counter (negative) when its value decreased. +TEST_F(PropertyRankingBaseTest, SeqDown_SilentAfterTalking_WinsOverNeverTalked) { + RankingHarness h; + auto sub = makeSession(); + + h.ranking().registerTrack(ftn("a"), 0, defaultPublisher_); // seqUp=0 + h.ranking().registerTrack(ftn("b"), 0, defaultPublisher_); // seqUp=1 + h.ranking().updateSortValue(ftn("b"), 100); // B talks; seqUp=2 + h.ranking().updateSortValue(ftn("b"), 0); // B goes silent; seqDown=-1 + + // At value=0: B has seqDown=-1, A has seqUp=0. Lower seq wins → B is rank 0. + h.ranking().addSessionToTopNGroup(1, sub, /*forward=*/true); + EXPECT_EQ(h.selectCount(ftn("b"), sub.get()), 1); + EXPECT_EQ(h.selectCount(ftn("a"), sub.get()), 0); +} + +// A and B both talk then go silent. B stops last (seq=-2). B should rank +// above A (seq=-1) because B's seq is more negative. +TEST_F(PropertyRankingBaseTest, SeqDown_MostRecentStopper_WinsAmongStoppers) { + RankingHarness h; + auto sub = makeSession(); + + h.ranking().registerTrack(ftn("a"), 0, defaultPublisher_); + h.ranking().registerTrack(ftn("b"), 0, defaultPublisher_); + h.ranking().updateSortValue(ftn("a"), 100); + h.ranking().updateSortValue(ftn("b"), 100); + h.ranking().updateSortValue(ftn("a"), 0); // A stops first → seqDown=-1 + h.ranking().updateSortValue(ftn("b"), 0); // B stops second → seqDown=-2 + + // B has seqDown=-2 < A's seqDown=-1 → B is rank 0. + h.ranking().addSessionToTopNGroup(1, sub, /*forward=*/true); + EXPECT_EQ(h.selectCount(ftn("b"), sub.get()), 1); + EXPECT_EQ(h.selectCount(ftn("a"), sub.get()), 0); +} + +// A and B both increase to the same value. Seq is preserved on increase, +// so A's earlier registration seq (0) beats B's (1). +TEST_F(PropertyRankingBaseTest, SeqUp_EarlierTalker_WinsAmongActiveTalkers) { + RankingHarness h; + auto sub = makeSession(); + + h.ranking().registerTrack(ftn("a"), 0, defaultPublisher_); // seqUp=0 + h.ranking().registerTrack(ftn("b"), 0, defaultPublisher_); // seqUp=1 + h.ranking().updateSortValue(ftn("a"), 100); // seqUp=2 + h.ranking().updateSortValue(ftn("b"), 100); // seqUp=3 + + // A has seqUp=2 < B's seqUp=3 → A rose to 100 first → A is rank 0. + h.ranking().addSessionToTopNGroup(1, sub, /*forward=*/true); + EXPECT_EQ(h.selectCount(ftn("a"), sub.get()), 1); + EXPECT_EQ(h.selectCount(ftn("b"), sub.get()), 0); +} + +// Sanity check: with no value updates, earlier registration still wins. +TEST_F(PropertyRankingBaseTest, RegistrationOrder_PreservedNoUpdates) { + RankingHarness h; + auto sub = makeSession(); + + h.ranking().registerTrack(ftn("a"), 0, defaultPublisher_); // seqUp=0 + h.ranking().registerTrack(ftn("b"), 0, defaultPublisher_); // seqUp=1 + + h.ranking().addSessionToTopNGroup(1, sub, /*forward=*/true); + EXPECT_EQ(h.selectCount(ftn("a"), sub.get()), 1); + EXPECT_EQ(h.selectCount(ftn("b"), sub.get()), 0); +} + } // namespace