Skip to content

Make the NRT stats counters optional. - #8235

Merged
sklam merged 6 commits into
numba:mainfrom
stuartarchibald:wip/nrt_optional_stats
Jul 30, 2022
Merged

Make the NRT stats counters optional.#8235
sklam merged 6 commits into
numba:mainfrom
stuartarchibald:wip/nrt_optional_stats

Conversation

@stuartarchibald

@stuartarchibald stuartarchibald commented Jul 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

As title. They can be switched on/off with env var:
NUMBA_NRT_STATS. Testing is updated to ensure that they are on
when running CI/conda build test phase.

Fixes #8156

Depends on #8106, only the final patch (b3c7d18) is new. Rebased, dependency resolved, a73612b is b3c7d18.

This was referenced Jul 8, 2022
Closed
Closed
@stuartarchibald
Make the NRT stats counters optional.
a73612b 
As title. They can be switched on/off with env var:
`NUMBA_NRT_STATS`. Testing is updated to ensure that they are on
when running CI/conda build test phase.

Fixes numba#8156
@stuartarchibald
stuartarchibald force-pushed the wip/nrt_optional_stats branch from b3c7d18 to a73612b Compare July 11, 2022 08:58
@stuartarchibald
stuartarchibald marked this pull request as ready for review July 11, 2022 10:48
@stuartarchibald
stuartarchibald requested a review from sklam as a code owner July 11, 2022 10:48
@esc
esc requested review from gmarkall and removed request for sklam July 12, 2022 14:32
@esc esc added this to the Numba 0.57 RC milestone Jul 12, 2022
gmarkall
gmarkall reviewed Jul 19, 2022
View reviewed changes

@gmarkall gmarkall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The code changes look fine to me.

I have one query, which is about the use of the canary value for stats values when counting is disabled - wouldn't it be be safer / more Pythonic to raise a Python exception if the counter values are requested when counting is disabled? Presently the risk is that an invalid value is returned unnoticed - or, it needs to be checked for and dealt with anyway.

Comment thread numba/core/runtime/nrt.cpp
int shutting;
/* Stats */
std::atomic_size_t stats_alloc, stats_free, stats_mi_alloc, stats_mi_free;
struct {

@gmarkall gmarkall Jul 19, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The struct is a nice style improvement :-)

@gmarkall gmarkall added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jul 19, 2022
@stuartarchibald
Improve the user experience of NRT stats counters.
6da63c3 
* Makes it such that querying NRT stats counters when they are
  disabled results in a RuntimeError.
* Updates the Numba derived unittest TestCase to ensure that stats
  counters are on by default.
* Adds tests for the stats counters API and more tests for the
  associated NUMBA_NRT_STATS environment variable.
* Reverts changes to CI integrations setting the env var, the test
  suite handles this now.
@stuartarchibald
Fix typo.
214de1e 
As title.
@stuartarchibald
Fix issue with enabling stats in multiprocess test runner.
1287780 
As title.
@stuartarchibald
Fix up unit tests that use the NRT stats counters.
8aeb510 
As title.
@gmarkall

gmarkall commented Jul 20, 2022

Copy link
Copy Markdown
Member

/azp run

(Checking if the test running issues are resolved)

@azure-pipelines

azure-pipelines Bot commented Jul 20, 2022

Copy link
Copy Markdown
Contributor
Command 'run

(Checking' is not supported by Azure Pipelines.



Supported commands

  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@gmarkall

gmarkall commented Jul 20, 2022

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

azure-pipelines Bot commented Jul 20, 2022

Copy link
Copy Markdown
Contributor
Azure Pipelines successfully started running 1 pipeline(s).

@gmarkall

gmarkall commented Jul 21, 2022

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

azure-pipelines Bot commented Jul 21, 2022

Copy link
Copy Markdown
Contributor
Azure Pipelines successfully started running 1 pipeline(s).

@stuartarchibald stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Jul 21, 2022
@stuartarchibald

stuartarchibald commented Jul 21, 2022

Copy link
Copy Markdown
Contributor Author

The code changes look fine to me.

I have one query, which is about the use of the canary value for stats values when counting is disabled - wouldn't it be be safer / more Pythonic to raise a Python exception if the counter values are requested when counting is disabled? Presently the risk is that an invalid value is returned unnoticed - or, it needs to be checked for and dealt with anyway.

From an OOB conversation, it was decided that the more Pythonic thing is to raise a Python exception if the counter values are requested when counting is disabled. This however would break the test suite out-of-the-box as there's a lot of tests that check for reference leaks and need the stats counters to be enabled. This is addressed by updating the test suite to enable the stats counters if the reference leak checking mixin is used, it also adds a new mixin to enable the stats counters as some tests use them for other purposes.

gmarkall
gmarkall approved these changes Jul 21, 2022
View reviewed changes

@gmarkall gmarkall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the updates - I think this now looks good. As agreed per OOB discussion, the best path is for the EnableNRTStatsMixin to always disable stats counters when it tears down - generally stats counters should be off, and any test that requires them should be using the mixin to make sure they're on - therefore the current state is most likely to catch any errors in the expectations of tests about the enablement of counters.

@esc / @stuartarchibald could this have a full buildfarm run please?

@gmarkall gmarkall added Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jul 22, 2022
@stuartarchibald

stuartarchibald commented Jul 27, 2022

Copy link
Copy Markdown
Contributor Author

Buildfarm ID: numba_smoketest_cpu_yaml_117.

@stuartarchibald

stuartarchibald commented Jul 28, 2022

Copy link
Copy Markdown
Contributor Author

Buildfarm ID: numba_smoketest_cpu_yaml_117.

Passed.

@stuartarchibald stuartarchibald added BuildFarm Passed For PRs that have been through the buildfarm and passed 5 - Ready to merge Review and testing done, is ready to merge and removed Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm 4 - Waiting on CI Review etc done, waiting for CI to finish labels Jul 28, 2022
@sklam
sklam merged commit e4295a4 into numba:main Jul 30, 2022
@sjperkins sjperkins mentioned this pull request May 23, 2023
Closed
2 tasks
@dlee992 dlee992 mentioned this pull request Sep 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmarkall gmarkall gmarkall approved these changes

Assignees

@gmarkall gmarkall

Labels

5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed Effort - medium Medium size effort needed

Projects

None yet

Milestone

Numba 0.57 RC

Development

Successfully merging this pull request may close these issues.

Make NRT stats counters optional, off by default

4 participants

@stuartarchibald @gmarkall @esc @sklam