Problem/Motivation
ResourceTypeRepository::get is looping excessively over the list of resources exposed by the API. In my case I have ~120 entity types and not all of them are likely to be used but they are still iterated over. I am getting 50k+ calls to $resourceType->getEntityTypeId() and around 1k calls to $this->all() method.
Proposed resolution
Introduce a static cache that will have entity:bundle mapping over the exact resource in the list or keep a tomb-store record in case it's missing, so we spare any full list loops.
Remaining tasks
Discussion, patch, benchmarks.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
None.
Comment | File | Size | Author |
---|---|---|---|
#14 | Screen Shot 2018-11-30 at 14.08.50.png | 469.35 KB | gabesullice |
#13 | 3017239-13.patch | 1.35 KB | gabesullice |
#13 | interdiff.txt | 1.27 KB | gabesullice |
#10 | 3017239-10.patch | 1.38 KB | ndobromirov |
#10 | interdiff-10-5.txt | 1.32 KB | ndobromirov |
Comments
Comment #2
ndobromirov CreditAttribution: ndobromirov at FFW commentedComment #3
Wim LeersI've never liked that bit either. I'm +1 to your general proposal :)
Mind rolling a patch?
Comment #4
gabesullice+1, BUT #3015438: Wrap entity objects in a ResourceObject which carries a ResourceType may make this less consequential. Nonetheless, it's a small change that makes sense.
Comment #5
ndobromirov CreditAttribution: ndobromirov at FFW commentedHere is the patch and the after benchmarks (9-10 times faster).
Comment #6
gabesullice@ndobromirov, can you reroll the patch to use a protected property, so it won't break when we do #3016733: Simplify ResourceTypeRepository; use a protected property in place of an in-memory cache bin.
Comment #7
gabesulliceComment #8
ndobromirov CreditAttribution: ndobromirov at FFW commentedYeah but how will the cache invalidation happen based on cache tags on a private property?
Otherwise I am all for it. It's going to be even faster :)
Comment #9
e0ipso@ndobromirov we should not need cache invalidation in this case, should we? Nothing that invalidates the cached entry should change mid-request.
Comment #10
ndobromirov CreditAttribution: ndobromirov at FFW commentedHere is the newly requested version :). No benchmarks - should be faster...
Comment #11
gabesulliceI think this is now unnecessary? Otherwise, I think this looks good.
Comment #12
ndobromirov CreditAttribution: ndobromirov at FFW commentedIt still is. We are checking with an
isset()
for cache presence.Having a key that is with a NULL value is treated by PHP as not set, thus a cache miss. We want to have that cached as well, so we are not looping over all of the list to find that it's not in it again and again.
Comment #13
gabesullice8.5 failed because it's broken on HEAD. I find the attached a little bit easier to reason about. It's just me being nitpicky. If tests pass and @ndobromirov is +1, I'll commit.
Comment #14
gabesullice@e0ipso brought up the performance of
array_key_exists
vs.isset
and we had this exchange:Comment #16
e0ipsoCommitted and credited @ndobromirov, who is great and is helping a lot!
Comment #17
Wim Leers🎉
Comment #18
gabesullice👏 @ndobromirov
Comment #19
ndobromirov CreditAttribution: ndobromirov at FFW commentedIt could be
Isset() || array_key_exists()
to handle both cases. I was trying to not have the function call at all.