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 performance of common subexpression elimination and parallel testing #380

Merged
merged 11 commits into from
Jan 31, 2024

Conversation

rihi
Copy link
Collaborator

@rihi rihi commented Jan 25, 2024

Currently common subexpression elimination uses lists to store which instructions use a specific expression. This PR replaces this usage with pythons Counter class to improve asymptotic runtime.

Additionally this pr adds parallelization to tests using pytest-xdist.

@rihi rihi added the priority-high High priority issue label Jan 25, 2024
@rihi rihi self-assigned this Jan 25, 2024
@rihi rihi marked this pull request as ready for review January 25, 2024 11:39
@blattm blattm self-requested a review January 29, 2024 15:40
Copy link
Collaborator

@blattm blattm left a comment

Choose a reason for hiding this comment

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

The first part of the PR (common subexpression elimination) is looking very good! I just fixed the typing issue you encountered add added some further notes. I think this part is ready to be be merged.

The second part (parallel tests) also seems to be working just fine. However, I am a little concerned about the limitations [1] concerning a consistent order of test parameters. Random errors thrown by pytest-xdist (or worse: maybe even missed tests) should be avoided.
Can we guarantee a consistent order? I already verified, that we only use lists in @pytest.mark.parametrize. It also seems like all functions called to generate such lists produce their output in a well-defined order. However conftest.py uses e.g. glob, which uses an arbitrary order. Applying a sort function seems like a reasonable Idea.

  • Please ensure that all tests from conftest.py will be in a well-defined order. Please include comments in your work about the necessity of well-defined orders because of parallel testing.

  • Do you come to the same conclusion about the "normal" test parameter lists all having a well-defined order?

Thank you for your contribution so far!

[1] https://pytest-xdist.readthedocs.io/en/stable/known-limitations.html

@blattm blattm changed the title Improve performance of common subexpression elimination. Improve performance of common subexpression elimination and parallel testing Jan 30, 2024
@rihi
Copy link
Collaborator Author

rihi commented Jan 31, 2024

I didn't know about that limitation of pytest-xdist. That is a little annoying.
I've changed the code in conftests.py so that test cases are sorted alphabetically by their id (sample_name::function_name), which should provide a deterministic ordering. I've also checked all usages of pytest.mark.parametrize and also think that they all should be deterministic.

Copy link
Collaborator

@blattm blattm left a comment

Choose a reason for hiding this comment

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

Thank you! I will wait for the current run of the extended pipeline to finish and then merge if it all went well.

@blattm blattm merged commit e80b8ca into main Jan 31, 2024
2 checks passed
@blattm blattm deleted the cse-optimization branch January 31, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-high High priority issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants