_field_info_collate_fields() memory usage

One approach could be splitting static caches about instances info by entity type - possibly bundles as well, but less sure about that.

I don't see how we can minimize the 'fields' info size, though. Fields are potentially used in any entity type.

Subscribing.

One of the reasons for creating drupal.org/project/properties is that the entity/field configuration is global and can become huge if you are trying to create large amount of bundles and fields.

Marked #1165826: field_info_fields cache workaround fail as duplicate.

Coming from #1165826 which still mot marked as duplicate, I think both should be fixed

field_info_fields() also CPU-eater because iterates all fields mostly without reason, suppose we need to store deleted items in separate cache

file_get_file_references() the only usage of field_info_fields() should be rewritten with more lightweight implementation of this

+1 to andypost #5 #1165826: field_info_fields cache workaround fail

here are xhprof on real project with

mysql> select count(0) AS `COUNT(*)` from `field_config_instance`;
+----------+
| COUNT(*) |
+----------+
|    15968 |
+----------+
1 row in set (0.02 sec)

all the 15968 created by commerce BTW
this run was recorded in xhprof without field_info_field cache bin

here are output with cached field_info_fields

the same database and codebase

Wow. For now, I'll just say that the current code is indeed in no way designed for 16.000 different instances. I don't think Drupal Commerce handles hundreds of different entity types, so I'd more likely suspect they are creating bundles like mad ?

If that # of instances is "legitimate", then caching per-entity-type will hardly cut it.

Pondering on the items 2 and 3 in @catch's #9 (I don't think I get item 2)

2yched

I don't think Drupal Commerce handles hundreds of different entity types, so I'd more likely suspect they are creating bundles like mad ?

this is possibly offtopic, but in that project there are 800+ types with 20-40 fields in it
duplication of fields was minimized via using same fields(bundles) for different types(instances)
with those huge fields database (50GB +) I`ve got a 10 seconds trouble - any page that used fields displayed in 10 seconds 8(

If You are interesting in it here are few issues that become real when there are more than 10K bundles(fields)
#1173108: Permissions page performance + #1165826: field_info_fields cache workaround fail + #435694: >1M data writes incompatible with memcached (memcached -I 32M -m32) + #1131980: commerce_product_ui_menu performance fail

2catch
We have a thin place in fieldapi when getting info about all the fields for getting fields for a single entity or a bundle...
We need be more specific in that place - without caching all the data but caching a data at different levels
Storing field_info_fields in database is like a bomb - serialized 10M blob(~500 types) unserialized 100M
FE
xxx_get_all_fields - returns an array or an object with only field names at all (1000 fields will be few KB in a single cache blob)
xxx_get_field_info($field_id) - returns particulary the same as _field_info_collate_fields() but only for a single field (a small cache blob too)

@podarok: you are trying to bend Drupal 7 outside of its design specifications. Drupal 7 assumes that *entity types, bundles and fields are configuration*, which means that they can be (and are) loaded into memory at every request. A system that scales to a large number of fields would be designed completely differently, and would likely have worse performance in the simple cases.

2Damien Tournoud
nope, I`m using really standard core D7 config.

I just use a simple online store configuration with a 800 product types, created in commerce with a ~30 fields for each

