Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NPS measurement for TC scaling #2081

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Viren6
Copy link

@Viren6 Viren6 commented Jun 19, 2024

This PR modifies the NPS measurement for TC scaling to more closely resemble actual testing conditions. In particular, it addresses the point raised in #2077

Currently we run one process with a bench and one process with the search of n-1 threads. This doesn't account for these RAM bandwidth limitations discussed and therefore the measured nps is far faster than the real nps.

Instead, this PR runs a bench process for each active core and takes the average NPS.

Co-Authored-By: Jamie Whiting <99771266+jw1912@users.noreply.github.com>
@Viren6
Copy link
Author

Viren6 commented Jun 19, 2024

@vondele
Copy link
Member

vondele commented Jun 19, 2024

I think that's reasonable direction. Things to consider (idk the right answer)

  • this will penalise SMP tests, where the actual nps will be higher than the one measured in this way. Could be solved by doing some SMP measurement for SMP tests.
  • I have observed that on very large core workers the 1 second test might actually not be such a good measurement, as the system is spawning engines and only once everything is running the measurement becomes stable.
  • This effectively changes the TC for the progression test, so will have some effect there. Maybe that's something to consider merging shortly after release (i.e. when we usually update the reference nps?).

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.

2 participants