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

Performance improvement #16

Closed
wants to merge 1 commit into from

Conversation

DaKnig
Copy link
Contributor

@DaKnig DaKnig commented Dec 11, 2022

this branch will track whatever performance improvements I can come up with and depends on #14 .
I will only add here changes that provably improve the performance in the tests available to me.
I will also later add the tests in a separate branch, including the setup to check performance.

Copy link
Owner

@eievui5 eievui5 left a comment

Choose a reason for hiding this comment

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

This is great, thank you :)
Let me know if there's more you wanna add to this PR, otherwise I can merge this.

@@ -185,9 +185,12 @@ impl TestConfig {
result: None,
}
}
fn set_name(&mut self, name: String) {
Copy link
Owner

Choose a reason for hiding this comment

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

What is this function for? I'm not a fan of setter/getters that don't do anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's used in line 304. I want to change the whole approach from "do it with the global test, then the local one" to only have normal test configs and initialize them with the global config, but then I need to set the new name of course :) I realized an issue with my current approach and I think the whole TOML reading routine should be replaced. it is all WIP.

@DaKnig
Copy link
Contributor Author

DaKnig commented Dec 15, 2022

this is an ongoing PR I think I have a lot to contribute in terms of performance stuff :) I am trying to understand the characteristics of the performance. so far it seems like parallelizing does not add any speed. I have had a local version that did that and really even on huge tests the bottleneck is the test file parsing. there must be better ways to improve the speed before resorting to parallelism even if it is quite easy to implement.

@DaKnig
Copy link
Contributor Author

DaKnig commented Dec 15, 2022

I will add some changes that add tests, some performance metrics :)

@eievui5
Copy link
Owner

eievui5 commented Dec 22, 2022

Could you rebase this? A lot of the changes seem out of date (copies of your first PR?) and some new stuff has been added.

@DaKnig
Copy link
Contributor Author

DaKnig commented Dec 25, 2022

the changes in this branch were merged already... would have been easier to track if merge policy was a bit different ;-)

@DaKnig DaKnig closed this Dec 25, 2022
@DaKnig DaKnig deleted the performance-improvement branch December 25, 2022 06:24
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.

None yet

2 participants