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

Enabling Perflint checks #3403

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

martinhoyer
Copy link
Collaborator

@martinhoyer martinhoyer commented Dec 4, 2024

Will probably have negligible performance improvements, but it seems like a low-hanging fruit.
The PERF203 would be a bit more tricky to address fully, but I've at least tried to fix those simpler cases.

Depends on #3402

Pull Request Checklist

  • implement the feature

@martinhoyer martinhoyer added the code | style Code style changes not affecting functionality label Dec 4, 2024
@martinhoyer martinhoyer self-assigned this Dec 4, 2024
@martinhoyer martinhoyer added status | blocked The merging of PR is blocked on some other issue code | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. ci | full test Pull request is ready for the full test execution labels Dec 4, 2024
@martinhoyer martinhoyer changed the title Perf203 Enabling Perflint checks Dec 4, 2024
@@ -84,22 +84,17 @@ def _nitrate_find_fmf_testcases(test: 'tmt.Test') -> Iterator[Any]:
"""
import tmt.base
assert nitrate
for component in test.component:
try:
with suppress(tmt.utils.StructuredFieldError, nitrate.NitrateError):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does this change make sense @happz? I'm thinking

try:
    with suppress(tmt.utils.StructuredFieldError):

except nitrate.NitrateError:
    pass

but not sure if there is an advantage in doing so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed in 64f6242

@lukaszachy
Copy link
Collaborator

@martinhoyer Huston, a little problem - https://stackoverflow.com/questions/34566806/why-use-contextlib-suppress-as-opposed-to-try-except-with-pass

I've checked same timeit command on python3.13 (my laptop) and it indeed is much worse with suppress.
So I'd say different approach is necessary if the whole point is to get better performance :/

@martinhoyer
Copy link
Collaborator Author

martinhoyer commented Dec 5, 2024

@martinhoyer Huston, a little problem - https://stackoverflow.com/questions/34566806/why-use-contextlib-suppress-as-opposed-to-try-except-with-pass

I've checked same timeit command on python3.13 (my laptop) and it indeed is much worse with suppress. So I'd say different approach is necessary if the whole point is to get better performance :/

Nice, good info. Thanks!

Actually the supress wasn't added as part of performance optimization. The perflint is complaining about try except being inside the loop.
We can do the "same" changes without the supress, by moving the loops within try-except(where applicable).

btw, have you run the timeits with some of the changed code here? Just curious if it's also slower.

@martinhoyer
Copy link
Collaborator Author

Out of curiosity, I've tried to solve the remaining PERF203 and tested modifying this function:

    for i in range(attempts):
        try:
            return func(*args, **kwargs)
        except Exception as exc:
            exceptions.append(exc)
            logger.debug(
                'retry',
                f"{label} failed, {attempts - i} retries left, "
                f"trying again in {interval:.2f} seconds.")
            logger.fail(str(exc))
            time.sleep(interval)
    raise RetryError(label, causes=exceptions)

to

    retries_left = attempts

    while retries_left > 0:
        retries_left -= 1
        try:
            return func(*args, **kwargs)
        except Exception as exc:
            exceptions.append(exc)
            logger.debug(
                'retry',
                f"{label} failed, {retries_left} retries left, "
                f"trying again in {interval:.2f} seconds.")
            logger.fail(str(exc))

            if retries_left > 0:
                time.sleep(interval)

    raise RetryError(label, causes=exceptions)

An the test results over a milion iterations:
Average 1: 0.0000056165 seconds
Average 2: 0.0000056047 seconds

image

Anyway, I'm going to do something useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci | full test Pull request is ready for the full test execution code | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. code | style Code style changes not affecting functionality status | blocked The merging of PR is blocked on some other issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants