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

Improve process spawn speed and remove trailing lines. #50

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matu3ba
Copy link

@matu3ba matu3ba commented Dec 6, 2024

No description provided.

Problem: # FIXME: popen is slow
Solution: use subprocess.run
@matu3ba
Copy link
Author

matu3ba commented Dec 6, 2024

Should I file more issues for all the formatting and linting issues ruff shows or what is the policy?

With ruff.toml (ruff version ruff 0.8.2)

line-length = 100

[format]
quote-style = "single"
indent-style = "space"

and running ruff format I get

cpuset/commands/common.py |   5 +--
 cpuset/commands/mem.py    |  44 +++++++++++-----------
 cpuset/commands/proc.py   | 319 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------
 cpuset/commands/set.py    | 283 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------
 cpuset/commands/shield.py | 196 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------
 cpuset/config.py          |  22 +++++++----
 cpuset/cset.py            | 288 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------
 cpuset/main.py            |  77 +++++++++++++++++++++-----------------
 cpuset/util.py            |  54 ++++++++++++++++-----------
 setup.py                  |  44 +++++++++++-----------
 t/test_cset.py            |  66 ++++++++++++++++-----------------
 t/test_util.py            |  11 +++---
 12 files changed, 789 insertions(+), 620 deletions(-)

ruff check shows me

Found 160 errors.
[*] 24 fixable with the `--fix` option (12 hidden fixes can be enabled with the `--unsafe-fixes` option).

You might want to run this yourself to have less stuff to review.

Copy link
Member

@Werkov Werkov left a comment

Choose a reason for hiding this comment

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

The whitespace cleanup (c8d6482) looks good to me, thanks.
The subprocess substitution isn't likely speeding up it as expected.

There is no coding style policy. Would unified style help you before further modifications or what is it helpful for (it's non-functional change, right)?

# --> use /proc/<pid>/status -> Cpus_allowed
# int(line.replace(',',''), 16)
# note: delete leading zeros to compare to allcpumask
line = os.popen('/usr/bin/taskset -p ' + str(proc) +' 2>/dev/null', 'r').readline()
taskset_res = subprocess.run(['/usr/bin/taskset -p', str(proc)], capture_output=True)
line = taskset_res.stdout.readline()
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this is really faster?
I think the comment suggests reading info from procfs directly instead of spawning a helper program.

Copy link
Author

Choose a reason for hiding this comment

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

Would unified style help you before further modifications or what is it helpful for (it's non-functional change, right)?

Unfortunately ruff as linter shows on linting both formatting and non-formatting problems. From my experience linting catches mostly unhandled cases and type confusion.
Personally I disagree very often with what Python decided in their formatting pep(s), but configuring it feels too painful/annoying.

The subprocess substitution isn't likely speeding up it as expected.

My bad, will read from procfs directly.

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