Process filter string once instead of per test

[IYP-Programer-Yeah]

Fixes #3614

Improve filter performance by removing extra string processing that is performed for each test that can rather be performed only once per execution .

[anholt]

I tried dropping this into our test suite, and it looks like a promising way forward, but the perf is still the same because the cost is in the pattern matching, not splitting on ':' (and I did confirm by the profile that I'm running your code).

But I think this is a good base for implementing the set suggestion to fix the algorithmic complexity issue.

[IYP-Programer-Yeah]

Yeah, your suggestion may improve the performance but the Big O complexity will be the same, since the glob matching algorithm is O(N) which in terms of complexity is no different than normal string matching, this fix rather tackles the Big O complexity and improves that.
I am doubtful about the benefits of separating the glob and non-glob patterns, because there will be extra overhead introduced, while the complexity will stay the same, but the performance for matching normal strings will be improved. Which means there is going to be a trade off between the overhead we're going to introduce and the performance improvements, which makes me think it is not going to be a fruitful endeavor.
If you can share a bit of your latest profiling results, it can open up ways for further improvements in the future. (The FilterMatchesTest function, for which you previously provided profiling results, is no longer used in the code, so new profiler results should indicate the issue you're encountering atm.)

[IYP-Programer-Yeah]

Let's discuss more on it. My opinion may help if not for current then for future abstract processing
…
On Oct 21, 2021 at 2:25 AM, <Hossein GhahremanzadehAnigh @.***)> wrote: Yeah, your suggestion may improve the performance but the Big O complexity will be the same, since the glob matching algorithm is O(N) which in terms of complexity is no different than normal string matching, this fix rather tackles the Big O complexity and improves that. I am doubtful about the benefits of separating the glob and non-glob patterns, because there will be extra overhead introduced, while the complexity will stay the same, but the performance for matching normal strings will be improved. Which means there is going to be a trade off between the overhead we're going to introduce and the performance improvements, which makes me think it is not going to be a fruitful endeavor. If you can share a bit of your profile results it can open up ways for further improvements in the future. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub (#3615 (comment)), or unsubscribe (https://github.com/notifications/unsubscribe-auth/ARB7WBOYYLZU3VMKHJTAJN3UH6W6RANCNFSM5GEWSUTA). Triage notifications on the go with GitHub Mobile for iOS (https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or Android (https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub).

With recent bot spams I'm not sure if this is a spam or a legitimate mod, @derekmauro @dinord can you follow up on on this?

I think further discussion should take place on the issue rather than this merge request, since this merge request obviously does not aim to fix every issue in the repository but rather part of it. I think suggestion for other fixes should go into the issue and be handled on later merge requests (if not related to the current one).

[anholt]

It's O(n^2) (n=number of tests) if one is running all the tests by using filters naming each test across different invocations of the testsuite. By making a set of non-glob strings, then it reduces to O(n) (O(1) lookup per test name) assuming your number of processes is fixed (for example because you have one per CPU)

[IYP-Programer-Yeah]

I'm not sure if I understand you correctly. You want to use a hashmap like data structure to do O(1) access to the tests that are named exactly, rather than doing a O(n*n) string matching? That works, not sure though if that's going to be considered common use to get accepted as a merge request (if mods agree that it is a good way to improve performance looks like that can be handled in an other merge request).

[IYP-Programer-Yeah]

Hey, just wanted to ping this PR for review. @dinord

[IYP-Programer-Yeah]

@derekmauro @dinord wanted to ping so that this merge request doesn't go stale.

[dinord]

Apologies for the delay, we'll begin reviewing this shortly.

[IYP-Programer-Yeah]

Pinging again. Honestly this is extremely odd that a merge request takes 5 weeks.

[dinord]

Neat.

[IYP-Programer-Yeah]

@dinord requested changes are applied.

[IYP-Programer-Yeah]

@dinord The main branch is failing, and I do not think the failure in the CI is on my part. Also I have applied the requested changes.

[IYP-Programer-Yeah]

Wanted to ping again.

[IYP-Programer-Yeah]

Is any one actually reviewing this code?

[dinord]

We are reviewing it. However, there are issues with how this proposed change interacts with our internal codebase (even after the latest commit).

[IYP-Programer-Yeah]

Ah ok, it would be nice to provide some feedback. Since I try to actually be helpful and proceed with requested changes as early as possible. Thanks for the response.

[dinord]

I released a bunch of internal test cases for the matching logic in 97a4675.
Make sure to rebase and run against them.

[IYP-Programer-Yeah]

Thanks for the review will read soon and apply changes accordingly.

[IYP-Programer-Yeah]

@dinord Requested changes are applied, and the newly added tests pass. I want your confirmation on the last open discussion.

[IYP-Programer-Yeah]

@dinord Any other issues to be concerned about?

[IYP-Programer-Yeah]

Happy new year, any hopes of getting this merged this year :D

[IYP-Programer-Yeah]

@dinord Should I continue pinging this? or there is no hopes of getting this merged? Are there more internal errors?

[joshiayush]

@dinord Should I continue pinging this? or there are no hopes of getting this merged? Are there more internal errors?

@IYP-Programer-Yeah There's no activity in this repository for the last 14 days, not only your PR is unmerged but there are other PRs as well, likely because the engineers are not able to devote time here but it's been 2 weeks so there are chances that they will be coming here soon :). You have to be patient :).

[dinord]

Thanks for addressing all the comments. I've started the import process once again. Hopefully we'll get this PR merged early next week.