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.
I figured out it's possible to write to $this->$name without issuing a __set() call from inside __set(). This allows for a "magic-magic-static-cache". I've run some quick benchmark:
With my branch:
17009.16ms for 1000000 runs.
0.01700916ms per run
With the attached patch applied:
12672.81ms for 1000000 runs.
0.01267281ms per run
So the property access improve from 17µs to 12,67µs on my machine. I'd have expected the difference to be bigger though. :/
Benchmarked code:
$iterations = 1000000;
timer_start('test');
for ($i = 0; $i < $iterations; $i++) {
$entity->user_id->value = 1;
}
timer_stop('test');
echo timer_read('test')."ms for $iterations runs. \n";
echo (timer_read('test') / $iterations) ."ms per run";
Comment | File | Size | Author |
---|---|---|---|
d8_property_magic_performance.patch | 1.48 KB | fago | |
Comments
Comment #1
fagoI figured out that the magic magic cache only applies to the first level in my quick-patch, so the not cached lower level influence my results. Re-run it just with the first level:
With patch:
Without patch.
So that's an improvement, more than 10x times faster. :)
Comment #2
fagohttp://vdev.local/drupal-8/node/3?page=3&q=node/3
With having the latest patch from #1778178: Convert comments to the new Entity Field API applied I ran "ab -c1 -n100" on page http://vdev.local/drupal-8/node/3?page=3 (showing 50 comments).
So here is a comparison of the impact of the overall page request:
With the patch applied (branch property-comment)
and here without the patch (regular 8.x brach)
So the version without the changes appears to a bit faster, however differences are below the standard derivation. Running it multiple times gave similar results though.
However, it should be noted that the current implement of comment rendering is everything else than efficient as it disables and enables the entity property API compatibilityMode multiple times (each time a field api attacher is called) per rendering a comment, whereas enable the mode means copying around property values...
Comment #3
dixon_Here are my manual tests with Devel output.
The numbers are the average of 20 page requests on each setup on two separate sites, installed on the same local virtual server. All tests are done as user ID 1.
Ubuntu 12.04
PHP 5.3.10
MySQL 5.5.4
Comment patch, without any comments
D8 core, without any comments
Comment patch, with 50 comments
D8 core, with 50 comments
Comment #4
dixon_Same setup with
ab -c1 -n100
as anonymous user. Page cache turned off.Comment patch, with 50 comments
D8 core, with 50 comments
Comment #5
dixon_Same setup with
ab -c1 -n100 -C SESS...=...
as user ID 1.Comment patch, 50 comments
D8 core, with 50 comments
Comment #6
catchIt'd be good to see some profiling output from xhprof in terms of function calls etc., I don't think ab or devel is going to be useful here.
Comment #7
fagoI guess this should be moved to the core queue now.
Comment #8
fagoadding tag
Comment #9
fagoComment #10
yched CreditAttribution: yched commentedOne unfortunate thing is that the "New Field API" basically destroys the efforts that went into #1040790: _field_info_collate_fields() memory usage, since the new field_entity_field_info() (called from DataBaseStorageController::getFieldDefinitions()) calls field_info_instances(), which loads all instances in memory.
The plan in D8 is to deprecate field_info_fields() / field_info_instances() (the "give me all" variant, only keep field_info_instances($entity_type, $bundle)), because they are performance killers. They are still here after #1040790: _field_info_collate_fields() memory usage went in because the goal there was to keep an easy D7 backport.
You should only access instances within a given $entity_type and $bundle - that, we can keep reasonably performant.
Meaning, we shouldn't allow getFieldDefinitions($constrants), but only getFieldDefinitions($entity_type, $bundle).
For the cases where you do need to have an overview of all existing fields and instances, we now have a much lighter field_info_field_map(), which seems to somewhat duplicate DatabaseStorageController::$entityFieldInfo.
In short, this and #1040790: _field_info_collate_fields() memory usage were worked on in parallel and ignorant of each other, and landed about the same time. They will need to be merged somehow, but I'm not familiar enough with the new API to have a clear notion of how. Also, I'm afk for the next month or so :-/
Comment #11
amateescu CreditAttribution: amateescu commentedThis is at least major.
Comment #12
fagoWell, this runs per entity-type, so it's not that bad.
Makes sense. If the bundle is ommitted, just the base-fields of an entity should be returned.
Looks like it follows the same idea, yep. Once we only have entity base fields and configured fields, the hook is mostly unnecessary - except for dynamic fields. I guess we have to look into optimizing this again then, depending on how it turns out to be exactly in the end. Maybe the cache could be moved to the upper level (= on the entity field api level) then and make configurable fields use it as well.
Comment #13
yched CreditAttribution: yched commentedNo, it is a problem. field_info_instances($entity_type) is as expensive as field_info_instances(). It's only field_info_instances($entity_type, $bundle) that got optimized.
Comment #14
chx CreditAttribution: chx commentedComment #15
Berdir@chx: Note that #1778178: Convert comments to the new Entity Field API contains a large amount of changes related to performance, so you probably want to check the patch there and possibly help to finalize that so that we can finally convert an actual entity type to NG and start working on the others. I'd suggest to postpone this issue on that one and re-evaluate after it went in.
Comment #16
chx CreditAttribution: chx commentedComment #17
BerdirBack to active, comments got in a while ago.
Comment #18
chx CreditAttribution: chx commentedLet's not wait on me :) if I get to it I will re-assign.
Comment #19
fagoProblem with writing a field object to a public $entity->field property is that someone could replace it with a new $field object, what could lead to quite some breakage. So given this doesn't present a big boost, I don't think we should do this.
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedI agree with at least holding off on this until it can be shown to have a big boost. I think there are some situations where it will, especially if also applied at the item level (
->value
).Comment #21
tim.plunkettSeems like it should be postponed or won't fix.
Comment #22
Fabianx CreditAttribution: Fabianx commentedLeaving as postponed, but adding "needs profiling" tag
Comment #23
Fabianx CreditAttribution: Fabianx commented.
Comment #24
moshe weitzman CreditAttribution: moshe weitzman commentedTwo months later. Should we unpostpone this? Close it?
Comment #25
Fabianx CreditAttribution: Fabianx commentedI think we should re-profile this to see what impact this has - if any. Also the benchmark of #0 might be misleading (if and I don't know that) debug() code was left in ...
Comment #26
Fabianx CreditAttribution: Fabianx commentedRe-titling for scope ...
Comment #27
moshe weitzman CreditAttribution: moshe weitzman commentedComment #28
mgiffordJust unassigning issues that haven't been developed for a bit in the D8 queue.