Include paths are resolved for every resource in a resource collection, instead of once per unique resource type

The PoC patch is somewhat related with #3018287: ResourceTypeRepository computes ResourceType value objects on *every request*.

There are two patches that:
- One that is standalone with the new service added to manage the static + persistent cache.
- One that depends on #3018287: ResourceTypeRepository computes ResourceType value objects on *every request* and is not adding the service ( I will be using this ).

The PoC follows the idea to maintain an always expanding list of valid include paths in cache for a resource type. Have the requested includes cross-checked against the cached ones and compute only the missing ones. The solution should cover point 2 and 3 from the issue summary.

XHProf runs for the change:

Single entity before:

Single entity after:

List 27 entities before:

List 27 entities after:

I will try to see what is possible to be done in point 1 at some point in near future (if anything).

I've tried to speed the issue in point 1 a bit I am hitting a wall, as it seems it can not be made faster for the cold cache scenarios and this is purely on the computational complexity of the problem at hand.

I have the following includes added through jsonapi_defailts.

field_image
field_image.field_media_image
field_content
field_content.field_custom_content
field_content.field_custom_content.field_image
field_content.field_custom_content.field_image.field_media_image
field_content.field_custom_content.field_image.field_media_provider_api_image
field_content.field_news
field_content.field_news.field_image
field_content.field_news.field_image.field_media_image
field_content.field_news.field_image.field_media_provider_api_image
field_content.field_gallery
field_content.field_person
field_content.field_person.field_image
field_content.field_person.field_image.field_media_image
field_content.field_person.field_image.field_media_provider_api_image
field_content.field_person.field_phone_number
field_content.field_files
field_content.field_files.field_media_file
field_content.field_products
field_content.field_products.field_product
field_content.field_left_video
field_content.field_left_image
field_content.field_left_image.field_media_image
field_content.field_left_image.field_media_provider_api_image
field_content.field_right_video
field_content.field_right_image
field_content.field_right_image.field_media_image
field_content.field_right_image.field_media_provider_api_image
field_content.field_list_topics
field_content.field_list_topics.field_topics
field_content.field_list_topics.field_topics.field_image
field_content.field_list_topics.field_topics.field_image.field_media_image
field_content.field_list_topics.field_topics.field_image.field_media_provider_api_image
field_content.field_list_topics.field_topics.field_image_icon
field_content.field_list_topics.field_topics.field_image_icon.field_media_image
field_content.field_list_topics.field_topics.field_image_icon.field_media_provider_api_image
field_content.field_questions
field_content.field_questions.field_image
field_content.field_questions.field_image.field_media_image
field_content.field_questions.field_image.field_media_provider_api_image
field_content.field_media_image
field_content.field_media_image.field_media_image
field_content.field_media_image.field_media_provider_api_image
field_content.field_media_video
field_content.field_form
field_content.field_articles
field_content.field_articles.field_image
field_content.field_articles.field_image.field_media_image
field_content.field_articles.field_image.field_media_provider_api_image
field_content.field_articles.field_media_image
field_content.field_articles.field_media_image.field_media_image
field_content.field_articles.field_media_image.field_media_provider_api_image
field_content.field_articles.field_article
field_content.field_articles.field_article.field_image
field_content.field_articles.field_article.field_image.field_media_image
field_content.field_articles.field_article.field_image.field_media_provider_api_image
field_content.field_articles.field_product
field_content.field_topics
field_content.field_topics.field_image
field_content.field_topics.field_image.field_media_image
field_content.field_topics.field_image.field_media_provider_api_image
field_content.field_topics.field_image_icon
field_content.field_topics.field_image_icon.field_media_image
field_content.field_topics.field_image_icon.field_media_provider_api_image
field_content.field_left_column
field_content.field_left_column.field_questions
field_content.field_left_column.field_media_image
field_content.field_left_column.field_media_image.field_media_image
field_content.field_left_column.field_media_image.field_media_provider_api_image
field_content.field_left_column.field_media_video
field_content.field_left_column.field_form
field_content.field_center_column
field_content.field_center_column.field_media_image
field_content.field_center_column.field_media_image.field_media_image
field_content.field_center_column.field_media_image.field_media_provider_api_image
field_content.field_center_column.field_media_video
field_content.field_right_column
field_content.field_right_column.field_questions
field_content.field_right_column.field_media_image
field_content.field_right_column.field_media_image.field_media_image
field_content.field_right_column.field_media_image.field_media_provider_api_image
field_content.field_right_column.field_media_video
field_content.field_right_column.field_form

