Speed up retrieval of membership data by using a SELECT query #559
Conversation
pfrenssen
added
Drupal 8
Performance
|
I'm getting failures when running this on my large project. I am investigating. It looks like the correct cache contexts are not returned if the user is a "normal" member, only when they have a dedicated role such as administrator. |
This ensures that the SQL-based calculation of the cache context gives the same results as the membership based one.
|
All tests are green now, here and in my project :) |
There was a problem hiding this comment.
I have checked both this and the #555 and they both work fine. However, as I noted in the other PR, I do not see the ::getRolesIds being tested anywhere.
In this test everything is prophesied (I am not very familiar with UnitTests but I see that you are programming what will be returned) and the only other case where ::getRolesIds is called is a test where it can fail if wrong information are returned. Can we have a small addition in the OgMembershipTest Kernel test (or even a UnitTest if you are more comfortable with) that actually test the return of the method?
Or at least point me to where it is if I did not find it.
|
Yes I agree that a KernelTest for the SQL version would be better. There was a bug in the query and I adapted the test, but it is not very reliable in this state. The test only really assures that the query is doing certain conditions but does not really guarantee that it won't regress if for example the conditions change or a new condition is added. |
|
@pfrenssen, should this still be marked as postponed? |
|
Yes this is still postponed on #555 which should go in first. But #555 is now near completion, the latest remarks were addressed. There were some failures but they seem to be infrastructure related. For this PR I would like to convert the unit test for the query into a kernel test so that this is more reliably tested. I can work on this today, this is back in sprint for our project. |
|
#555 is in so this is unblocked! |
By executing the query against a real database instance rather than using a mocked result we can better assert that the functionality is actually working as expected.
Co-Authored-By: Maarten Segers <MPParsley@users.noreply.github.com>
|
Awesome, this concludes the work on #487. With this in the lookup of the cache context is around 14x faster. For more details on the speed improvements see this comment: #487 (comment) |
This is the final PR for #487. This replaces the loading of the (cached)
OgMembershipentities with a directSELECTquery if a SQL database is used for storing the entities. The original approach is kept as a fallback for NoSQL databases as was suggested in #487.On my system this shaves off another ~60ms of the wall time for a user with 1140 groups since the full entities no longer need to be retrieved from cache. This brings the total time down to ~11ms.
This is built on top of #555 so includes all commits from that PR. This is blocked until that PR is merged.