Don't avoid serialize() in the registry
Damien Tournoud - November 23, 2008 - 21:42
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
#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.
#1
Simple enough patch.
#2
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.
#3
Test case: http://drupalbin.com/4152
$ php5 mem-test.php 1109236
$ 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...).
#4
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.
#5
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?
#6
_registry_set_lookup_cache() survived the first patch somehow :)
#7
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 1109236
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.
#8
My return is about the same, FWIW:
Macintosh-105:core webchick$ php moosh.php 197288
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.
#9
http://drupalbin.com/4158
when i run the code above like this:
justin@starscream:~/code/drupal/7$ time php cputest.php 1i get serialize returning quicker in terms of cputime, but its barely measurable.
#10
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.
#11
hehe, full circle.
i'm totally fine with #1, i only introduced the 'create string' stuff to appease others :-)
#12
simple patch, testbot is happy, this simplifies the registry, so RTBC.
#13
This RTBC is of course of #6.
#14
Great. The code makes MUCH more sense now, and testbot seems happy.
Committed to HEAD.
#15
This was actually not committed.
#16
Confirmed this was not committed.
#17
Ok, really committed this time. :P Thanks, not sure what happened there.
#18
Automatically closed -- issue fixed for two weeks with no activity.