#1969728: Implement Field API "field types" as TypedData Plugins needs FieldItem objects to be able to access their own plugin_id and plugin_definition.
The patch over there currently includes this, but it would be best in its own issue IMO.

This was not doable before #1986802: rename PluginInspectionInterface::getDefinition() to getPluginDefinition(), because PluginInspectionInterface had getDefinition(), which clashed with TypedDataInterface's own unrelated getDefinition().

Files: 
CommentFileSizeAuthor
#14 TypedData-inspection-2018323-14.patch1.13 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 57,628 pass(es).
[ View ]
#11 TypedData-inspection-2018323-11.patch7.23 KByched
PASSED: [[SimpleTest]]: [MySQL] 57,418 pass(es).
[ View ]
#11 interdiff.txt7.67 KByched
#8 TypedData-inspection-2018323-8.patch6.86 KByched
PASSED: [[SimpleTest]]: [MySQL] 57,285 pass(es).
[ View ]
#8 interdiff.txt6.29 KByched
#4 TypedData-inspection-2018323-4.patch6.93 KByched
PASSED: [[SimpleTest]]: [MySQL] 57,462 pass(es).
[ View ]
#4 interdiff.txt784 bytesyched
#1 TypedData-inspection-2018323-1.patch7.01 KByched
PASSED: [[SimpleTest]]: [MySQL] 57,403 pass(es).
[ View ]

Comments

yched’s picture

Status:Active» Needs review
StatusFileSize
new7.01 KB
PASSED: [[SimpleTest]]: [MySQL] 57,403 pass(es).
[ View ]

Patch extracted from #1969728: Implement Field API "field types" as TypedData Plugins.
Not sure I caught all the pieces, let's see what the bot thinks.

Note: not sure what to do with LocaleTypedConfig (extends TypedData).
The code that is in charge of instanciating those objects should be modified accordingly, but I don't know where that is.
Doesn't seem to cause test fails though...

yched’s picture

