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

Refactoring of cpu topology / game fix for The Forest #167

Merged
merged 2 commits into from
Jan 8, 2024

Conversation

Root-Core
Copy link

@Root-Core Root-Core commented Jan 8, 2024

Supersedes #163

As discussed previously, I have moved the logic for cpu topology limiting to util.py.
I also refactored existing fixes and added a fix for The Forest.

I think, there are plenty of games that would benefit from cpu topology fixes (and maybe these).
It would be nice to identify if some engines struggle in all games and fix them altogether.

CC ValveSoftware/Proton#411


Have a look at ValveSoftware/Proton#5927 (comment)

Moved corresponding logic of gamefixes to util.py

util.py:
- Added functions to set the cpu topology
- Added is_smt_enabled()
- Added get_cpu_count()
@Root-Core Root-Core mentioned this pull request Jan 8, 2024
@@ -661,3 +661,80 @@ def try_show_gui_error(text):
subprocess.run(["notify-send", "protonfixes", text])
except:
log.info("Failed to show error message with the following text: {}".format(text))

def is_smt_enabled() -> bool:
Copy link

@R1kaB3rN R1kaB3rN Jan 8, 2024

Choose a reason for hiding this comment

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

Nit: Since users should be using set_cpu_topology_nosmt , set_cpu_topology_limit and set_cpu_topology to control the topology, I would prefix the function with an underscore to indicate that they're private and shouldn't be invoked directly

_is_smt_enabled()

Copy link
Author

Choose a reason for hiding this comment

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

I think there are valid reasons to use them in fixes, beyond of the topology functions.
For example setting some cpu variable in an ini file or add an argument.

I know nothing about python, is that really how you mark a function as private?
Crazy. But not that stupid.

Copy link

@R1kaB3rN R1kaB3rN Jan 8, 2024

Choose a reason for hiding this comment

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

Python isn't my strongest language either so I don't know too many of its details/conventions, but I believe the runtime wouldn't enforce the function as "private" in the classical OOP sense. As a result, you'd still be able to access them externally without a runtime error

Instead, the underscore would really serve as a friendly indicator that it's intended for internal use. At least, that's what I had assumed as the convention in this codebase. For example, _killhanging() in util.py

Copy link

@R1kaB3rN R1kaB3rN Jan 8, 2024

Choose a reason for hiding this comment

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

I think there are valid reasons to use them in fixes, beyond of the topology functions.

There are valid reasons. It's just I can't foresee how useful those functions would likely be when used externally from a game fix file outside the topology context...

Though, it's just a nit and don't really mind

log.warn(f'SMT status not supported by the kernel (errno: {e.errno})')
return False

def get_cpu_count() -> int:
Copy link

Choose a reason for hiding this comment

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

_get_cpu_count()

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.

3 participants