WIth xhprof I`ll found a problem with memory in _field_info_collate_fields() and here I am 8)
As described at #1 - memory trouble is even at small D7 installations and I thought my configuration will be helpfull for this issue
FieldAPI really good for storing and displaing such huge number of fields with a memory trouble for _field_info_collate_fields() workaround

If we fix it - D7 would be a good solution for enterprise systems like electronic, car parts catalogs (10K+ types with 50+ fields each) or any other

@podarok

You misunderstood Damien. He meant that you are using Drupal 7 in a way for which it was not designed to be used.

Have you looked the Properties project that I linked above? ( http://drupal.org/project/properties ). One of the main reasons for creating that was that you practically *can not* create hundreds of field instances. It works around that by being a single field that has an more or less unlimited amount of key/value pairs, grouped in categories. It is limited to strings as values currently, so it doesn't work for anything but it allows to create any number of pre-defined templates, which are *not* read into memory.

The fields system in D7 simply has not been designed for what you are doing (creating 800 product types). I don't think that can simply "be fixed", certainly not in D7, as that requires architectural changes to really solve this properly.

@podarok : everyone agrees there's a data size (memory + CPU) issue with _field_info_collate_fields(), and the discussion in this thread will hopefully come to enhance that (with the particular requirement that a solution for D7 needs to avoid breaking the field_info_*() API).

However, as for any performance and optimization issue, there needs to be a target as to what range of data size we should support with a "reasonable" performance, so that we can reason on the necessary tradeoffs. "800 bundles & 16.000 field instances" seems out of reach (and scope), and definitely not the main case we should try to optimize for.

2catch

if you are using exactly the same set of fields for two different bundles of the same entity type

nope
Every content type (bundle) has its own set of fields, but some fields are common.
but anyway... it is possibly offtopic...

re @catch #16 :
I'd say the assumptions and the approach look correct.

Below is a a small audit of the use of the field_info_*() API functions (at least those that fetch from _field_info_collate_fields()), with core + a couple 'big' contrib suspects (Views, Ctools, OG, Field permissions, Field collection, OG, Date, References, Relation)
Disclaimer, though : the Entity API module does advanced (and convoluted) stuff with fields (entity metadata et al), I'm not familiar enough with it to evaluate the performance impact over there. We wight want to bring @fago in here.
I also didn't dare looking through Drupal Commerce :-/

field_info_field($field_name) / field_info_field_by_id($field_id)
Those are generally called after knowing an instance (sometimes collected higher in the callstack, no problem). So having instances pre-fetch their fields sounds like a good pattern.
For other cases (code inquiring about a specific field, e.g "field settings page for field_foo", tests, or views field handlers, that generally deal with a field across bundles), that will mean a possible extra db hit, should be OK.

field_info_fields() / field_info_field_by_ids()
Then we have the "return all fields" functions : .
We'll need to load every field definition, but we should probably make sure those do not remain in the persistent "on-demand" cache (and possibly not in the static cache as well ?)
field_info_field_by_ids() is mainly used internally, and each time the code could use field_info_field_by_id($id) instead. We can keep the function around, but our current core use cases should be moved to field_info_field_by_id().

field_info_instance($entity_type, $field_name, $bundle_name)
Perfect match for filling the static and persistent "on-demand" caches, and pre-fetching the corresponding $fields. We can probably load all fields and instances for the whole bundle while we're at it.

field_info_instances($entity_type = NULL, $bundle_name = NULL)
That one is a bit special, because of the two optional params.
- field_info_instances(NULL, NULL), like field_info_fields(), needs to load all definitions, and should probably not keep anything in the persistent cache (nor in the static ? in which case there's no need to pre-fetch the corresponding $fields)
- Same for field_info_instances($entity_type, NULL) (very rare).
- field_info_instances($entity_type, $bundle_name), like field_info_instance(), is a perfect match.

Also : I'm leaving tomorrow for a whole month away from my coding env. Not completely AFK, but I won't be able to help much code-wide :-/.

Crossposting #1220168: Standardize information returned by _field_info_collate_fields() storage bins for consideration in whatever fix is implemented.

Started to look into this now that DrupalCacheArray is in.

I'm actually not sure how we should best leverage the tool here :
- One single DCA object for both fields and instances across all entity types and bundles (filled on demand for each separate entity_type+bundle) ? - means one single db cache entry, which can still theoretically grow huge and hit MySQL max_allowed_packet limit ?
- Two separate DCAs for fields and instances ?
- Different DCAs for each entity_type+bundle ?

I started to work on some code myself, not nearly finished, but I will try to post it later today.

Posting what I have. This is still largely rough code, unpolished and unfinished - notably, not tied with the actual field_info_*() functions yet.

The FieldInfoFieldCache class extends DrupalCacheArray for now, but uses a different logic around the persistent and static caches (one DCA but several cache entries, static data stored not only in $this->storage, and massaged before returning the accessed values), so that in the end it has little in common. At some point it might be clearer to drop the reference to DCA completely, not sure yet.

Guiding principles :
- We want separate cache entries for each bundle, to keep them to reasonable sizes. Only load / unserialize / keep in memory the data for the bundles used in the page. That's one cache_get() per bundle used on the page, I think this is a fair trade_off.
- For each bundle, the persistent cache stores the list of instances, and the list of corresponding fields. Bundles are populated 'as a whole' : all instances and fields for the bundle; I don't think it makes sense to have on-demand loading of specific fields or instances within a given bundle.
- The static per-bundle entries ($this->storage["$entity_type:$bundle"]) do not hold field definitions, but only a list of field ids, to avoid in-memory duplication of fields shared in several bundles. When the persistent cache entry for the bundle is loaded in memory (in offsetGet()), the field definitions are taken out and merged in a global list of fields ($this->fields_by_id)
- When per-bundle entries are accessed (offsetGet()) or persisted (__destruct()), the relevant field definitions are merged back into the returned value.

Subscribing.

For the record, I'm still working on this, but I've had limited core time recently. I'm currently working in #1305362: Helper issue for #1040790 to have a testbot playground for the work-in-progress patch without cluttering this issue.

Still a fair amount of code polish ahead when the tests over there come back green, though.

Still some polish needed, and a couple @todos, but we should be getting close.
I'll try to post a human-reviewable patch with a patch summary shortly.

Here's a green and almost final patch (still has a couple docs @todos).
I'll try to wrap a summary asap.

Interesting.

Is this still applicable for a D7 backport? The functions which are moved around are prefixed with an _, sure, but this is quite a huge change...

So much for "here's a green patch". For some reason patch #32 included a revert of my #1391384: Wrong static clear on taxonomy_vocabulary_save() fix and thus failed the tests I added over there. That's called self-biting I guess...

Updated patch fixes that and clears a couple doc @todos.

@Berdir: yes, this is definitely done with a D7 backport as a target.
The changes on existing functions only involve a couple _internal_funcs(). For some of them (field_info_prepare_*()) I suspect there might be legitimate cases for calling them from contrib, but the D7 version of the patch can preserve BC implementations.

Wow, big patch! It will be great to see this get in. Somewhere in the middle I switched from reading the code to just doing a docs review, which follows.

1. +++ b/core/modules/field/field.info.incundefined
   @@ -6,6 +6,681 @@
   + * The FieldInfo class serves informations about the active field and instance
   + * definitions in the current runtime environment.
   

   This should be one line, 80 chars or less, and start with a third-person verb. Also, "information" is not pluralized. How about simply:

   Provides information about active field and instance definitions.

2. +++ b/core/modules/field/field.info.incundefined
   @@ -6,6 +6,681 @@
   + * It holds static and persistent caches of the collected data. The methods ¶
   + * retrieving all fields or all instances across all bundles do not read from ¶
   + * the caches (and do not populate the caches either). As such, they should be ¶
   + * used sparingly.
   

   Trailing whitespace at the end of these lines. Also, because of the above change, I'd start the function either with "Holds..." or "This class holds..."

3. +++ b/core/modules/field/field.info.incundefined
   @@ -6,6 +6,681 @@
   + * cache entries (the static cache ). Cache entries are loaded for
   

   Unneeded space before the parenthesis here. Also, I think we can fit more words on this line?

4. +++ b/core/modules/field/field.info.incundefined
   @@ -6,6 +6,681 @@
   +   * Lightweight map of fields across entity types and bundles,
   

   Typo here (ends with a comma instead of a period).

5. +++ b/core/modules/field/field.info.incundefined
   @@ -6,6 +6,681 @@
   +   * Information about the contents of bundles ($instance definitions and
   +   * extra fields), keyed by entity type and bundle name.
   

   We should try to word this such that it fits in one line.

6. +++ b/core/modules/field/field.info.incundefined
   @@ -6,6 +6,681 @@
   +   * Constructs a.FieldInfo object.
   

   I think there is a typo here (a period instead of a space).

7. +++ b/core/modules/field/field.info.incundefined
   @@ -6,6 +6,681 @@
   +  protected function persistentCacheGetBundleInfo($entity_type, $bundle) {
   +    if ($cached = cache('field')->get("bundle_info:$entity_type:$bundle")) {
   +      return $cached->data;
   +    }
   +  }
   +  ¶
   +  /**
   

   Trailing whitespace after this method.

8. +++ b/core/modules/field/field.info.incundefined
   @@ -6,6 +6,681 @@
   +   * This method does not read from nor populates the static and persistent
   +   * caches.
   ...
   +   * This method does not read from nor populates the static and persistent
   +   * caches.
   ...
   +   * This method does not read from nor populates the static and persistent
   +   * caches.
   

   These should read:
   "does not read from nor populate..."

9. +++ b/core/modules/field/field.info.incundefined
   @@ -6,6 +6,681 @@
   +   * Returns a field definition from a field id.
   ...
   +   *   The field id.
   

   "ID" should be uppercase in both these lines.

10. +++ b/core/modules/field/field.info.incundefined
    @@ -6,6 +6,681 @@
    +   * Ratrieves all active, non deleted instances definitions.
    

    "Non-deleted" should be hyphenated. Edit: Also, there's a typo ("Ratrieves").

11. +++ b/core/modules/field/field.info.incundefined
    @@ -6,6 +6,681 @@
    +   * @return
    +   *   If $entity_type is not set, return all instances keyed by entity type and
    +   *   bundle name. If $entity_type is set, return all instances for that entity
    +   *   type, keyed by bundle name.
    

    We can remove the word "return" here.

12. +++ b/core/modules/field/field.info.incundefined
    @@ -6,6 +6,681 @@
    +    // We need the field type for each instance. Rather than loading all field
    +    // definitions in memory, retrieve field types with an ad-hoc query.
    

    Do we really need to call the query "ad-hoc"? :P

13. +++ b/core/modules/field/field.info.incundefined
    @@ -6,6 +6,681 @@
    +  /**
    +   * @todo
    +   *
    +   * @param $entity_type
    +   *   The entity type.
    +   * @param $bundle
    +   *   The bundle name.
    +   * ¶
    +   * @return
    +   *   An array with the following key/value pairs:
    +   *   - 'instances': the list of instance definitions.
    +   *   - 'extra_fields': the list extra fields.
    +   *   The function also populates the correpsonding field definitions in the
    +   *   static cache.
    +   */
    

    1. Let's add summaries for all these @todo.
    2. There's a bit of trailing whitespace between the params and return value.
    3. We don't need quotes around the names of the array keys. Also, I think the "The" after the colons should be capitalized.
    4. Additionally, let's move the line about populating the cache up to a 2nd paragraph (after the summary).

14. +++ b/core/modules/field/field.info.incundefined
    @@ -6,6 +6,681 @@
    +      $this->staticCacheSetBundleInfo($entity_type, $bundle, $info);
    +      return $info;
    +    }
    +    ¶
    +    // Cache miss: collect the information from the database.
    

    Trailing whitespace here.

15. +++ b/core/modules/field/field.info.incundefined
    @@ -6,6 +6,681 @@
    +      // The persistent cache additionally contains the definitions of the
    +      // fields involved in the bundle.
    +      $cache = $info + array('fields' => array());
    +      foreach ($info['instances'] as $instance) {
    +        $cache['fields'][] = $this->staticCacheGetFieldbyId($instance['field_id']);
    +      }
    +      $this->persistentCacheSetBundleInfo($entity_type, $bundle, $cache);
    +    }
    +    ¶
    +    return $info;
    +  }
    

    Trailing whitespace just above the return value.

16. +++ b/core/modules/field/field.info.incundefined
    @@ -6,6 +6,681 @@
    +      // Fallback to default formatter if formatter type is not available.
    

    REALLY nitpicky point, but "Fall back" should be two words when it's a verb.

Phew! :)

Adding a novice tag in case someone wants to clean up the points listed above and write one-line summaries where there are currently @todo before others get back to it.

So I didn't mean to mark this NW actually. Conversation in IRC about how dreditor sets it to NW reminded me. :) Sorry.

Fixes @xjm's remarks in #37.

Some more refactoring.
Moving the static and persistent cache gets and sets to separate helper methods really helped make the code flow legible in the rest of the class, but the method names themselves sucked big time (staticCacheGetFieldbyId(), persistentCacheSetBundleInfo()...).

Factorized those into cacheGetX() / cacheSetY() methods, with CACHE_STATIC and CACHE_PERSISTENT class constants as params.

Untagging.

Oops, #41 came back green with 0 passes, which gets easily overlooked. Sorry about the typos.

Patch summary.

added API addition on field_read_fields()

Added an issue summary.

formatting

bump - anyone up for review ?

I would be happy to test this if there were a "light" way to consistently measure the memory consumption.
Something like drush ev '[RAM usage]; _field_info_collate_fields(); [RAM usage]' or something using mysqldump db {cache_field}|wc -c, so we can do several tests easily before and after the patch.

#46

clean drupal 8.x today git clone
with manually created module for filling one entity_type 'node' , bundle 'test_type', with 1000 fields(instances)

# mysql -p
Enter password: 
Welcome to the MySQL monitor.  Commands end with ; or \g.
Your MySQL connection id is 1013
Server version: 5.5.20-log Source distribution

Copyright (c) 2000, 2011, Oracle and/or its affiliates. All rights reserved.

Oracle is a registered trademark of Oracle Corporation and/or its
affiliates. Other names may be trademarks of their respective
owners.

Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

mysql> use d8
Database changed
mysql> select count(*) from field_config_instance;
+----------+
| count(*) |
+----------+
|     1009 |
+----------+
1 row in set (0.00 sec)

all tested on freebsd 9.0-RELEASE-amd64bit Core2Quad Q9300 CPU + 6GB RAM +
apache 2.2.21
php 5.3.9 (all dynamic extensions)
mysql 5.5.20
all 64bit builds
no opcode cachers

without patch #46

truncated cache tables run

xhprof data

Overall Summary
Total Incl. Wall Time (microsec): 3,144,291 microsecs
Total Incl. CPU (microsecs): 1,666,097 microsecs
Total Incl. MemUse (bytes): 69,812,944 bytes
Total Incl. PeakMemUse (bytes): 78,344,640 bytes
Number of Function Calls: 141,986

cached run

Overall Summary
Total Incl. Wall Time (microsec): 656,892 microsecs
Total Incl. CPU (microsecs): 342,460 microsecs
Total Incl. MemUse (bytes): 41,655,360 bytes
Total Incl. PeakMemUse (bytes): 48,087,848 bytes
Number of Function Calls: 17,281

with patch #46

truncated cache tables run

Overall Summary
Total Incl. Wall Time (microsec): 3,398,989 microsecs 8,1% lost
Total Incl. CPU (microsecs): 1,630,657 microsecs 2,2% won
Total Incl. MemUse (bytes): 69,860,680 bytes 0,1% lost
Total Incl. PeakMemUse (bytes): 76,471,984 bytes 2,4% won
Number of Function Calls: 146,831 3,41% lost

cached run

Overall Summary
Total Incl. Wall Time (microsec): 728,249 microsecs 10.9% lost
Total Incl. CPU (microsecs): 340,286 microsecs 0,6% won
Total Incl. MemUse (bytes): 41,559,464 bytes 0,2% won
Total Incl. PeakMemUse (bytes): 47,992,400 bytes 0,2% won
Number of Function Calls: 18,288 5,8% lost

for the bottom results i`d use most repeatable result from ~5-10 page runs