Tagging

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/TypedData/TypedData.phpundefined
@@ -7,13 +7,16 @@
+use Drupal\Component\Plugin\PluginInspectionInterface;
...
+abstract class TypedData extends PluginBase implements TypedDataInterface, PluginInspectionInterface {

This is redundant as Drupal\Component\Plugin\PluginBase already implements PluginInspectionInterface, but I guess it can't hurt.

yched’s picture

Issue summary:View changes

Rephrase a bit

yched’s picture

StatusFileSize
new784 bytes
new6.93 KB
PASSED: [[SimpleTest]]: [MySQL] 57,462 pass(es).
[ View ]

True. Doesn't hurt, but I guess it's not something we do in other places.

Status:Needs review» Needs work
Issue tags:-Entity Field API, -Field API NG blocker

The last submitted patch, TypedData-inspection-2018323-4.patch, failed testing.

yched’s picture

Status:Needs work» Needs review
Issue tags:+Entity Field API, +Field API NG blocker

#4: TypedData-inspection-2018323-4.patch queued for re-testing.

fago’s picture

+++ b/core/lib/Drupal/Core/TypedData/TypedData.php
@@ -41,6 +43,10 @@
+   * @param string $plugin_id
+   *   The plugin_id for the plugin instance.
+   * @param array $plugin_definition

I must say I'm not in favour of having those generic plugin_id and definition in there. That only makes sense to you when you are exactly clear which plugin this is about, but when someone is working with those classes that's not necessarily the case. (So, yes I can follow the reasoning people don't like generic typed data stuff in their classes - it's the same.)

Cannot we just call it differently here, e.g. $data_type and $type_definition? Or is having $plugin_id & $plugin_definition everywhere the common pattern?

yched’s picture

StatusFileSize
new6.29 KB
new6.86 KB
PASSED: [[SimpleTest]]: [MySQL] 57,285 pass(es).
[ View ]

The params received in __construct() can be named whatever we want.
The only constraint is, if we want to be able to use PluginBase's implementations of getPluginId() / getPluginDefinition(), then we need to place the injected values in $this->pluginId / $this->pluginDefinition class members.

We can also use class members named completely differently, but then we can't extend PluginBase, and need to reimplement PluginInspectionInterface methods ourselves on top of our own property names. I'd rather avoid that, the inconsistency would add confusion IMO.

Updated patch uses the var names proposed in #7.

fago’s picture

In general, that names would work for me. But then, I just realized $data_type is now duplicating $definition['type'].

However, I just realized that this will add two additional object properties to each typed data object, which results in additional memory usage. I don't recall the numbers any more, but I think it was significant :/ A quick check on a patch with loads of entities (300 comments?) would be a good idea I guess.

So, instead of doing that I guess we should come up with an optimized implementation. plugin-id? -> that's equal to getType(). plugin-definition - couldn't we look that up via the manager? We'll have to inject it anyway, as other methods use it already.

yched’s picture

couldn't we look that up via the manager? We'll have to inject it anyway, as other methods use it already

That's how PluginBase::getDefinition() worked until #1875182: Pass plugin definition to plugin instances instead of discovery object. Was hell.
The definition arrays are not physically copied, PHP's copy-on-write optimizes that (and those are never written to, just read from).

yched’s picture

StatusFileSize
new7.67 KB
new7.23 KB
PASSED: [[SimpleTest]]: [MySQL] 57,418 pass(es).
[ View ]

Removes the extraneous $date_type param, and adds custom implementations of PluginInspectionInterface

yched’s picture

Devel numbers on a node page with 260 comments:
HEAD: 1172.8 ms. Memory used at: devel_boot()=6.25 MB, devel_shutdown()=21.63 MB, PHP peak=26.25 MB
Patch: 1163.21 ms. Memory used at: devel_boot()=6.26 MB, devel_shutdown()=22 MB, PHP peak=27 MB

That's +1.7% Mem at shutdown, +3% peak

yched’s picture

Doh, I was not displaying all the comments.
HEAD: 2759.65 ms. Memory used at: devel_boot()=5.74 MB, devel_shutdown()=46.38 MB, PHP peak=56.25 MB.
Patch: 2727.11 ms. Memory used at: devel_boot()=5.75 MB, devel_shutdown()=47.74 MB, PHP peak=57.75 MB.

+2,9% at shutdown, +2,7% peak

effulgentsia’s picture

StatusFileSize
new1.13 KB
PASSED: [[SimpleTest]]: [MySQL] 57,628 pass(es).
[ View ]

So here's the alternative that I think was suggested in #9. Given that TypedData is already accessing the manager in that way in its other methods, I think this is ok for now. However, like yched, I prefer #11 if fago is ok with it.

I got different numbers than #13 though for a node with ~250 comments viewed as root user. Each run I had to refresh about 3 times before the memory numbers stopped varying:

HEAD:
Page execution time was 5491.79 ms. Memory used at: devel_boot()=4.06 MB, devel_shutdown()=23.68 MB, PHP peak=32 MB.

#11:
Page execution time was 5567.52 ms. Memory used at: devel_boot()=4.07 MB, devel_shutdown()=23.79 MB, PHP peak=32.25 MB.

#14:
Page execution time was 5492.87 ms. Memory used at: devel_boot()=4.07 MB, devel_shutdown()=23.69 MB, PHP peak=32 MB.

So, #14 is a little more performant, but probably only because getPluginDefinition() isn't being called on it yet. That can change with #1969728: Implement Field API "field types" as TypedData Plugins.

In short, I'd be fine with whichever one gets RTBC'd that helps #1969728: Implement Field API "field types" as TypedData Plugins most.

fago’s picture

#11 has some unnecessary whitespace:

+++ b/core/lib/Drupal/Core/TypedData/TypedData.php
@@ -64,6 +76,20 @@ public function getType() {
+  }
+  ¶
+  /**

Very interesting comparisons! It's interesting that the results of effulgentsia are better. Maybe, did you use PHP 5.4 vs yched (and me) PHP 5.3 ?

The definition arrays are not physically copied, PHP's copy-on-write optimizes that (and those are never written to, just read from).

I know - still, it's just the additional object property that results in more memory being used - its content doesn't matter.

So, #14 is a little more performant, but probably only because getPluginDefinition() isn't being called on it yet. That can change with #1969728: Implement Field API "field types" as TypedData Plugins.

I guess that's also because of less stuff being assigned - this code runs through very often. Also, once the manager becomes injected, I don't see a reason to repeat the type definition in the class. We already have problems like #1875182: Pass plugin definition to plugin instances instead of discovery object - but we'll have to solve it at least for the typed data and entity managers in order to inject dependencies into fields and entities anyway.

So - given #14 is simple, faster and uses less memory and we really have to watch performance I think we should go with this one. If field types start using getPluginDefinition() very often, we can still optimize it more.
Thus, I'd call #14 RTBC - if that works for you?

yched’s picture

My tests were done with 5.3, yes.

I'm fine with #14.
(+, #11 still has a @todo that's above my head in LocaleTypedConfig)

But we really need to solve #2004282: Injected dependencies and serialization hell if we intend to actually inject the manager...

yched’s picture

Status:Needs review» Reviewed & tested by the community

RTBC then, #14 is not my patch :-)

effulgentsia’s picture

Yay. Glad there's agreement.

It's interesting that the results of effulgentsia are better. Maybe, did you use PHP 5.4 vs yched (and me) PHP 5.3 ?

Yes, my numbers are for PHP 5.4.10.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 94b0b70 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary:View changes

link to the main issue, not the helper issue