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 71387c1c..bc44f367 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()]++; // Update extended threshold since a publisher's track count changed. @@ -97,8 +97,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 fc8919a8..437a7d08 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) { @@ -373,7 +378,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}; // Maximum effective window across all publisher-subscribers: max(N + selfTrackCount). // Used in crossesThreshold() to avoid fast-path exits that would miss updates 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