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

bpo-34060: Report system load when running test suite for Windows #8287

Closed
wants to merge 2 commits into from

Conversation

ammaraskar
Copy link
Member

@ammaraskar ammaraskar commented Jul 15, 2018

See the bpo ticket for more technical details on the implementation and otherwise.

https://bugs.python.org/issue34060

@jkloth
Copy link
Contributor

jkloth commented Jul 15, 2018

A quick note on the LOADAVG_FACTOR_1F and SAMPLING_INTERVAL; that given dampening factor is for a sample rate of 5 seconds. SAMPLING_INTERVAL should really be changed to 5 or re-run the equation with the different interval.

@ammaraskar
Copy link
Member Author

Thanks again Jeremy, I changed the interval to 5 to be more in line with the Linux scheduler.

HQUERY hQuery;
HCOUNTER hCounter;

if ((s = PdhOpenQueryW(NULL, 0, &hQuery)) != ERROR_SUCCESS)
Copy link
Member

Choose a reason for hiding this comment

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

Please move the assignment on a separated line. Same comments for other assignments in if().

goto WMIerror;
}

HANDLE event = CreateEventW(NULL, FALSE, FALSE, L"LoadUpdateEvent");
Copy link
Member

Choose a reason for hiding this comment

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

Hum. It seems like you allocate a resource, but never release it.

I would suggest to create an object which keeps track of these resources and release them later. getloadavg() would be a method of that object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be acceptable to do it in the module's m_free function? I was initially thinking of doing something similar but thought that would create a lot of extra code and complexity for an internal API with one use case.

Copy link
Member

Choose a reason for hiding this comment

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

I dislike using m_free() for that.

@ammaraskar
Copy link
Member Author

Also, I don't think this actually needs a NEWS entry right? It relates to cpython build process and doesn't actually affect the language. Could you mark it with the skip news label if so?

@zooba
Copy link
Member

zooba commented Jul 16, 2018

What extra runtime dependencies does this add to python38.dll (use dumpbin /imports python38.dll and grab the DLL filenames)?

Also, winapi methods are public, if undocumented (since they are supposed to reflect the Windows API calls directly, so that documentation should suffice). If you really want an internal test suite helper it should go in its own extension module with a _ prefix.

(Haven't looked too closely at the implementation yet, but when I do, it will be with the view that it is a public API change. Unless you rework it into its own module first.)

@zooba
Copy link
Member

zooba commented Jul 16, 2018

(Edited above post). Let me take that back - was thinking of winreg module, not the _winapi module. But it still should be treated as a publicly usable API with all the robustness guarantees unless it's in its own module, since it will be used.

@ammaraskar
Copy link
Member Author

Aah yeah I think you have a good point, _winapi despite being undocumented is being consumed by end users https://github.com/search?p=1&q=import+_winapi&type=Code

I think making it its own module would be a bit overkilll so I'll clean up the API anticipating some public usage.

@ammaraskar
Copy link
Member Author

Closed in favor of #8357

Thank you @csabella 😄

@ammaraskar ammaraskar closed this Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants