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

Mac: compute non-BOINC CPU usage in a more accurate way #4875

Merged
merged 4 commits into from
Aug 10, 2022

Conversation

CharlieFenton
Copy link
Contributor

Fixes #4327, #4328

Description of the Change
Implement #4859 for Macintosh computers

static natural_t processorCount = 0;
processor_cpu_load_info_t cpuLoad;
mach_msg_type_number_t processorMsgCount;
static double scale;
Copy link
Member

@AenBleidd AenBleidd Aug 9, 2022

Choose a reason for hiding this comment

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

There is no default value for scale.
In case processorCount is not equal zero, this value will be set to 0 by default, so later this function will always return 0 because the last statement is multiplication with scale.
If this is intended - then fine, but I believe this is a mistake

Copy link
Member

Choose a reason for hiding this comment

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

P.S. If you want just to initialize this static variable once, then it's better to have either some flag or just verify if this value is zero (and then initialize it with a proper value) otherwise just continue execution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AenBleidd I intended this. I have no better solution in case of such errors because the code by @davidpanderson ( line 495 in client/app.cpp) which calls this function has no ability to handle errors returned from total_cpu_time(). The UNIX / Linux version of total_cpu_time() created for #4859 in lib/procinfo_unix.cpp also returns 0 in case of any error opening or parsing /proc/stat. The MSW version of total_cpu_time() in lib/procinfo_win.cpp from #4859 does no error checking at all.

I agree that the code which calls this should take some sort of appropriate action if total_cpu_time() indicates an error by returning 0.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, code looks OK to me. Please let me know when it's tested and ready to merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One possibility for handling errors from total_cpu_time() might be to revert to the old method of calculating non_boinc CPU usage (lines 502 - 523 in client/app.cpp).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

P.S. If you want just to initialize this static variable once, then it's better to have either some flag or just verify if this value is zero (and then initialize it with a proper value) otherwise just continue execution

I'm not sure I understand what you are suggesting. processorCount is always updated by the host_processor_info() call; there is no way to avoid that. But I am having it do double duty, both as a flag allowing me to call sysconf(_SC_CLK_TCK) only once, and also to indicate an error by returning 0 if for some reason processorCount is 0 (as you pointed out in the first comment above.)

Copy link
Member

Choose a reason for hiding this comment

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

This double duty is exactly that I don't like: it's an additional complication and fragile point, and in case you need to change the code, you still should remember that this variable serves as a flag too.
I believe in this case it's acceptable but in general case I'd recommend to use a separate variable as a flag.

@davidpanderson
Copy link
Contributor

I suggest using
static bool first = true
if (first) {
first = false
... get CPU count etc.

Also: total_cpu_time() should return 0 if any error occurred.

@CharlieFenton CharlieFenton marked this pull request as draft August 9, 2022 23:14
@CharlieFenton CharlieFenton marked this pull request as ready for review August 10, 2022 10:30
@CharlieFenton
Copy link
Contributor Author

This code is now ready to be merged.

I tested this by setting local preferences to suspend if non-BOINC CPU usage > 50%, setting the mem_usage_debug flag and letting BOINC run while telling Xcode to do a full build of BOINC. This computer has 4 CPUs, so the Xcode build runs hundreds of instances of the compiler (one per source file) 4 at a time. Each compiler instance runs for a fraction of a second.

With BOINC 7.20.2 (using the old code), BOINC failed to detect the high non-BOINC CPU usage. It reported mostly between 4% and 15%, with 25% a few times. The old code never suspended BOINC.

With the new code, it reported mostly 80% to 90% non-BOINC CPU usage and correctly suspended until the Xcode build finished. So the new code is accomplishing what was intended: better recognizing the CPU load when there are multiple short duration processes.

@AenBleidd AenBleidd merged commit c0c5815 into master Aug 10, 2022
@CharlieFenton CharlieFenton deleted the mac_get_total_cpu_usage branch August 12, 2022 08:17
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.

Non-BOINC CPU usage time is incorrect
3 participants