Entity collection endpoint can request 50 content entities at once if we have 20 fields on content type then the serialization could take a while and on top of that, each field can have multiple values and multiple serializable properties.
sparse_fieldset value allow us to remove the unwanted fields. Let's say we only need 10 fields values in the response if we only serializer those 10 fields we can save a bunch of processing time.
Right now jsonapi serializes all the entity fields in \Drupal\jsonapi\Normalizer\EntityNormalizer::normalize() and then omits them.
Only serialize sparse_fieldset fields in \Drupal\jsonapi\Normalizer\EntityNormalizer::normalize()
Figure out that it is not messing up with cacheablity metadata.
None.
None.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 3011099-15-8.x-1.x.patch | 1.91 KB | wim leers |
| #4 | 3011099-2.patch | 1.03 KB | jibran |
Comments
Comment #2
jibranComment #4
jibranNow with the right patch.
Comment #5
gabesullice@jibran++
I had this exact change in #2997600: Resolve included resources prior to normalization at one point (this was impossible prior to that issue) but I removed it because it was out of scope. I forgot to open a followup though! Thanks for being on top of it!
Comment #6
wim leers😲🙈
I wish I was the one who had spotted this! Such low-hanging fruit! AWESOME! 👏
Can we get some quick profiling? I'm sure the numbers look great :D
Comment #7
jibranMy local machine is slow so the response time in the browser is not great. I'll grab some blackfire.io stats on Monday.
Comment #8
wim leersMuch appreciated!
Comment #9
jibranHere is the testing scenario.
Before applying the patch.
No page cache, no dynamic_page_cache. Disabled xdebug.
Cleared drupal caches.
Visit the jsonapi endpoint for cold cache profile.
Visit the jsonapi endpoint warm cache profile.
Repeat all of the above after applying the patch.
Little background this content type has 21 fields + 2 computed fields not including basefield. Sparse fieldset consist of 6 fields + 1 computed field.
Cold Cache
Warm Cache
Comment #10
e0ipsoFood for thought. I have followed this approach in the past in a node.js project. We saw a much better improvement though always serializing the whole entity, and then removing the unwanted fields at the end. We cache the serialized entity, therefore we have an application cache hit for any combination of sparse field sets.
I think this is a good approach and we should get this merged, although I'd like to get impressions on the above. BTW, this approach was already described in a screenshot of an IRC conversation about performance with @gabesullice posted elsewhere.
Comment #11
jibranThis is a valid point one worth exploring in follow-up. IMO this depends on case by case bases maybe make it configurable using jsonapi_extras per entity bases?
Comment #12
jibranPatch for 1.x if anyone needs it.
Comment #13
e0ipsoThanks for the work @jibran!
I'll be adding the patch in #12 to Contenta CMS while we get around upgrading to JSON API 2.x.
Merging #4.
Comment #15
gun_dose commentedPatch from #12 causes error
With this patch $normalized_field variable is used before it declared. So this patch should be removed from Contenta as soon as possible.
Also you can see that test of this patch is failed, so I changed issue status to "Needs work".
PS: it is very strange decision to apply patch to Contenta without testing.
Comment #16
wim leersPlease report this at https://github.com/contentacms/contenta_jsonapi :)