The problem comes from the fact that field_content is a paragraph reference field and it is referencing about 40 different bundles in there and the loop that goes and checks for connection in each of them, in FieldResolver::resolveInternalIncludePath

The only way I can see to currently optimize the cold cache scenario, that's currently running for half a second, is to reduce the list size of includes you can see above.

I see 2 options:
1. Renaming and tuning, so including 1 thing can actually generate 2 iternal includes. For example:

field_content.field_right_column.field_media_image.field_media_image
field_content.field_right_column.field_media_image.field_media_provider_api_image

Through jsonapi_extras - rename the field in the end to a common public name:
field_media_provider_api_image -> field_media_image

and then include only one line for that:

field_content.field_right_column.field_media_image.field_media_image

This way defaults will pass on half the items for inclusion and jsonapi will internally load 2 things on one iteration instead of looping 2 includes to load 1 thing each. Could have been nicer to know this sooner :D. In my case this re-configuration should speed things around 25-30% (hooray)

Is it possible at the moment in jsonapi to have...

field_content.field_custom_content.field_image.field_media_image

... interpreted as

field_content
field_content.field_custom_content
field_content.field_custom_content.field_image
field_content.field_custom_content.field_image.field_media_image

From researching this, i see the overhead is mostly coming from the fact we are checking the include paths 1 by 1 and in a DFS manner. As a result any fields that have many possible related entities are processed multitude of times, returning a success only 1-2 out of 40 iterations.

In my case this is paragraph reference field that points to ~40 different types and I have ~80 includes that point to something through it. As a result there are ~40 * ~80 = ~2400 edges for traversal and it is going to get slower with more elements added to the system (in theory and practice). This can be mapped to the function calls count I see in the profiler for FIeldResolver::resolveInternalIncludePath()

We already have a utility to build a tree in IncludeResolver::buildTree(), so lets build the tree first from the requested include paths and have a BFS implemented on it to validate it and build the internal tree / list of paths from it in bulk.

The positives here are that we will be able to iterate over the nodes and traverse the edges in bulk. This way it will handle all paragraphs in bulk only 1 time, resulting in let's say 20 successes out of 40. Ideally the complexity for traversal should be based on the nodes in the relationships graph (about 50 in my case). Performance degradation should scale based on nodes in the graph linearly, compared to multiplication of edges and includes count (that increases with entities in the system).

I propose a new method FIeldResolver::resolveInternalIncludeTree() or FIeldResolver::resolveInternalIncludePaths() to be implemented based on the above description and see the change...

I suspect this should be able to drop some 3-4x+ the execution time for cold cache scenarios on point 1 for cases as complex as mine.

Points 2 and 3 are already alleviating the pain A LOT, so I am keeping the issue in needs review. The amount of work needed on point 1 will likely make it deserving it's own issue... Will see...

After some more experimentation and tweaks...

