Skip to content

Conversation

@Unisay
Copy link
Contributor

@Unisay Unisay commented Jan 15, 2026

Summary

Fixes #7525

The valueContains cost model uses const_above_diagonal which requires benchmark data in two regions:

  • Above diagonal (y > x): contained size > container size → constant cost (early exit)
  • Below diagonal (x >= y): container size >= contained size → linear cost

However, the benchmark function only generated samples in the below-diagonal region (contained was always a subset of container).

Changes

Benchmark Generation

  • Removed misleading aboveDiagonalCombos variable (referred to Value structure diagonal, not costing diagonal)
  • Added 20 true above-diagonal samples where contained.totalSize > container.totalSize with varied size ratios:
    • Small containers (4-16 entries) with larger contained values
    • Medium containers (25-100 entries) with larger contained values
    • Large containers (150-1024 entries) with much larger contained values
    • Very large containers (800-1024 entries) with extreme contained values
  • Updated comments to clarify two different diagonal concepts:
    • Value structure diagonal: numPolicies vs tokensPerPolicy (internal map shape)
    • Costing diagonal: container totalSize vs contained totalSize (used by cost model)

Cost Model Updates

  • Re-ran remote costing benchmarks with expanded above-diagonal coverage (20 samples)
  • Updated builtinCostModelA/B/C.json with refined parameters:
    • Above-diagonal constant: 233391 → 246085 (+5.4%)
    • Below-diagonal intercept: 658617 → 686640 (+4.3%)
    • Slope parameters adjusted for better fit
  • Updated benching-conway.csv with new benchmark data (~570 total points)

…chmark

Two different "diagonal" concepts were confused:
1. Value structure diagonal (numPolicies vs tokensPerPolicy)
2. Costing diagonal (container totalSize vs contained totalSize)

The previous `aboveDiagonalCombos` tested #1 but the cost model needs #2.

Changes:
- Remove misleading `aboveDiagonalCombos` variable
- Add true above-diagonal samples where contained size > container size
- Update comments to clarify both diagonal concepts
- Generate 3 independent (container, contained) pairs for the constant-cost region

Closes #7525
@Unisay Unisay requested a review from kwxm January 15, 2026 10:55
@Unisay Unisay self-assigned this Jan 15, 2026
@Unisay Unisay added the No Changelog Required Add this to skip the Changelog Check label Jan 15, 2026
@Unisay Unisay force-pushed the yura/issue-7525-fix-valuecontains-above-diagonal branch from cc3fe66 to 4c8c0c3 Compare January 15, 2026 11:56
Copy link
Contributor

@kwxm kwxm left a comment

Choose a reason for hiding this comment

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

Let's have a few more points above the diagonal!

Grid: 10×10 power-of-2 below-diagonal (55 combos)
Samples per container: 10 evenly distributed contained sizes
Total points: ~570
Above-diagonal: 3 independent (container, contained) pairs
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe 10 or 20 with some of them containing larger values would be better, just to make sure that we get a decent average: 3 points seems a bit small.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Kenneth requested more data points above the diagonal for a better
average of the constant-cost region.

Changes:
- Increased from 3 to 20 above-diagonal samples
- Added variety in container/contained size ratios:
  * Small containers (4-16 entries) with larger contained
  * Medium containers (25-100 entries) with larger contained
  * Large containers (150-1024 entries) with much larger contained
- Total benchmark points: ~570 (550 below + 20 above diagonal)
Updated valueContains cost model parameters based on benchmark run with expanded above-diagonal coverage (3→20 samples).

Cost parameter changes:

- Above-diagonal constant: 233391 → 246085 (+5.4%)

- Below-diagonal intercept: 658617 → 686640 (+4.3%)

- Slope parameters adjusted for better fit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

No Changelog Required Add this to skip the Changelog Check

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cost-model): valueContains benchmark missing above-diagonal samples

3 participants