Skip to content

Refactor PPR top-k selection helper - #723

Open
mkolodner-sc wants to merge 2 commits into
mainfrom
mkolodner-sc/ppr-selector-helper
Open

Refactor PPR top-k selection helper#723
mkolodner-sc wants to merge 2 commits into
mainfrom
mkolodner-sc/ppr-selector-helper

Conversation

@mkolodner-sc

@mkolodner-sc mkolodner-sc commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Description

This PR refactors PPR extraction by moving the existing inline finalized-PPR selection and residual top-up logic into smaller C++ helpers.

The extraction behavior is intended to stay the same, but the implementation is now split into reusable pieces:

  • selectFinalizedPPRPairs: selects finalized PPR candidates by raw PPR score
  • addResidualMassToPPRPairs: adds residual mass to selected finalized PPR candidates when residual top-up is enabled
  • appendResidualTopUpPairs: appends residual candidates into remaining sequence slots

These helpers replace logic that previously lived directly inside extractTopKWithResidualTopUp. The untyped extraction path still owns final output assembly and sorting, but the lower-level pieces can now be reused by the typed-channel PPR extraction work in the stacked PR.

No functional change is intended.

Performance

This refactor should be mostly performance-neutral, if not slightly faster.

The main algorithmic work is the same as before: scan finalized PPR scores, select finalized top-k candidates, optionally scan residuals for top-up candidates, add residual mass to selected finalized candidates, append top-up rows, and sort the emitted rows. The PR mostly moves those existing steps into named helpers.

One small implementation detail changes: finalized PPR selection now uses nth_element followed by a final sort of the selected output rows, instead of relying on partial_sort in the non-top-up path. This avoids ordering intermediate candidates before we know the final emitted set. The asymptotic behavior is unchanged for the final output, but this can save some unnecessary ordering work, so any runtime difference should skew neutral-to-slightly-faster rather than slower.

Testing

  • make -C gigl-core format_cpp
  • git diff --check
  • uv run ty check .github/scripts examples gigl tests snapchat scripts
  • make -C gigl-core unit_test_cpp

@mkolodner-sc
mkolodner-sc force-pushed the mkolodner-sc/ppr-selector-helper branch 5 times, most recently from b5b0bb0 to 3e093f5 Compare July 28, 2026 11:16
@mkolodner-sc
mkolodner-sc force-pushed the mkolodner-sc/ppr-selector-helper branch from 3e093f5 to 930569f Compare July 28, 2026 11:18
@mkolodner-sc mkolodner-sc changed the title Extract PPR top-k selection helper Refactor PPR top-k selection helper Jul 29, 2026

@kmontemayor2-sc kmontemayor2-sc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Matt! LGTM provided we are ok with this behavior change

https://github.com/Snapchat/GiGL/pull/723/changes#r3675350401

Comment thread gigl-core/core/sampling/ppr_forward_push.cpp Outdated
Comment thread gigl-core/core/sampling/ppr_forward_push.cpp Outdated
Comment thread gigl-core/core/sampling/ppr_forward_push.cpp
Comment thread gigl-core/core/sampling/ppr_forward_push.cpp Outdated
Comment thread gigl-core/core/sampling/ppr_forward_push.cpp Outdated
@mkolodner-sc
mkolodner-sc marked this pull request as ready for review July 30, 2026 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants