Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
At the moment serializing an NG entity can be a very expensive operation. We need to refactor the process so that we serialize just the actual data and not the whole field object tree.
Comment | File | Size | Author |
---|---|---|---|
#20 | sleep-2027795-20-interdiff.txt | 1.26 KB | Berdir |
#20 | sleep-2027795-20.patch | 2.04 KB | Berdir |
#16 | sleep-2027795-15-interdiff.txt | 1.23 KB | Berdir |
#16 | sleep-2027795-15.patch | 2.01 KB | Berdir |
#14 | sleep-2027795-14-interdiff.txt | 853 bytes | Berdir |
Comments
Comment #1
plachTagging for the rocketship.
Comment #2
BerdirMore tagging for them rocketships.
Comment #3
yched CreditAttribution: yched commentedMight it be that what we want is "serialized data === data that gets cached in the entity (load) cache" ?
Comment #4
plachSounds sensible :)
Comment #5
BerdirNot sure about the caching, because that's about precalculating things that we don't want in a lot of cases.
Result: 5800 instead of 13000
Comment #6
BerdirUpdating title.
Comment #7
amateescu CreditAttribution: amateescu commentedThis makes very much sense :)
Comment #8
amateescu CreditAttribution: amateescu commentedLooking a bit further into this, it seems that we can also get rid of $fieldDefinitions, thus further reducing the length of the serialized object to 2281.
Comment #9
yched CreditAttribution: yched commentedThe __wakeup() method has a "@todo This should be done before serializing the entity, but we would need to provide the full list of data to be serialized. See the dedicated issue at [this very issue]". Does it mean we could get rid of the @todo here ?
Comment #10
amateescu CreditAttribution: amateescu commentedSomething like this perhaps?
Comment #12
amateescu CreditAttribution: amateescu commentedFailures are correct, this breaks autocomplete becasue the field only contains a 'target_id' => 0 property, while the 'entity' property is stripped. The only reason for which #5 passed is because this test was added 4 days later, on Oct 10th :)
Comment #13
fagoShould be public function.
Also, should we use the serializable interface or sleep? Not sure, what's better here or we've guidelines?
hm, could we make ER-fields include the entity in getValue() when it is about to autosave? I think in this case it makes sense as it's obviously needed to restore the value of the field, i.e. $v = getValue() + setValue($v) should be idempotent. Given that, serialization should work also.
Comment #14
BerdirWe need to use _sleep(), \Serializable behaves differently in 5.4 and 5.3 and is apparently broken in 5.4. We already had to switch Field classes because tests blew up in weird ways.
Otherwise agreed on adding the to-be-saved entity to the ER field item values.
Comment #15
BerdirComment #16
BerdirForgot to make the method public.
Comment #17
yched CreditAttribution: yched commented+1 on the fix, but for future ref it would be nicer if the comment included the reasoning (serialization,, idempotency of setValue(getValue()) in the specific case when 'entity' contains a fresh entity / 'target_id' =0)
Comment #18
Berdir16: sleep-2027795-15.patch queued for re-testing.
Comment #19
amateescu CreditAttribution: amateescu commentedNW for #17. Also, we usually put {@inheritdoc} for implementations of magic methods.
Comment #20
BerdirOk, spent quite a bit of time discussing that comment in IRC, @yched and @timplunkett liked this version :) And according to git log -S"idempotent" is @amateescu a big fan of that word too and will be ok with it I guess ;)
Comment #21
yched CreditAttribution: yched commentedLooks good! Thanks!
(looks like we can't finish issues derailed on comment nitpicks while you're away :-/)
Comment #22
amateescu CreditAttribution: amateescu commented@Berdir, not sure where you get your git logs from but I never used that word :D
Comment #23
tim.plunkettAh, I actually think it's Wim who added some of those, @amateescu was just credited on the #post_render_cache issue :)
Comment #24
Wim LeersHah :)
Comment #25
xjm20: sleep-2027795-20.patch queued for re-testing.
Comment #26
catchCould we profile this? I can see that it means less storage space so makes sense anyway, but is the __sleep() definitely going to be quicker than the extra serializing?
Comment #27
BerdirWas wondering about that too, so I had some fun benchmarking this :)
The question is what to benchmark exactly, I went with a node object that has all field item objects initalized, to see the full gain of this.
HEAD:
With patch:
I wasn't sure if the serialization itself would be faster too but apparently it is. And the real gain is unserialize, because for that we just have to initialize the main object, everything else would then be lazy-instantiated again (which is then going to be slower but usually only a small subset of the object is needed, if at all).
Note that the cloning actually makes up a large part of the seralization, but without it, the comparison is unrealistic and unfair, this is how it would look like:
HEAD:
Patch:
That's because after the first run, __sleep() is pretty much a no-op and doesn't have anything to do at all.
Enough numbers? :)
Comment #28
catchThanks that's great!
Committed/pushed to 8.x