Refactor entity storage to load field values before instantiating entity objects

What can I say, seemed like a great issue to work on at 1am :p

Refactored the load methods to have only a single place where we create entity objects instead of two, and loading all the data upfront. Also renamed some variables, like making sure thatn when we have $entities, they are actually entity objects, and when not, renamed to $values or so. Downside is some places where we foreach over $values, went with $entity_values for the inner variables, better suggestions are welcome.

Some parts are not so pretty yet, maybe we can

EntityFieldTest passes, but that's likely not covering fun parts like delta/revision loading.

Will try to fix those tests later. BulkDeleteTest is very funny test fail and proves that it actually works. This is what happens:

1. We create the fields and entities, and then load them. We only access entity key values like the bundle, that we have fast-paced through $this->entity keys and don't need the field definitions to access them. So we never actually load them.
2. Then we delete the field.
3. Now we load the values from the field table, and try to compare with the entity.

BUT, we never loaded the field definitions in the entity. so trying to access to deleted field loads them *now*, and now the field doesn't exist anymore and we get those errors.

I can see two ways to fix this:
a) Quickfix: Force a fetch of the field definitions in the entity, so that they are loaded before we delete the field.
b) Rewrite it to do the same query before and after and then compare the results of the query.

other ideas?

This should fix the test fails. was missing support for not having a language key defined, looks like we only have accidental test coverage for that, e.g. because EntityTestLabel has no langcode key defined.

Yeah, docblocks need to be updated, also wasn't sure about the method names yet. maybe LoadSharedFields() (or FieldValues() and loadDedicatedFields()? but it's not the field that's dedicated or not, it's the storage...

Wrong langcode checked, this should be better.

Yay green. Some documentation improvements.

Thanks.

3. I already did, just forgot to remove that comment ;) See interdiff for the last patch.

4. I'm not sure. It's not called anywhere else, so we always call it with a value anywhere.

5. They're still used in a few places, e.g. in entity query. There's afaik an existing issue for removing it.

I'm not sure if we should also update the corresponding save methods. Not the implementations, just the method names.

Didn't profile this yet, but I think this is a nice performance improvement when loading entities from the storage.

1. I think we still need it, but I'm happy to be convinced otherwise. Note that languages are keyed below the field name in the values array still, so we'd need to loop over all fields to find the complete list of languages. If it were the other way round then it would be easy.

Renamed a few more methods for save and delete, better suggestions are welcome.

As an additional step, would be nice to group them a bit more consistently in the class, current order seems a bit random. Probably best in a followup, since it would make the diff harder to review.

Yes, that's exactly why I didn't do it yet :)

Correct, that all happens in doDelete(), would be overkill to move that to a separate method I think, it's just an if (hasTable) { delete query }

What we could do is make the delete in the dedicated table multiple, but again, not sure if we should do more refactoring here.

I know this is pre-existing, but loading more stuff in "mapFromStorageRecords" is not what would one expect.
Can we move that loading to an earlier step?

Yes, it doesn't. But it *does* belong right between the two steps that *do* belong into mapFromStorageRecords() (converting $records into a values array that can be passed into the entity class and creating that entity class). I honestly don't know how to change that in something that makes more sense.

Same for translations, I had another look and I'm not sure. Yes, we already loop over $values, but we would have to loop over languages inside each $value array as well, and find any language that we don't know about yet. That feels like it might be slower than what we have now. I think the issue to do this would be #2142885: Simplify ContentEntityBase internal field storage by removing special treatment for LANGCODE_DEFAULT, because that would make it trivial to get rid of the $translations argument.

Udated the issue summary and added a beta eval.

Still want to some performance comparisons.

Ok. Was running the following script on a real site:

use Drupal\node\Entity\Node;

\Drupal::entityManager()->getStorage('node')->resetCache();
$nids = \Drupal::entityQuery('node')
  ->condition('type', '*****')
  ->range(0, 100)
  ->execute();

$start = microtime(TRUE);
Node::loadMultiple($nids);
$end = microtime(TRUE);

var_dump($end - $start);

The site only has a single language, but ~25 configurable fields and *a lot* of content (320k nodes), so a fair amount of that time is probably spent on the actual queries.

HEAD: 0.52s
Patch: 0.39s

I think that's pretty nice :)

Did not yet do an actual profiling of where that time is spent, that would probably also be good to know. I'v planned to do a fair amount of profiling with that site, to see how it performs.

Ok, repeated the same with xhprof, see attached screenshot.

mapFromStorageRecords() is 64% faster and uses 44% less memory (A part of that memory will then instead be used later, but only the fields that are actually being accessed).

Also, as usual, the uprofiler/xdebug overhead is huge (even if just one of those extensions is enabled). The same script with without either of those extensions enabled is 0.28s vs 0.22s, so almost twice as fast.

Not much I think? @plach already mentioned that he's OK with changing/removing $translation later, I think it's up to @fago to RTBC this, assuming he's OK with keeping the map/load method names/calls as it is for now.

Small conflicts with #2431329: Make (content) translation language available as a field definition.