#325665: Cache registry lookups introduced _registry_set_lookup_cache() and _registry_get_lookup_cache(), and worked a lot to avoid using serialize() there... under the false impression that it would lessen the memory cost... but it turns out it is less expensive, both in memory and CPU cycles to just call serialize() directly.

CommentFileSizeAuthor
#6 338184-dont-avoid-serialize.patch3.53 KBdamien tournoud
#1 338184-dont-avoid-serialize.patch3.14 KBdamien tournoud

Comments

damien tournoud’s picture

damien tournoud commented
Status: Active » Needs review
StatusFileSize
new 338184-dont-avoid-serialize.patch3.14 KB

Simple enough patch.

Anonymous’s picture

Anonymous (not verified) commented

care to share the tests/data that show this is more memory efficient? i'm totally in favour of this patch if it can be shown to be even equal in memory use, as it simplifies the code.

damien tournoud’s picture

damien tournoud commented

Test case: http://drupalbin.com/4152

$ php5 mem-test.php 1
109236
$ php5 mem-test.php 2
176480

Calling serialize() directly save about 40% of the memory used in the serialization process.

There is no doubt that it is also more efficient CPU-wise (C code always is...).

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Nice. That "belly-dancing" didn't look right to me when I reviewed that patch, so it'd be nice if we could use more standard PHP means for dealing with this.

Would like to see a few more benchmarks confirming this though.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Hm. Also I see a big - hunk for _registry_get_lookup_cache(), but not its setter... don't we need to remove both in favour of cache_get/cache_set?

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new 338184-dont-avoid-serialize.patch3.53 KB

_registry_set_lookup_cache() survived the first patch somehow :)

Anonymous’s picture

Anonymous (not verified) commented

DamZ, thanks for sharing that, it inspired me to look at this again.

my implementation just sucks, here's another that's much better memory wise: http://drupalbin.com/4156

avoiding the array in the middle, i get:

justin@starscream:~/code/drupal/7$ php memtest.php 1
109236
justin@starscream:~/code/drupal/7$ php memtest.php 2
48296

UPDATE: forgot to add, please confirm these findings, and then i'll submit a patch along these lines.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

My return is about the same, FWIW:

Macintosh-105:core webchick$ php moosh.php 1
97288
Macintosh-105:core webchick$ php moosh.php 2
48368

A reduction of the RAM footprint by basically half is yummy!

However, it seems to me that RAM consumption is only part of the battle here; we should do more comprehensive benchmarking to see if the C code in the native serialize() function helps us out at all on the overall performance.

Anonymous’s picture

Anonymous (not verified) commented

http://drupalbin.com/4158

when i run the code above like this:

justin@starscream:~/code/drupal/7$ time php cputest.php 1

i get serialize returning quicker in terms of cputime, but its barely measurable.

damien tournoud’s picture

damien tournoud commented

I can confirm those findings. But given that:
- If I just copy paste _registry_get_lookup_cache() / _registry_set_lookup_cache() in my small memory test file (without even using them) and display the $start value, I see an increase in the total memory used of about 7kB
- Having our own serialization function is quite inelegant
- We are only talking of saving about 50k of temporary memory (those will only be consumed during the function called, and freed just after)

So we add several loc, waste 7kB of permanent memory to save about 50k of temporary memory. My conclusion is that it does not worth it. And I suggest we fallback to #1.

Anonymous’s picture

Anonymous (not verified) commented

hehe, full circle.

i'm totally fine with #1, i only introduced the 'create string' stuff to appease others :-)

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Reviewed & tested by the community

simple patch, testbot is happy, this simplifies the registry, so RTBC.

damien tournoud’s picture

damien tournoud commented

This RTBC is of course of #6.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Great. The code makes MUCH more sense now, and testbot seems happy.

Committed to HEAD.

damien tournoud’s picture

damien tournoud commented
Status: Fixed » Reviewed & tested by the community

This was actually not committed.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Confirmed this was not committed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Ok, really committed this time. :P Thanks, not sure what happened there.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.