apachebench for a custom page

function pfield_menu(){
$items = array();

  $items['pfield'] = array(
    'title' => 'Pfield test',
    'page callback' => 'pfield_form',
    'access arguments' => array('access devel information'),
  );

return $items;
}

function pfield_form($form, &$form_state){
$form = array();
$mem = field_info_instances('node', 'test_type'); // just for test huge array from 1000+ fields
                                            
$form['pfield_test'] = array(
 '#markup' => '<pre>'.print_r('hello', TRUE).'</pre>'
);
return $form
 

without patch

# ab -c5 -n1000 "http://127.0.0.1/drupal/?q=pfield"
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Server Software: Apache/2.2.21
Server Hostname: 127.0.0.1
Server Port: 80

Document Path: /drupal/?q=pfield
Document Length: 1648597 bytes

Concurrency Level: 5
Time taken for tests: 171.750 seconds
Complete requests: 1000
Failed requests: 0
Write errors: 0
Total transferred: 1649080000 bytes
HTML transferred: 1648597000 bytes
Requests per second: 5.82 [#/sec] (mean)
Time per request: 858.750 [ms] (mean)
Time per request: 171.750 [ms] (mean, across all concurrent requests)
Transfer rate: 9376.59 [Kbytes/sec] received

with patch

# ab -c5 -n1000 "http://127.0.0.1/drupal/?q=pfield"
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Server Software: Apache/2.2.21
Server Hostname: 127.0.0.1
Server Port: 80

Document Path: /drupal/?q=pfield
Document Length: 1648597 bytes

Concurrency Level: 5
Time taken for tests: 168.670 seconds
Complete requests: 1000
Failed requests: 0
Write errors: 0
Total transferred: 1649080000 bytes
HTML transferred: 1648597000 bytes
Requests per second: 5.93 [#/sec] (mean) (in other run got 5.7#/sec :( )
Time per request: 843.349 [ms] (mean)
Time per request: 168.670 [ms] (mean, across all concurrent requests)
Transfer rate: 9547.83 [Kbytes/sec] received

~ 1.8% speedup at all
I do not see any speed/memory optimization here
Or possibly show me right way to test it

Ok @catch

I`ll try to test such config

ok...
another test

14 bundles of node entity
10 of em with 100 fields

function pfield_form($form, &$form_state){
$form = array();
$mem = field_info_instances('node', 'test_1');
$mem2 = field_info_instances('node', 'test_2');
$mem3 = field_info_instances('node', 'test_3');

$form['pfield_test'] = array(
 '#markup' => '<pre>'.print_r($mem3, TRUE).'</pre>'
);

/*
// the code for bundle and field generation
 $num_bundles = 10;
 $num_fields = 100;

for ($y = 0; $y < $num_bundles; $y++) {
field_attach_create_bundle('node', 'test_'.$y);
for ($i = 0; $i < $num_fields; $i++) {

$field = array(
 'field_name' => 'pfield'.$i,
 'type' => 'text',
);

field_create_field($field);

$instance = array(
 'field_name' => 'pfield'.$i,
 'entity_type' => 'node',
 'bundle' => 'test_'.$y,
 'label' => 'pfield_label'.$i,
 'widget' => array('type' => 'text_textfield', 'settings' => array('size' => 60)),
 'display' => array(
    'default' => array(
       'label' => 'above', 'type' => 'text_default', 'settings' => array())),
);
 field_create_instance($instance);
}
}
*/

return $form;
}

without patch

xhprof

Overall Summary
Total Incl. Wall Time (microsec):	318,519 microsecs
Total Incl. CPU (microsecs):	280,442 microsecs
Total Incl. MemUse (bytes):	43,985,304 bytes
Total Incl. PeakMemUse (bytes):	45,825,528 bytes
Number of Function Calls:	15,132

apachebench

# ab -c5 -n1000 "http://127.0.0.1/drupal/?q=pfield"
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking 127.0.0.1 (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Completed 600 requests
Completed 700 requests
Completed 800 requests
Completed 900 requests
Completed 1000 requests
Finished 1000 requests


Server Software:        Apache/2.2.21
Server Hostname:        127.0.0.1
Server Port:            80

Document Path:          /drupal/?q=pfield
Document Length:        168282 bytes

Concurrency Level:      5
Time taken for tests:   143.457 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      168765000 bytes
HTML transferred:       168282000 bytes
Requests per second:    6.97 [#/sec] (mean)
Time per request:       717.285 [ms] (mean)
Time per request:       143.457 [ms] (mean, across all concurrent requests)
Transfer rate:          1148.84 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.6      0      18
Processing:   452  716 293.0    637    3088
Waiting:      293  535 283.1    458    2926
Total:        452  716 292.9    637    3088

Percentage of the requests served within a certain time (ms)
  50%    637
  66%    754
  75%    821
  80%    878
  90%   1060
  95%   1303
  98%   1566
  99%   1671
 100%   3088 (longest request)

with patch

xhprof output

Overall Summary
Total Incl. Wall Time (microsec):	361,425 microsecs 13,4% lost
Total Incl. CPU (microsecs):	315,218 microsecs 12,4% lost
Total Incl. MemUse (bytes):	21,415,336 bytes 51.3% won
Total Incl. PeakMemUse (bytes):	22,897,072 bytes 50% won
Number of Function Calls:	34,433 127% lost

apachebench

# ab -c5 -n1000 "http://127.0.0.1/drupal/?q=pfield"
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking 127.0.0.1 (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Completed 600 requests
Completed 700 requests
Completed 800 requests
Completed 900 requests
Completed 1000 requests
Finished 1000 requests


Server Software:        Apache/2.2.21
Server Hostname:        127.0.0.1
Server Port:            80

Document Path:          /drupal/?q=pfield
Document Length:        168282 bytes

Concurrency Level:      5
Time taken for tests:   144.301 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      168765000 bytes
HTML transferred:       168282000 bytes
Requests per second:    6.93 [#/sec] (mean)
Time per request:       721.507 [ms] (mean)
Time per request:       144.301 [ms] (mean, across all concurrent requests)
Transfer rate:          1142.12 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.5      0      10
Processing:   441  720 375.7    584    3097
Waiting:      313  577 363.7    439    2762
Total:        441  720 375.7    584    3097

Percentage of the requests served within a certain time (ms)
  50%    584
  66%    741
  75%    816
  80%    899
  90%   1152
  95%   1486
  98%   2021
  99%   2261
 100%   3097 (longest request)

looks like this patch is a good memory optimization fix
possibly we can also do some work for multiple function call optimization and get a more nice code execution

but anyway 50% memory optimization is good enouph for making it RTBC

Many thanks for the profiling data, @podarok.

Attached patch fixes @catch's remarks, except those on cacheGetX() / cacheSetX() methods.

As I wrote earlier, I wasn't fully satisfied with the code legibility until I moved the caching logic in dedicated methods, but @podarok's numbers in #55 make me reconsider, since this adds function calls in the critical path.
With a primed static cache, field_info_instance($entity_type, $field_name, $bundle) makes :
- 1 nested function call in current D7/D8 : _field_info_collate_fields()
- 3 nested function calls with the patch : _field_info_field_cache()->getBundleInfo(), which calls cacheGetBundleInfo().

Moving from a giant statically cached array to a class with methods means we get two func calls (get the object, call a method) instead of one (get the array, read the part we need), but we should probably stop there and favor performance over legibility.

Will try to inline the cache methods back and see how this translates in XHProf.

- re: field_ui_inactive_instances() : yes, that's performance only, no API change. The current code calls the expensive field_info_instances($entity_type) in all cases, even when a $bundle_name is provided. My take is we could drop support for $bundle_name = NULL, but I don't want to do this in this patch. New patch makes the code in there more explicit and adds comments.

- re: _field_info_collate_fields_reset() : sure, this could be kept as a BC function in D7. I keep it removed in the current D8 patch.

looks like testing bot happy, so will try xhprof results in 24 hours against #55

@podarok : #57 has no real changes that would deserve another round of profiling.

I want to explore a bit the impact of cacheGetX()/cacheSetX() methods, I'll do that hopefully tonight or tomorrow.

ok, following up

So after spending some time with XHProf and ab, turns out the cacheGetX()/cacheSetY() methods add like 3% CPU overhead compared to when all caching logic is inlined. So I removed them in favor of the inlined version. A bit less friendly to read (see the getBundleInfo() method), but 3% is worthy.

New patch attached.

My setup (test code attached as well) :
20 node types
On each node type, 20 text fields :
- 10 fields shared across all node types
- 10 fields specific to the node type
That's 210 fields, 400 instances across 20 bundles. A "medium+" sized site.
+ devel_generate to create 100 nodes with dummy field data.

Memory measured with XHProf, CPU measured with "ab -n 300 -c 1"

1st test : Display all the fields on one entity (aka node/%nid)
- 8.x:
Requests per second: 13.80/s
Time per request: 72.441 ms (s/d 5.0)
Total Incl. PeakMemUse 15,662,856 bytes
- patch:
Requests per second: 14.51/s
Time per request: 68.941 ms (s/d 4.5) --> CPU -4,83%
Total Incl. PeakMemUse 8,384,776 bytes --> Memory -46,5%

2nd test: Display 10 entities (all fields) across 10 different bundles (aka "worst-case listing page")
- 8.x:
Requests per second: 6.61/s
Time per request: 151.173 ms (s/d 5.6)
Total Incl. PeakMemUse 17,905,632 bytes
- patch:
Requests per second: 6.64/s
Time per request: 150.651 ms (s/d 4.5) --> CPU -0,35%
Total Incl. PeakMemUse 13,871,712 bytes --> Memory -22,5%

the 'static cache' is just a class property since the class itself is held in a static, we should call it something else

Dang. You puzzle me, actually. I'll try to see if I can make things clearer in the class docblock, but I'd really prefer to keep the expression "static cache" in the code comments within the methods.

Even though not directly based on PHP static vars, conceptually those properties really act as a static, 'request only' cache as opposed to data read from the persistent cache bin. The various methods are just implementations of the common pattern present all over core :

if (value present in static cache) {
  return value;
} 
if (value present in persistent cache) {
  save in static cache;
  return value;
}
build value from scratch;
save in static and persistent caches;
return value;

For efficiency I removed the explicit getCache()/setCache() methods which made the pattern very explicit in code, but I tried to keep it clear in code comments. If we can't use the words 'static cache' there while it's the expression used throughout all of core for this pattern, then all I'm left with is some fairly cryptic code :-(.

[dishonest mode] The rest of core says "static cache" but doesn't directly use PHP static vars either, they use a userland construct based on a function that "happens" to have the word 'static' in its name :-p. [/dishonest mode]

What about this ?
- tried to make the phpdoc block for the class clearer
- added "quotes" around the term 'static' in code comments.
+ additionally fixes extraneaous arg passed to hook_field_storage_details_alter()

Interdiff attached.

#65
what about testing ab -c5 ?
today we mostly have a multicore environments and c5 is more realistic in results

Given this has been profiled already I don't see a need to do benchmarks - trust xhprof over ab any day.

actually, in #62, CPU measurements come from ab, memory measurements come from xhprof. I couldn't get xhprof to produce somewhat consistent and reproducible cpu ratios between runs, whereas the ab processing times over 300 runs were fairly stable.

@catch : what is missing before we can get this in ?

I didn't test this patch and I'm not able to comment on the functional code changes at this point.

However, I'd like to see the phpDoc warnings/notes to be shortened and clarified, as it seems that it's important that developers understand the implications of using whichever functions quickly and crystal-clearly.

To achieve that, put the most important info upfront. Skip the preceding "Warning:" or "Note:" hint. Since all of the functions seem to be cached, but only some retrieve a particular key/item of the cache, whereas the others can lead to memory issues, it's a bit hard to distill the important bits, but the docs should really try hard to communicate that clearly and quickly.

I'm also missing concrete pointers or examples for when it is appropriate to use the memory-hog-functions. It sounds like that's discouraged and almost not supported anymore (which would make sense to me), but the functions are still around.

E.g.:

 * Returns [blub blaah].
 *
 * Retrieves the requested field only.

and

 * Returns all field definitions.
 *
 * Retrieves all defined fields. Use field_info_field() instead, if possible.

I'll try to address @sun's remarks tonight.

Removing the Novice tag, I don't see this as a Novice tag just because it's only about comments :) #71 merely contains an example and basically says "Improve the documentation" and for that you need to understand the code, and that isn't that easy.

Subscribing, and adding that I have two sites now where this issue has reared its ugly head; one community-based site lets people set up 'networks' with profile2 profiles (for each network), and people can put any number of fields into their profiles... this means that we already have a few thousand field instances, and that site's field_info_field blob is 6.1MB.

I've checked through the patch, and code-wise/functionality wise it looks like it's good. Haven't tested it, but will try to review docs and submit a new patch if I can get a few spare moments. (Right now, this issue is pushing cache_clear's on one of the two sites over our memory limit, and causing some other minor performance issues, so it's in our best interest to get this backported too :).

Attaching a re-roll of the patch from #65, no actual changes yet.

Why is there an init() method on the FieldInfo class? You could just write protected $fieldMap = array(); (default property values) in class properties declarations, and get rid of both the constructor and the init method, this would be more CPU wise and code would be shorter.

Instead of isset() on the static cache arrays you could use array_key_exists(), this ensures you would not retry a second cache load in some cases, where the key would be here but could be NULL due to a previous failure; Another solution is to set an explicit FALSE on every miss (non existing field id or name or whatever) in order to ensure that isset() would return TRUE for non existing identifiers (if you don't like array_key_exists(), but that's a matter of taste).

EDIT: The previous paragraph is important, because I'm afraid that unstored invalid identifier could cause the underlaying cache get attempts and framework to do unwanted extra SQL or cache backend queries.

This should pass the tests, not sure if these two assertions are new or why they fail now. It is a change because previously, we returned "empty" bundles (= no fields) and now we don't do that anymore. Not sure if we that's ok.

I also moved the FieldInfo class to PSR-0 and tried to address the problem with trying to access missing fields. Not sure if that could result in problems because it worked differently previously, for example code that creates a field if it doesn't exist yet and runs multiple times, e.g. for multiple bundles. No idea if the caches are correctly cleared when the field is actually created. Let's see if there any test fails because of this

I also have no clue how this overlaps with the new Field API, any chance @yched or others working on this can check that?

Attaching an interdiff for the changes suggested in #78, there are a few other changes and the move that's not in there, because that makes the interdiff useless (moving and changing code at the same time).

@Berdir : thanks for grabbing the task.
I'll try to follow up with a review later today - meanwhile, if that's ok with you, I'll upload the patch in a branch in the new "D8 Field API" sandbox we opened for the we're currently running sprint.

@yched: Sure, that's ok.

Pushed.
D8 field API sandbox : http://drupal.org/node/1736366
field-info-cache-1040790-berdir branch.

Reviewed Berdir's changes in #79, adjusted some of them.
See attached interdiff.

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.phpundefined
@@ -213,7 +201,7 @@ class FieldInfo {
+    if (isset($this->fieldIdsByName[$field_name]) || array_key_exists($field_name, $this->fieldIdsByName)) {

I don't think the array_key_exists call is needed here, because we never store NULL in fieldIdsByName, we store MISSING_FIELD_ID.

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.phpundefined
@@ -230,6 +218,11 @@ class FieldInfo {
+      $this->fieldsById[self::MISSING_FIELD_ID] = NULL;

OK, this allows us to do return $this->fieldsById[$this->fieldIdsByName[$field_name]] when we return directly from "static" cache above. Seems a bit hackish though.
I'd prefer handling the case where fieldIdsByName[$field_name] == MISSING_FIELD_ID explicitly when we return from static cache.
See attached interdiff.

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.phpundefined
@@ -285,10 +281,13 @@ class FieldInfo {
   public function getBundleInfo($entity_type, $bundle) {
     // Read from the "static" cache.
-    if (isset($this->bundleInfo[$entity_type][$bundle])) {
+    if (isset($this->bundleInfo[$entity_type][$bundle]) || (isset($this->bundleInfo[$entity_type]) && array_key_exists($bundle, $this->bundleInfo[$entity_type]))) {
       return $this->bundleInfo[$entity_type][$bundle];
     }
 
+    // Make sure that the bundle info is initialized.
+    $this->bundleInfo[$entity_type][$bundle] = NULL;

Those changes don't seem needed since the code below always puts a non NULL / non-empty value in $this->bundleInfo[$entity_type][$bundle]

We currently don't check whether the [$entity_type, $bundle] pair is a valid, existing bundle.
I started doing so at some point, but it introduced some nasty race conditions & dependency on entity_get_info() caches, and I reverted that.
So I'd I'd go for not checking the validity of bundle. If some code asks for info about a non-existing node type, we cache (both statically and persistently) the fact that we don't have any fields on that node type. Broken code repeatedly asking for non existing bundles will therefore add more "empty" cache entries, but that's not a big deal, those cache entries won't bloat the cache entries that get loaded in memory for valid bundles.

Dunno why this switched to "need work".
I still need to get back at @sun's remarks in #71, but let's have the bot run meanwhile.

(side note : I forgot to push the changes corresponding to patch #83 in the field-info-cache-1040790-berdir sandbox. Did that just now.)

I've tested the patch on D8, and before, there's one row with all the field info (field_info_fields) that's over 100KB. After patch, there is a row per entity type (field_info:bundle:node:article and the like), and each cached entry per-bundle is only about 8KB.

Tests passed on the previous patch (which applied cleanly on my local computer), I think this patch looks great (and WOW there is a lot less data being passed around on one page request in my particular instance, instead of having an 8MB field_info_fields row... now it's about 10-50KB per entity type), and docs are reasonable.

Memory savings ftw!

Ah, still haven't incorporated docs suggestions from sun in #71, so back to needs work, but imo, only the docs need some work. Taking a stab at this now, but only have a little time, so might not be able to get too far.

Didn't get time to work on this, and probably won't by next week :(

Back to CNR: I've made a few spelling corrections and moved around/edit a couple bits of text for clarity. Everything seems clear to me. For D8, maybe it makes sense to remove some of the now 'wrapper-ish' functions like field_info_field_by_ids(), but that would have further implications beyond what this patch is trying to achieve.

The warnings about memory use are fairly clear to me (especially after rearranging a couple docblocks), but please highlight what specific bits might be vague or too verbose still.

#89: field-info-cache-1040790-89.patch queued for re-testing.

#89 is green, the fail is #1779638: Unexplained test failure in LocaleFileImportStatus->testBulkImportUpdateExisting().

Thanks @geerrlinguy !
The comment changes are fine by me.

For D8, maybe it makes sense to remove some of the now 'wrapper-ish' functions like field_info_field_by_ids(), but that would have further implications beyond what this patch is trying to achieve.

Fully agreed, there might totally be some API refactors worth considering around here, but let's do that in followups. The API around getting field and instance definitions will probably be impacted by the "convert Field API to CMI" patch.
Also, the FieldInfo object will definitely be read from the DIC at some point, but we'll see that later. (#1708692: Bundle services aren't available in the request that enables the module is still a blocker for this anyway)

The goal here is to keep changes minimal for an easy D7 backport.

I've been willing to see whether a fix for #1400256: Tests for: $info['fields'], not always set? could be baked into this patch, but that can also happen in a followup and shouldn't prevent anyone from RTBCing this :-)

Updated issue summary.

Since I just changed a few comments, didn't touch code, and since the testbot and my manual testing confirmed no issues with the patch in #83 (now #89), I'm marking RTBC.

I've also updated the Issue Summary with more details, and some things to consider as follow-ups.

The current patch should fix #1400256: Tests for: $info['fields'], not always set?. The steps-to-reproduce for this bug mostly exist on the D7 side, though, so I'm working on the D7 backport patch (being tested right now at #1305362-38: Helper issue for #1040790).

crosspost :-)

Better formatting, added example, clarified RTBC status.

Attached is a D7 backport. Passes tests : #1305362-47: Helper issue for #1040790

Re-roll, hope it's ok, did it really quick, so wait for the bot :)

Meh forgot a file.

#100 should be correct, thks @swentel !

Ah wait, the patch was broken by #1757504: Regression: language field is not visible on manage display, made changes in _field_info_prepare_extra_fields(). We need to report the changes to the new location of the function. Working on it.

OK, here it is.

Tried to test the D7 backport on a test instance of Commerce Kickstart, got:

Fatal error: Call to undefined function _field_info_collate_fields() in modules/contrib/entity/entity.module 

.
(both on frontpage and on update.php). So, contrib will need some updates I guess?

@bojanz : aw, right, some contrib modules call _field_info_collate_fields() directly :-/. I guess we'll need to bake some more BC in the D7 version. Will try to look into that later today.

#103's fail is #1779638: Unexplained test failure in LocaleFileImportStatus->testBulkImportUpdateExisting() again, patch is green.
Back to RTBC.

#103: field-info-cache-1040790-103.patch queued for re-testing.

@catch : it's the same unrelated fail - it's all over the queues these days :-)
Let's try to be lucky, but #103 is green, really.

Green :-)

@bojanz: can you try with the attached D7 patch ?
(interdiff with #96 attached)

This needs more testing.

Applying the patch to a Kickstart v1 (D7 + Entity API + Rules + Commerce) install worked for me, but applying it to Kickstart v2 (Kickstart v1 + many contribs such as message, title, fences, crumbs, etc) didn't (breached 128M memory limit).

Preliminary poking showed that the problem is with calling field_info_instances() from hook_field_extra_fields(), because that causes it to loop back to FieldInfo::getBundleInfo(), over and over. This is something that Title does, as well as Message, as well as Commerce (and yet I didn't trigger from Kickstart v1, but maybe that's because that site uses less memory, so the 128M limit is further away).

After commenting out the call to hook_field_extra_fields() from inside FieldInfo::getBundleInfo(), I got the site to run.
Initial Kickstart v2 install without the patch: 1.5M.
With the patch: 0.25M.
(I used memory_get_usage(TRUE), which might be a bit too crude)

@bojanz: what about this one ? (also includes @David Rothsein's test from #1400256: Tests for: $info['fields'], not always set?)
(interdiff with #113 attached)

Sorry, rather this one.
(interdiff with #113)

(Still testing Kickstart v2, as a heavy enough environment to represent a typical site)

This one works. However...
"drush cc all" now takes significantly longer (about a minute and a half I'd say).
The first page load after the "drush cc all" exceeds the 30s load time and crashes.
(This happens for every "drush cc all", not just the initial one after applying the patch)
After that it works, and I see a consistent decrease in memory usage (from 0.85M to 1.25M per page as reported by devel).
I'm also seeing a consistent increase in page load times, between 200ms and 500ms per page.
So it seems there's still something fishy going on.

I can xhprof this on sunday if nobody beats me to it.

Tested this on 7.x as well, seeing similar results. looking at with xhprof shows 18 calls to FieldInfo::getFields() through field_info_fields(), which is called by commerce_info_fields() (commerce_product_reference_entity_view()) and _nodeconnect_get_ref_fields() in mycase.

I'm quite sure that commerce can be optimized to get rid of these calls, especially because we're not actually using commerce_product_reference on this site at all. Don't have time to look into that right now, @bojanz, can you have a look at that? Another performance issue related to that function: #1567090: Unconditional call to entity_metadata_wrapper() in commerce_product_reference_entity_view().

Did a test with the D7 patch as well on a current site:

- 138 fields
- 305 instances

yes it's a complex site :)

Before patch:
Page execution time was 112.85 ms. Memory used at: devel_boot()=1.82 MB, devel_shutdown()=16.98 MB, PHP peak=17.25 MB.
After patch:
Page execution time was 84.28 ms. Memory used at: devel_boot()=1.82 MB, devel_shutdown()=11.82 MB, PHP peak=12.25 MB.

Also as bojanz reported, drush cc all takes a long time, first call to the page didn't go white though.
Memory drop is fantastic, and didn't see an increase on page loads so far.

I'm going to run this one in pre-production for a few days because I only briefly browsed around on the site so far. Client is giving in a lot of content and browsing a lot, so I'll hear reports from them soon enough once something might go wrong.

(Firstly, sorry for the brain-dump of a comment to follow… I’ve been debugging for an hour or so, and can’t piece together everything as succinctly as I’d like.)

After applying the patch to a local D7 site, I kept getting out of memory errors (with 256MB RAM allocated in memory_limit), in different field files (field.crud.inc, field.info.inc, field_sql_storage.module, etc.).

I was using the APC module and caching cache_field and some other caches in APC, but this was causing out of memory errors even when I had PHP’s memory limit set to 1, 2, 3 or 4 GB (yikes!). So, I turned off APC caching, and instead used database caching (which helped the httpd process stay below 200MB for the most part). But now the load switched from memory to CPU, and the max time limit kept getting hit (I tried letting it run for 15 minutes at one point).

When I could get some debug output, after 15 minutes of a process running, I found that there were tons and tons of calls to field_read_instances(), each with slow-running queries to {field_config_instance} and {field_config} that took about 43ms (looks like these calls were coming from getFieldMap()). One such query is:

SELECT fci.* FROM field_config_instance fci INNER JOIN field_config fc ON fc.id = fci.field_id WHERE (fc.active = :db_condition_placeholder_0) AND (fc.storage_active = :db_condition_placeholder_1) AND (fc.deleted = :db_condition_placeholder_2) AND (fci.deleted = :db_condition_placeholder_3)

The below query was also called over 450 times, and I don't think it was cached:

SELECT field_name, type FROM field_config WHERE active = 1 AND storage_active = 1 AND deleted = 0

The entire time, CPU usage spiked at 100% and stayed there.

So… for a site with 550 fields, 5,049 field instances, 12 node types, and 479 profile types (so 491 entity bundles), it looks like this patch isn’t very helpful when trying to build caches…

Yes, I was to hopefull but had to revert as well. Clearing cache, but also simply adding a new field, was a real pain. In our case, cache was handled by memcache which sometimes got completely crazy. I'm going to dig in this a bit during the weekend.

If I statically cache the results of the second query in #120, I still run out of execution time after 470 calls to the first query and 800 seconds of time.

Ok, real-life use cases are on D7 right now, so I think we'd better switch this to D7.
When we get a correct D7 patch, we can come back to the usual "fix in D8 then backport" way.

Regarding the slowness reported in #120:

I'd be surprised if getFieldMap() was a problem. It's statically and persistently cached, and even in contexts where cache gets cleared 10 times a request, I don't see how the request could get painfuly slow, even with 5000 records in field_config_instance

What I think is the issue, though, is that the patch changes field_info_fields() and field_info_instances() (the ones that return *all* fields and *all* instances) to not be statically nor persistently cached. As per the early posts in this discussion (around #20 / #30), it was decided to stop caching those.
While I think that's valid for D8, it's not a behavior we can change for D7. While working on this patch earlier this spring, I grepped a couple "big" contrib modules, and there's an *awful* amount of code out there that calls field_info_instances(), even in places where it could do differently - just because they can : field_info_[fields|instances()] currently cost nothing, everything is in memory anyway.
I filed a couple patches against a couple of those modules back then, but it's a lost battle.

- In D8, there will be smarter ways to navigate through the list of fields and instances to find a specific one (the fieldMap is one).
- In D7, we need to keep field_info_fields() and field_info_instances() fully cached - just not loaded in memory by default on every page, only when explicitly requested.
Then, a call to one of those will have a cost (load or rebuild a huge array, have it sit in memory), but a series of calls - which is what I think is happening on the cache clears mentioned in the comments above) will mean no extra cost.
Contrib modules calling those in regular page views will need to be fixed individually.

I see no other way around for D7. We can fix field_info_instances($entity_type, $bundle), we cannot fix calls to the "gimme all" versions, only try to progressively move away from them.

Current D7 patch, being a straight backport of the D8 patch, still has field_info_fields() and field_info_instances() uncached, which is why IMO we have those issues on cache clears.

I see no other way around for D7. We can fix field_info_instances($entity_type, $bundle), we cannot fix calls to the "gimme all" versions, only try to progressively move away from them.

Current D7 patch, being a straight backport of the D8 patch, still has field_info_fields() and field_info_instances() uncached, which is why IMO we have those issues on cache clears.

That sounds workable. The main offender, in my case, is Profile2, and since it's hopefully a candidate for core inclusion, it's already being cleaned up quite a bit... And this kind of conversion would probably be considered as part of the inclusion cleanup. (See #1668292: Move simplified Profile module into core).

Attached patch adds back static caching for field_info_fields() and field_info_instances().

Tested on Commerce Kickstart 2, full setup with demo content :
- without patch, 'drush cc all' takes 30 seconds
- with patch, 'drush cc all' takes 32 seconds

Also :
I actually installed kickstart 2.0-beta2 a couple days ago.
Back then, the patch #116 would die on an infinite recursive loop, because commerce_product_reference_field_extra_fields() was calling field_info_instance().
I needed to split collecting extra fields into a separate function and cache entry to be able to get a cache clear to run.

Since bojanz was able to test the patch in #117, I assume the code has changed in kickstart 2.0-rc1.
It seems wise, however, to keep this separation. I don't know what else we have in contrib. That means an extra cache hit per bundle rendered in the page.

So, attached patch:
- adds back static caching on getInstances() and getFields()
The static lists are shared with getField($field_name) and getBundleInstances($entity_type, $bundle), so that we don't bloat memory with twice the same fields and instances.
Still @todo : add persistent caching ? Not sure, given the numbers above
- moves getBundleExtraFields() to a separate method

Oops, missed a rename.
Attached interdiff with #116.

Sharing the static lists raises an issue because they keep track of non-existent fields and bundles :

field_info_instances('non_existsing_entity_type') -> array()
field_info_instances('non_existsing_entity_type', 'non_existing_bundle') -> array()
field_info_instances('non_existsing_entity_type') -> array('non_existing_bundle' => array())

field_info_field_by_ids() -> array(1 => $field_1, 2 => $field_2)
field_info_field_by_id(99) -> NULL
field_info_field_by_ids() -> array(1 => $field_1, 2 => $field_2, 99 => NULL)

Hence the test fails above.

Not sure about the right fix.

I see two different fixes (probably more are possible):
1) Keep two statics, one with real data, and the other one which only says 'exists' or 'does not exist' (TRUE or FALSE for example), when requesting a single field, attempt lookup in the second, and if exists then pick the value from the first. When requesting the complete list just return the first.
2) Proceed to array_filter() calls when returning it complete.
EDIT: While 2) is simpler, it's also less performance wise (it creates a whole lot of new arrays with array_filter()), so I think 1) is a good path even if a bit more complex.

@pounard :
Right, I considered array_filter() too, but we don't want to create copies of the massive arrays.
Attached patch goes for something like your approach 1).

It passes the 'field info' tests, let's see about the rest.

Patch also updates phpdocs, since field_info_fields() and field_info_instances() do statically cache their stuff now.

It looks fine, but I don't know the whole patch so don't consider my judgement is reliable. It needs to be manually tested and measured I guess.

Awesome results! This patch halved the page load times for most pages on my test site. The site has 541 fields, 4,629 field instances, 12 node types, and 490 profile types. I've attached a file with results from three page loads, with a cache clear and a normal page load (cached), on two different pages on the test site. One page was complex, with a couple views, a few entities loaded, etc. Another page was a simple node page.

Here's a quick summary of the results (before and after the patch):

Before Patch

• Complex page, cache clear all: 11,324ms, 186MB peak memory use
• Complex page, normal/cached load: 1,473ms, 93.25MB peak memory use
• Simple page, cache clear all: 11,281ms, 185.35MB peak memory use
• Simple page, normal/cached load: 1,221ms, 88MB peak memory use

After Patch

• Complex page, cache clear all: 30,680ms, 174.5MB peak memory use
• Complex page, normal/cached load: 668ms, 34.75MB peak memory use
• Simple page, cache clear all: 31,991ms, 175.35MB peak memory use
• Simple page, normal/cached load: 463ms, 32MB peak memory use

Differences

• Complex page, cache clear all: +19,356ms, -11.5MB peak memory use
• Complex page, normal/cached load: -805ms, -58.5MB peak memory use
• Simple page, cache clear all: +20,710ms, -10MB peak memory use
• Simple page, normal/cached load: -758ms, -56MB peak memory use

After the caches are built, page load times and memory usage more than halve. This is better than I had expected! For smaller sites, this will only mean a couple MB savings at most... and cache clears won't take nearly as long as my tests (I don't know of any other site that has as many entities, but I imagine Commerce and Profile2-heavy sites may come close).

Anyways, I'd gladly take an extra bit of time during a cache clear (which only happens every few days on this particular site) for 2x faster page loads and half the memory requirements!

[Edit: One more note: if you look at the detailed results file in the next comment, you'll notice I have the sizes of the cache_field table before and after the patch listed as well. Before the patch, it's about 6 MB. After, it's over 100 MB. I wouldn't be able to cache table in APC after the patch (limited RAM), but that's fine with me since the entire 6 MB field doesn't need to be loaded on every request. Just something to note.]

Whoops! I forgot to attach the detailed results file...

Cool! Let's get some results from a few more people. Also moving to 8.x and to be ported.

FYI... When next re-rolled, please check for white spaces at the end of lines. There were two instances in the patch in #131.

Yeah, noticed that, but forgot to mention.

Attached patch for D7 fixes whitespace errors.

@geerlingguy : thanks for the tests (and reroll) ! It would be good to hear from Commerce Guys guys (sorry, lame, couldn't resist :-p), but your numbers are very encouraging.

Regarding the size of the cached entries : what the patch does is store fields + instances involved in a given bundle in a dedicated cache entry for the bundle. Thus, $field definitions get duplicated in the cache entries for the bundles it appears in.

Going from 6Mb to 100Mb seems a lot, though. Beware that this cache table also contains the field values cache.
Would be interesting to check how many "field_info:bundle:[entity_type]:[bundle]" entries you have, and how many fields are stored in each of them, see if that matches your sites field map.

Also - we still need to add back persistent caching of "all fields" and "all instances" IMO. I'm hoping to be able to roll a patch later tonight

Oops. Guess this is still NW then. :(

Sorry, traveling at the moment, will try to test the latest patch as soon as I stop (in about 2 days).
Great work so far!

Attached patch:
- adds back persistent caching for field_info_fields() and field_info_instances()
- exposes the "lightweight" field map (now includes field type) through a new field_info_field_map() function, which should cover most current use cases for field_info_fields() / field_info_instances(), so that contrib can start getting rid of those expensive calls.
- updates phpdocs for the new caching behavior (we do cache full lists)

Fixes phpdoc for Fieldinfo::getExtraFields(), was blindly copy pasted from getBundleInstances().

And the corresponding D8 patch.

I'm going afk til the end of October. I'm leaving it to you folks to drive this home :-)

Er, D8 patch attached.

Adjusted follow-up task description.

Updated the "API changes" section in the issue summary, to match the current patch.

Updated for latest changes

#146: field-info-cache-1040790-145-D8.patch queued for re-testing.

I applied the latest D7 patch version and I get exceptions thrown by search api, when rebuilding views data, when flushing the caches. I'm not giving any details right now but I'm not sure of the source, but this wasn't happening before applying the patch. Ignore that this doesn't seem related, just a weird coincidence.

I've applied this patch to my dev site that has commerce and a ton of fields on lots of bundles, and it certainly seems to make page loads quicker. No error messages on cache clear.

+++ b/modules/field/field.info.inc
@@ -34,7 +57,25 @@ function field_info_cache_clear() {
+function _field_info_collate_fields($reset) {

@@ -162,284 +203,6 @@ function _field_info_collate_types($reset = FALSE) {
-function _field_info_collate_fields($reset = FALSE) {

This function has changed its signature.

Does this patch needs a change request, or is it ok because it's a private function?

Using D7 patch in #144, clearing caches takes about 5 seconds longer, but normal page load times and memory usage were the same. It's crazy how much faster the site feels with this patch. Most pages load in half the time they used to!

Hoping to hear more of the same from some commerce user... If all that's needed is a quick test of before/after with a default commerce install, I'd be glad to do that, but it seems like someone who has an actual site with real data to test would probably be preferable.

When I tested it over a complex commerce site with 100+ fields here, I didn't see any difference in page generation time at all (tested on pages displaying 30 products and 30 associated nodes). Important note thought: the windows box I'm working on spends half the time on drupal_load() and PDO::execute() alone. Aside of that, drupal_alter() take more time than every other field API operation (3000+ drupal_alter() calls dues to the field API alone). I didn't notice any difference when clearing the caches. After all that I got unrelated troubles with Search API.

This is an objective feedback, aside of that, all other feedbacks are really encouraging.

Did two testing scenarios (with the D7 patch), created a setup with a drush script (works for D7 and D8) to quickly create content types and generate a number of fields (currently only textfields though) - see http://drupal.org/sandbox/swentel/1781486 - just run 'drush fact anynumber' (defaults to 10)

Scenario 1
7 content types, 286 fields and instances, 100 nodes - 1 node type has 236 fields, others have between 10 and 26.

Request on frontpage - nodes from various types generated by devel generate (100)
Before: Page execution time 130.94 ms. Memory used at: devel_boot()=0.99 MB, devel_shutdown()=11.14 MB, PHP peak=11.5 MB.
After: Page execution time 134.38 ms. Memory used at: devel_boot()=0.99 MB, devel_shutdown()=10.38 MB, PHP peak=10.75 MB.

Request on node with 236 fields
Before: Page execution time 131.76 ms. Memory used at: devel_boot()=0.99 MB, devel_shutdown()=11.27 MB, PHP peak=12.75 MB.
After: Page execution time 127.95 ms. Memory used at: devel_boot()=0.99 MB, devel_shutdown()=10.28 MB, PHP peak=11.75 MB.

Request on node with 16 fields
Before: Page execution time 62.61 ms. Memory used at: devel_boot()=0.99 MB, devel_shutdown()=10.06 MB, PHP peak=10.25 MB.
After: Page execution time 51.03 ms. Memory used at: devel_boot()=0.99 MB, devel_shutdown()=4.98 MB, PHP peak=5.25 MB.

Request on node with 4 fields (standard core article content type)
Before: Page execution time 57.77 ms. Memory used at: devel_boot()=0.99 MB, devel_shutdown()=10.02 MB, PHP peak=10.25 MB.
After: Page execution time 48.71 ms. Memory used at: devel_boot()=0.99 MB, devel_shutdown()=4.74 MB, PHP peak=5 MB.

Scenario 2

10 content types, 50 fields each (so yeah total of 500), 1000 nodes

Request on frontpage - nodes from various types generated by devel generate
Before: Page execution time 113.48 ms. Memory used at: devel_boot()=0.98 MB, devel_shutdown()=14.95 MB, PHP peak=15.25 MB.
After: Page execution time 107.48 ms. Memory used at: devel_boot()=0.98 MB, devel_shutdown()=11.12 MB, PHP peak=11.5 MB.
On the frontpage, only 7 different node types where rendered.

Request on node with 50 fields
Before: Page execution time 79.62 ms. Memory used at: devel_boot()=0.99 MB, devel_shutdown()=14.07 MB, PHP peak=14.75 MB.
After: Page execution time 62.09 ms. Memory used at: devel_boot()=0.99 MB, devel_shutdown()=5.62 MB, PHP peak=6.25 MB.

Whoops—we need to get an answer for #154:

This function has changed its signature.
Does this patch needs a change request, or is it ok because it's a private function?

Other than that, it seems like this can go back to RTBC...

#146 seems fine to me; works for D8 in manual testing (creating fields, deleting fields, viewing nodes, etc.). I didn't do any performance benchmarks on it, though, since I only had a couple bundles and a few fields. Anyone else able to test and confirm RTBC for D8?

Then we could set this back to CNW to resolve the question mentioned in #154 for D7, after a D8 commit.

Reroll of D8 patch

Ok, now that's a nice surprise, I'll recheck this evening.

Ok, needed some re-rolling after #1326634: Clean up API docs for field module and #1785256: Widgets as Plugins got in - c'mon bot!

- edit - also removed the t() calls in fieldInfoTest.php, but mistakenly thought that was a new file, will need a new re-roll tomorrow. see #1797106: Remove t() from asserts messages in tests for the Field module for more background.

Ok, let's see what this does.

Spoke to @swentel and re-un-tagging based on the current status here since there are functional parts of the _ficf() patch that are broken after widgets-as-plugins. Sorry for the noise!

Alright here we go - thanks to Stalski as well for the debugging.

#172: 1040790-170.patch queued for re-testing.

Looks good to me. Here are some quick performance comparisons, from an upgraded site with about 25 fields shared across 8 node types (bundles) - 3 runs each for comparison:

Before Patch

• Cache clear: 636.37ms, 25.75MB RAM
• Complex Page load: 438ms, 23.25MB RAM
• Simple Page load: 483, 23MB RAM

After Patch

• Cache clear: 640ms, 25.5MB RAM
• Complex Page load: 555ms, 23MB RAM
• Simple Page load: 470ms, 22.75MB RAM

The page load time for the complex page is a little odd, don't know why it took longer after patch, but RAM savings are apparent, even though slight on a smaller site. I think swentel said he was going to do a little testing on a larger site instance too.

Did a test like the last one in #157 for the D8 patch as well:

10 content types, 50 fields each (so yeah total of 500), 1000 nodes (devel generate)

Request on frontpage - nodes from 7 different node types
Before: Page execution time was 184.68 ms. Memory used at: devel_boot()=1.18 MB, devel_shutdown()=15.47 MB, PHP peak=15.75 MB.
After: Page execution time was 178.27 ms. Memory used at: devel_boot()=1.19 MB, devel_shutdown()=12.5 MB, PHP peak=12.75 MB.

Request on a single node page with 50 fields
Before: Page execution time was 95.89 ms. Memory used at: devel_boot()=1.18 MB, devel_shutdown()=14.41 MB, PHP peak=14.75 MB.
After: Page execution time was 72.91 ms. Memory used at: devel_boot()=1.2 MB, devel_shutdown()=5.72 MB, PHP peak=6 MB.

#1797170: Remove t() from assertion messages in tests for field_ui.module and #1797106: Remove t() from asserts messages in tests for the Field module got in - last re-roll of the day. Tagging with 'avoid' as well.

Still looks good. Let's get this in, it's about time :)

#144: field-info-cache-1040790-144-D7.patch queued for re-testing.

I think that the D7 patch should be RTBC if tests pass, but there was a lingering question in #154: "This function [_field_info_collate_fields()] has changed its signature. Does this patch needs a change request, or is it ok because it's a private function?"

I'm thinking that a lot of modules may use this function. Even if the underscore denotes a private function, I guess that a lot of modules will be tempted to use it.

#180, #1288364: Clean up reset patterns in field.info.inc already changed the signature, and there was no change notice. It's a private function, if they used it that's their problem.

I grep'd through major contrib modules, and all I could see was one call in entity.module that would need to be changed to _field_info_field_cache()->flush();

if they used it that's their problem

I agree wholeheartedly. Now that I think about it... everywhere private functions are mentioned, there are heavy warnings to NOT use them in external code, and to do so at your own risk (see two examples).

Therefore, with a passing test in #144, I'm RTBCing.

I *really* can't see any use case for 3rd party code to use _field_info_collate_fields() (no param) directly, we have wrapper API funcs for that. Any module doing so could be rightfully considered broken, but I seriously doubt there is any.

There are direct calls to _field_info_collate_fields(TRUE) (reset the cache), though. Entity API is one example (see #105/#107 above).
Entity API is not *supposed* to do that, but I guess it has kind of valid reasons - possibly because the official field_info_cache_clear() API func also clears the "list of available field types, widgets, formatters" cache, and Entity API, doing funky cross dependent stuff with various info caches, needed a more granular clear). So it seems a valid expectation to keep those calls working, and we can do so with minimal efforts.

IMO, this doesn't deserve a mention in the release notes, but I guess that's up to @webchick and @David_Rothstein to decide.

PS : yay for D8 commit, thanks @swentel & @Stalski for the rerolls !