The biggest overhead in that method is the generation of the error messages :(.
Just removing that it, by returning empty lists and not throwing exceptions accelerated it like 10-11x times. (450ms -> 40ms) for the whole validation tree.

I have a PoC implementation that behaves 99% the same based on the BFS solution described above that runs (with no exceptions for 5-6ms).

Here is the PoC code for it. Note that there is no error handling in there...

  public static function resolveInternalIncludeTree(ResourceType $resource_type, $includesTree) {
    $result = [];
    $lookup_list = [];

    $fill_lookup_list = function ($branch, ResourceType $resource_type, $path) use (&$lookup_list) {
      foreach (array_keys($branch) as $name) {
        $internal_field_name = $resource_type->getInternalName($name);
        if (!$resource_type->hasField($internal_field_name)) {
          continue;
        }

        $new_path = array_values($path);
        $new_path[] = $internal_field_name;

        $lookup_list[] = [
          'public_name' => $name,
          'resource_type' => $resource_type,
          'string_path' => implode('.', $new_path),
          'path' => $new_path,
          'branch' => $branch,
        ];
      }
    };

    $fill_lookup_list($includesTree, $resource_type, []);

    while($lookup_list) {
      $item = array_shift($lookup_list);

      $next_branch = $item['branch'][$item['public_name']];
      foreach ($item['resource_type']->getRelatableResourceTypesByField($item['public_name']) as $related_resource_type) {
        $fill_lookup_list($next_branch, $related_resource_type, $item['path']);
      }

      if (!isset($result[$item['string_path']])) {
        $result[$item['string_path']] = $item['path'];
      }
    }

    return array_values($result);
  }

... and now using that in IncludeResolver like so:

# Used to reduce duplicated things from the input...
$public_tree = static::buildTree($paths);
$internal_paths_from_tree = FieldResolver::resolveInternalIncludeTree($resource_type, $public_tree);

The only functional difference is that I am adding intermediary includes. If you have includes list passed as:

relation1.relation2.relation3

The BFS solution will generate the following things if all is correct:

relation1
relation1.relation2
relation1.relation2.relation3

This for me has the hidden benefit that actually configuring the includes on the resources in jsonapi_extras or clients gets easier to maintain in the long run, as you need to add only paths to leaf resources and not all of the sub-paths in there (like the example some comments above).

I am currently working on the errors handling in the method, but as I see it, it needs to be made in a scalable way. Current implementation that is causing 90%+ overhead is not acceptable.

Here is a new patch that is implementing point 1.

Currently it is handling the cases for invalid includes passed in, but this is without the fancy messages like before.
I am expecting tests to fail because of exactly that :( if at all.

No time to tweak test at the moment. If the direction is OK, I will update them as well.

I will provide cold cache benchmarks as well shortly.
Teaser - currently this is running in 5-10 ms consistently in cold caches for a single term for the same list of includes.3

Functionally everything is the same except:
- The exceptions... They are now handled in the IncludeResolver::resolveInternalIncludePaths
- Valid sub-paths are always added to the result.

Here is the old cold cache scenario for a single term entity (repost from #3).

Here is the promised benchmarks for cold cache scenario 8ms :D!

At this point it's debatable whether we will need the persistent cache on top of this, but in my case it shows a measurable speed-up of at least 5-6 ms per call.

On top of all that there is still a new method that will likely need tests...
Leaving as needs review. (PARTY)

After sleeping on it for a couple of days - the only reason for the need of static cache in issue 3 is that the code was executed multiple times. Essentially in a simple loop. If we move the code to work on the first iteration only, we drop the need for static cache.

On that train of thought, this is only triggered on a per root entity cases.
If we want to have a persistent cache in here, we can move it's storage on the resource configuration objects directly. Have it lazy initialized as it currently is and stored on the value objects we already have. This way no additional cache management overhead will be introduced

If we consider that this is now fast-enough - 8ms on each request (that has that many includes defined, this can go without the need for persistent cache either.

Some tuning internally on the method can shed some of the internal overhead as well - the de-duplication logic. It is not that needed as in the end we will be creating a tree out of the result set either way. So it will be duplicated at that point in a much more efficient manner.

I will be implementing a new patch that aims at dropping the need for a cache layer fully at the cost of around 5-10 ms per request in my case.

If we then decide the cache is needed. It can be added as a follow-up. This will resolve one of the open TODOs I currently have in there - the cache tags management that are currently hard-coded and taken from jsonapi and jsonapi_extras.

Here is the patch from solution proposed in #14.
- Generate and validate the includes list only one time during a request.
- Optimized algorithm - currently running for 8 ms in my case. (same as #12)
- No cache layers.

I consider this commitable.

Critical task based on the link you've provided:

- Over ~100ms or more savings with cold caches
- Gets measurably worse with lots of contrib modules or large data sets (e.g. non-indexed queries) and would have to be deferred to a minor version

This is not a bug - it works correctly (but slow).
I really do not care for the severity, as long as it's fixed :)

I have some other MISC questions, that cropped up while debugging this.

All the methods I've had to change were static.
If they were on a service this could have been possible to decorate and be tuned from outside.
Are the static methods intentional to make it hard to customize as possible?

How about the errors?
Is it OK that we are now just listing all the invalid include paths in bulk?
- I think it is, as now only 1 exception is generated and only when there are invalid includes passed in. This saves a huge overhead.
- All invalid includes are shown in absolute manner so it should be fairly easy to reason the connections. The only regression compared to the solution present in the module now is that we are first not giving guidelines to what piece of the path is incorrect, but that it as a whole is such.

No problem, I have a patch that I can live with :). I can wait.

#24

1. You can reproduce with Drupal core in the same way. Just replace paragraphs with something else for example node types or block types. Have a field that references 40 different block types and some of them need to have to media entities sprinkled in there.
2. There are 5 node types, 6 media types, and ~40 paragraph types.
   Paragraphs have 2 logical sub-types: layout ones and data ones.
   Note that this are not Drupal bundles, it's just how we treat them.
   - Data ones are only showing content.
   - Layout ones are having a multi-valued reference field to data paragraphs. They can reference most / all of the data paragraphs. We run a default like a single column and the layout paragraphs allow us to have 2 and 3 column grids on a per paragraph entry.

   Overall the content follows this structure:
   Node > data paragraph -> Media -> File.
   Node > Layout paragraph > data paragraph -> Media -> File.
   Node > Layout paragraph > data paragraph -> Node -> media -> file

3. Well for 85 valid include paths (as a result) I had something like 2k exceptions thrown somewhere in the recursive function and cached somewhere in there as well. Based on my experiment in #11, just removing all exceptions-related code resulted in ~90% speed-up in the original code.
4. The meta-data I am talking about is: List of valid include paths for the particular entity resource. If we have that available before-hand, this can be reduced to a simple array_diff. This is why I was focused on cache and cache merging initially in the patches before #15.
5. I am not arguing there, but it seems it can go faster ;). We already discussing alternatives internally how to get away from this, so we can simplify the overall request complexity, but we are hitting walls :(. This might be a good candidate for a support issue to discuss / or in any chat channel (slack / IRC). Just to not side-track this issue.
6. Yes, and there comes the overhead of managing the cache tags on the cache item. The responsibility is split between jsonapi and jsonapi_extras and encapsulated in resource type repository. This is why I've moved to drop the cache in later patches, as it simplifies the problem space a lot.

#25

Already simplified includes list today. As I am running with the last patch (#15 now), it's not getting much faster, if at all, from the reduced includes in the validation step. I've managed to shed some of the deepest includes and this helps to an extent on the loading step after that. A separate issue that I will maybe bother you in near future... :]

Thanks for the positive feedback and I am waiting any further comments / questions :).

This is essentially same as the break; approach.

I will test this today...

We've already reduced the include lines to the bare minimum to keep same functionality, so I suspect the overhead to be minimal now. We dropped from 80+ default include lines to something like ~40 on all 5 content types so I suspect there will be a lowered overhead with this.

I can say it is resolving the overhead for the multiple items case to a big extent.

With my current includes setup that has drastically improved compared to earlier benchmarks the recursive overhead has dropped from 500ms to ~200ms. Note all the XHProf things you see above are with xdebug and xhprof running. Though on the dev servers we were having 11-12 seconds of response times even without the 2 extensions...

The changes in the includes are that the following 2 have merged into only *.field_media_image, after the lattest commit in JSON:API.

*.field_media_image
*.field_media_provider_api_image

And the rest is just skipping the intermediary paths that were duplication like:

path1,path1.path2,path1.path2.path3 -> path1.path2.path3

Current list of includes (functionally equivalent):

field_image.field_media_image
field_content
field_content.field_custom_content.field_image.field_media_image
field_content.field_news.field_image.field_media_image
field_content.field_gallery
field_content.field_person.field_image.field_media_image
field_content.field_person.field_phone_number
field_content.field_files.field_media_file
field_content.field_products.field_product
field_content.field_left_video
field_content.field_left_image.field_media_image
field_content.field_right_video
field_content.field_right_image.field_media_image
field_content.field_list_topics.field_topics.field_image.field_media_image
field_content.field_list_topics.field_topics.field_image_icon.field_media_image
field_content.field_questions.field_image.field_media_image
field_content.field_media_image.field_media_image
field_content.field_media_video
field_content.field_form
field_content.field_articles.field_image.field_media_image
field_content.field_articles.field_media_image.field_media_image
field_content.field_articles.field_article.field_image.field_media_image
field_content.field_articles.field_product
field_content.field_topics.field_image.field_media_image
field_content.field_topics.field_image_icon.field_media_image
field_content.field_left_column.field_questions
field_content.field_left_column.field_media_image.field_media_image
field_content.field_left_column.field_media_video
field_content.field_left_column.field_form
field_content.field_center_column.field_media_image.field_media_image
field_content.field_center_column.field_media_video
field_content.field_right_column.field_questions
field_content.field_right_column.field_media_image.field_media_image
field_content.field_right_column.field_media_video
field_content.field_right_column.field_form

This is why I've made a black box test between the 2 approaches with xdebug and xhprof both disabled.
As both are equally solving the multiple entities handling in terms of call it only once (#28 is functionally correct...).

This is why I am testing only a single entity as essentially that's the same for this patch and reduce the normalization overhead.
All tests in this issue are done with no page dynamic or render cache.

Here are the benchmarks against the 2 (#15 & #28) patches on my local environment.
This is full request tests (AB) against a single entity. I am comparting Wim's patch from #28 and mine from #15.

There is a consistent 25-30% speed-up in the average / mean case and about 80-100% change in the worst case.

Wim's patch in #28 (ab -n 100 -c 1 http://HOSTNAME:81/api/taxonomy_term/sections/77d82ef5-3e45-4eb3-8490-85...)

Document Path:          /api/taxonomy_term/sections/77d82ef5-3e45-4eb3-8490-859cd950e79e
Document Length:        1028 bytes

Concurrency Level:      1
Time taken for tests:   12.677 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      192900 bytes
HTML transferred:       102800 bytes
Requests per second:    7.89 [#/sec] (mean)
Time per request:       126.772 [ms] (mean)
Time per request:       126.772 [ms] (mean, across all concurrent requests)
Transfer rate:          14.86 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.1      0       1
Processing:   117  127   9.9    126     212
Waiting:      117  126   9.9    126     212
Total:        117  127   9.9    126     213

Percentage of the requests served within a certain time (ms)
  50%    126
  66%    128
  75%    129
  80%    130
  90%    134
  95%    135
  98%    138
  99%    213
 100%    213 (longest request)

Patch in #15 (ab -n 100 -c 1 http://HOSTNAME:81/api/taxonomy_term/sections/77d82ef5-3e45-4eb3-8490-85...)

Document Path:          /api/taxonomy_term/sections/77d82ef5-3e45-4eb3-8490-859cd950e79e
Document Length:        1028 bytes

Concurrency Level:      1
Time taken for tests:   9.903 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      192900 bytes
HTML transferred:       102800 bytes
Requests per second:    10.10 [#/sec] (mean)
Time per request:       99.034 [ms] (mean)
Time per request:       99.034 [ms] (mean, across all concurrent requests)
Transfer rate:          19.02 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    91   99   5.8     98     135
Waiting:       91   99   5.8     98     135
Total:         91   99   5.8     98     135

Percentage of the requests served within a certain time (ms)
  50%     98
  66%    101
  75%    102
  80%    102
  90%    104
  95%    109
  98%    111
  99%    135
 100%    135 (longest request)

Here is the patch from 15 incorporating all the feedback from @Wim. (#28 + some code styles mentioned earlier).

This is the point where we need to decide are we introducing persistent cache on this or not. If we do - algorithm to implement the validation is not that important, as we will be validating only the non-cached paths and the DFS will be efficient to do that as it is. In that case, #3018287: ResourceTypeRepository computes ResourceType value objects on *every request* becomes a blocker.

If we steer away from the persistent cache, this might hit back in multi-resource type collections, as the overhead will be multiplied per resource type and we will be back here :).

This test with the AB was against a single entity, so patch in #28 should not add any meaningful overhead.

I will provide the same AB for a list of entities some time today.

Here are the promised benchmarks.
No page, dynamic or render caches.
No xhprof or xdebug running.

No patch against list with 27 entities.

$ ab -n 100 -c 1 http://HOSTNAME:81/api/taxonomy_term/sections
Document Path:          /api/taxonomy_term/sections
Document Length:        34457 bytes

Concurrency Level:      1
Time taken for tests:   115.891 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      3610600 bytes
HTML transferred:       3445700 bytes
Requests per second:    0.86 [#/sec] (mean)
Time per request:       1158.913 [ms] (mean)
Time per request:       1158.913 [ms] (mean, across all concurrent requests)
Transfer rate:          30.42 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:  1070 1159  57.8   1147    1332
Waiting:     1067 1157  57.8   1146    1330
Total:       1070 1159  57.8   1148    1333

Percentage of the requests served within a certain time (ms)
  50%   1148
  66%   1175
  75%   1189
  80%   1209
  90%   1255
  95%   1274
  98%   1297
  99%   1333
 100%   1333 (longest request)

Wim's patch from #28. Same endpoint. Same data.

$ ab -n 100 -c 1 http://HOSTNAME:81/api/taxonomy_term/sections
Document Path:          /api/taxonomy_term/sections
Document Length:        34457 bytes

Concurrency Level:      1
Time taken for tests:   48.563 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      3610600 bytes
HTML transferred:       3445700 bytes
Requests per second:    2.06 [#/sec] (mean)
Time per request:       485.635 [ms] (mean)
Time per request:       485.635 [ms] (mean, across all concurrent requests)
Transfer rate:          72.61 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   443  485  24.5    482     563
Waiting:      441  484  24.5    480     561
Total:        443  486  24.5    482     563

Percentage of the requests served within a certain time (ms)
  50%    482
  66%    490
  75%    494
  80%    503
  90%    520
  95%    537
  98%    562
  99%    563
 100%    563 (longest request)

My patch from #36 (BFS + Wim's changes in #28).

$ ab -n 100 -c 1 http://HOSTNAME:81/api/taxonomy_term/sections
Document Path:          /api/taxonomy_term/sections
Document Length:        34457 bytes

Concurrency Level:      1
Time taken for tests:   47.158 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      3610600 bytes
HTML transferred:       3445700 bytes
Requests per second:    2.12 [#/sec] (mean)
Time per request:       471.578 [ms] (mean)
Time per request:       471.578 [ms] (mean, across all concurrent requests)
Transfer rate:          74.77 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.1      0       1
Processing:   429  471  28.8    465     588
Waiting:      428  470  28.8    464     587
Total:        429  472  28.8    465     588

Percentage of the requests served within a certain time (ms)
  50%    465
  66%    472
  75%    479
  80%    486
  90%    515
  95%    538
  98%    566
  99%    588
 100%    588 (longest request)

Some thoughts...

1. There is a lot of normalization overhead here but we can still see the ~15-20 ms speed-up between #28 and #36 as seen in #35.
2. #28 will degrade further with non-optimal includes passed in for validation.

#40 I am +1 for 28 getting in, as it's a step in the right direction.

My only concern was that because I lacked knowledge on the jsonapi module and how includes worked I had a non-optimal configuration and I suspect this might be the case for many more people (not just me).

The main difference (ignore the 3% that may as well be 0) in there is that #36 does not degrade with non-optimal configs passed in for validation. If that is OK as a compromise on the module I can live with it.

At that point I will think of a follow-up to add persistent cache on the resource types for the valid list of includes and eliminate this overhead fully, once resource type definitions are loaded from cache. Intersect against the valid list and compute only the new ones. I think this can be a follow-up.

Ping.

Let's get this in, so we can start on caching it (eventually).

Here is a re-roll of #28.

I've been using it for the last month on over 60 sites without issues, and it helps a ton.

Any further speed-ups (based on cache) can be done in follow-ups.

If the tests pass, I'll be marking as RTBC as per recommendation from #43.

Should I Re-Roll this against core?

Moving to core's issue queue.
Rebased patch coming shortly.

Here is the new patch against core.
It's the same as #45 / #28.

Here is a re-rolled version of the patch, implementing the proposal.
Note that there are not many gains expected in there, as there is already the internally cached property for the value.

Note that there are not many gains expected in there, as there is already the internally cached property for the value

This is only for return from #49 - the fact that we are sparing a single php function call with the latest patch to a function that has a lazy initialized property in it, and storing the result in a local variable is not that big of a performance win (if any).

I do not expect to have any visible change compared to #48. No time to do benchmarks on this now.

Back to RTBC, as you can pick #48 or #50, as for me they are essentially the same. Sorry for the confusion my comment triggered. The overhead that is spared by both of them are worth the commit.

Awesome! :D