-
-
Notifications
You must be signed in to change notification settings - Fork 648
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
Collection indexes #3671
Collection indexes #3671
Conversation
@tpw56j You are putting out PRs faster then I can review them. 😂 Thank you for the contribution again, I will try to get to it as soon as possible. 😄 |
@SchoolGuy This PR is only a draft; there is still a lot of work to be done on it to be ready for review. |
faad11d
to
5572dee
Compare
@tpw56j A customer has asked already for this PR to be backported. As I can't do that before this PR is finished I would love it if we could agree on how to split this PR so that both of us can work on it simultaneously. As a first step, I would say that adding hard limits to the performance testsuite in a split-out PR is desirable, right? |
@SchoolGuy I'm almost done with this PR and will try to post the changes I made tonight or tomorrow. |
@tpw56j That sounds like a solvable challenge for me. Testing is what "I grew up with" and as such is natural to me. I'll do my best next week after I have returned from some family matter that needs to be taken care of. |
7905d40
to
eab51ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some small suggestions, but the rest looks good to me
It appears that with 50 there are less failing tests. I'll trigger a testrun for a master file so we can maybe get it working with 50 rounds. |
It appears that running a job longer the 6h is not possible with GitHub Action Runners. - https://github.com/orgs/community/discussions/26679 As we cannot easily combine fixtures we need to lower the runs. I'll try 40 runs... |
Okay so with 40 rounds the performance of the repo copy is varing by 80%... The results are just completely random. This is actually worse then with 25 rounds. Edit: Funny enough the deserialize, get autoinstall, start and sync tests are fine. It is just the edit methods that take very long that are that flaky. |
@SchoolGuy Maybe something will change if you add |
@tpw56j We can try but I find it unlikely as we are spreading the tests already about a good chunk of time. Lastly I am comparing the mean time so a single or two bad runs should already be fine since we also will have good runs. |
@SchoolGuy Then we can try to increase the number of iterations and reduce the number of rounds, for example |
@tpw56j Since we group tests by name I believe that this is not happening. Feel free to check the results to take a more educated guess but I believe that the shared nature of the VMs is causing this behaviour. Locally all of those tests run rock solid when I go with anything above 10 iterations. |
Summary after I looked at the results again:
|
Sudden idea in the train: Since many of our tests are related indectly to the search of the application - and this PR was created on the observation it is flawed - we could add dedicated benchmarks for this. If that test is stable we should be able to focus the attention on other possibilities. |
It may be that GitHub actions are not that unstable after all. The test
|
One performance bottleneck with Here is my weirdly structured notes that display the call chain. @tiltingpenguin please write a comment that confirms if I am right or wrong...
|
Yes, this is the call chain I found as well |
I think there was a slight slowdown in searching for non-indexed attributes, but overall I didn't notice a significant performance hit for It might make sense to add non-unique indexes for for these attributes . My results for
This PR:
|
@tpw56j I have discussed with several colleagues and friends offline and so far the conclusion that the shared nature of the GitHub Runners is causing the variation. I will attempt to address the open review suggestions by @tiltingpenguin and then we'll merge this PR. Mid term I will sponsor a dedicated cluster that will ensure that performance testing is much more stable. To prevent keeping this project tied to me, I will transparently document the required steps to duplicate the setup. |
This makes the tests fail if the mean average is different by 10%.
fbb8437
to
4e60242
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments were addressed, so it LGTM now
@tpw56j If you have no reservations about the changes that I added to your PR I would merge the PR independent of the results in the CI. I would address the CI failures as soon as time allows. I verified manually that the changes have the desired effect that you advertised. |
@SchoolGuy I have no reservations about the changes that you have added to my PR. |
I will merge this PR even though the performance testsuite is not working. It was established that the GitHub infrastructure is at fault for this. |
Linked Items
Fixes #3341
Fixes #3169
Description
Adds indexing of collections by non-key properties.
Behaviour changes
Old: Quick search is supported only by item name.
New: A quick search can be performed using non-key properties.
There is additional overhead for updating indexes and allocating memory for them.
Category
This is related to a:
Tests