In EntityNG text fields started using $item->processed instead of $item->safe_value. We need to make sure the prepareCache logic and the all the widgets/formatters use the same key. I'm not sure there are good reasons for changing the key, so I guess it should stay safe_value and safe_summary, i.e. renamed the computed EntityNG properties.
Then, the prepareCache() logic needs to be aligned such that the cached value can be set with NG API and it becomes cleared via onChange().
Comment | File | Size | Author |
---|---|---|---|
#32 | text-processed-caching-2026339-32.patch | 18.02 KB | Berdir |
#32 | text-processed-caching-2026339-32-interdiff.txt | 926 bytes | Berdir |
#30 | text-processed-caching-2026339-30.patch | 18.2 KB | Berdir |
#30 | text-processed-caching-2026339-30-interdiff.txt | 3.98 KB | Berdir |
#28 | text-processed-caching-2026339-28-test-only.patch | 6.02 KB | Berdir |
Comments
Comment #1
fagoAlso, #1969728: Implement Field API "field types" as TypedData Plugins makes TextProcessed::setValue() to skip the read-only exception. Once it gets committed, this should be solved as part of this clean-up.
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedSince we already have 'processed' and 'summary_processed' defined as our computed properties, I think we should stick with that, and remove all usages of 'safe_value' and 'safe_summary'. There's not that many of them.
Drupal\text\TextProcessed already implements the logic of text_sanitize(), so this patch should also remove that function. However, note that we need to add the nl2br() to the implementation of TextProcessed::getValue().
@fago: Can you roll the patch to do this? I'm a little lost as to what the correct way of setting (in order to cache) a computed property is.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedRetitling
Comment #4
BerdirThe safe value caching is currently broken, seeing a lot of uncached calls to check_markup(), making that the third-slowest function (right after queries and cache prepareItem()) on the frontpage with 10 nodes.
Raising to major and adding Performance tag.
Comment #5
BerdirThis seems to be working pretty well, actually. And the performance gains are impressive, which has two reasons:
- safe_value worked. But getValue() also called processed->getValue(), multiple times, and that was completely uncached.
- TextProcessed also used to fetch references to the text and format properties and therefore instantiated them. That circumvents all the work that went into avoiding exactly that.
ab -n 100 -c 1 on frontpage with 10 nodes with a body
8.x:
Requests per second: 6.42 [#/sec] (mean)
Time per request: 155.842 [ms] (mean)
With patch:
Requests per second: 6.96 [#/sec] (mean)
Time per request: 143.652 [ms] (mean)
According to xhprof, 12% less function calls and 8.7% less wall time, which is quite similar to the values reported by ab.
Just a few more crazy things like this and then there might still be hope that D8 will only be slow and not dead slow ;)
Comment #7
Berdir#5: text-processed-caching-2026339-5.patch queued for re-testing.
Comment #9
yched CreditAttribution: yched commentedThere might still be code that tries to read the "old" properties (safe_body, safe_summary) ?
Maybe this explains the fails ?
Should we really remove those ? the code still assigns $this->text / $this->format
While we're in there, could we switch to $field_definition = $field->getFieldDefinition();
$field_definition->getFieldSetting('text_processing'); ?
According to #2061331-20: Add getEntity() helper in FieldInterface / FieldItemInterface and following, it's wrong to consider that the item language is the parent entity language. Current code is wrong too, but can we add a @todo on that issue here ? Would make it easier to later fix the places where we make that wrong assumption.
I'm not sure that this code branch in EntityStorageControllerBase::invokeFieldMethod() doesn't still run, but you probably know better than me :-)
+ doesn't apply anymore :-(
Comment #10
BerdirThanks.
1. I didn't find any when searching for it, My guess is more that it's about stale data, as processed is now kept on the class.
2. Oh, forgot to remove that part. The problem there is that get('text') and get('format') instantiate the property object, which completely defeats the purpose of that lazy-instantiation stuff. That wasn't properly updated when we introduced that. Experimented a bit with assigning the value but that just makes on change handling harder. They're not used anymore.
3. Was wondering about that, yes, will update it.
4. Will add a todo.
5. There are various cases where property objects get destroyed/set again, at least right now with the bc decorator. So we need to be able to set the processed value again or the caching doesn't actually work.
Comment #11
yched CreditAttribution: yched commented5. Oh, right sorry, I misread the code. I though you had uncommented the exception back, but you removed it altogether. Fine :-)
Comment #12
BerdirOk, this should behave better :)
- Switched to use NULL as "not yet calculated" instead of FALSE, because that's what gets set when you call $item->setValue() and processed is not explicitly defined.
- Removed unecessary setContext() stuff.
- Implemented onChange(), will need to update the DUBT tests to make sure that this works as expected.
- Implemented yched's feedback.
Comment #14
yched CreditAttribution: yched commentedNitpicks:
Nitpick: maybe we could inline the $text_processing var & switch to the "$this->format" syntax.
(feel free to ignore me, those lines are not actually touched by the patch...)
weird phpdoc ?
Ok, sorry, my bad - $field->getFieldSetting('text_processing') can be used as shorthand.
And actually, $this->parent->getFieldSetting('text_processing') should work too, and then we don't need the $field and $entity vars (although maybe $item = $this->parent might make the code clearer)
Comment #15
BerdirActually, $field->getFieldSetting() was what I tried first, and it was a fatal error, that doesn't seem to exist? $this->parent->getFieldSetting() should work, yes, but I still need $entity for the broken language.
Comment #16
yched CreditAttribution: yched commentedYes, sorry, got confused, Field / FieldItem::getFieldSetting() is an internal protected helper, so yup, here we need to go through getFieldDefinition().
Still the method could be streamlined like so:
Comment #17
BerdirYes, that looks nice. Also fixed the tests and the docblock. I don't understand why this previously printed a \n at the end. Removed it from the tests for now unless someone knows if/why it's required.
Comment #18
yched CreditAttribution: yched commentedThe previous code used the raw unsanitized 'body' value. Using the processed one instead means filters get applied, and I guess _filter_autop() adds a newline ?
I guess we could leave the test untouched (comment_body->value should still work, right ?). But the new code makes more sense (it's the processed version that should be in the HTML, the current test is just lucky that they kind of match) so this change looks fine.
That one seems weirder. Both the old and the new code use the unprocessed body value, and it shouldn't be different before and after the patch ?
+ for consistency with the other test, should this one move to ->processed as well ?
Other than that, the patch is RTBC for me, but it would be best if we got to the bottom of this...
Comment #19
BerdirThere's no filter format, so it runs through nl2br(check_plain($text)), both of them do, both of them failed with ->value and ->processed before the \n removal.
Comment #20
yched CreditAttribution: yched commentedNo filter format ? There should be, comment_body is supposed to be filtered text.
Maybe the weirdness is related to what happens when a formatted text has format NULL ?
Comment #21
BerdirYeah, I don't know, but I was seeing a lot of calls going through the no-format route, have to check again. Agreed that it's supposed to have a format and that could be the difference.
Tagging for rocketship focus
Comment #22
yched CreditAttribution: yched commentedSo yes, the issue comes from the fact that those tests create comments without a format, and the patch changes how a "formatted text with a NULL format" is output.
- In HEAD, text_sanitize() does
so : check_markup($value, NULL), which applies filter_fallback_format(), that's 'plain_text' on a default install, which includes auto_p filter.
So:
'some value'
->'<p>some value</p>'
- with patch, TextProcessed::getValue() does
So it's the second branch that runs if format is NULL:
'some value'
->'some value'
Not sure which one is best in absolute, but we probably don't want to change the behavior of NULL format in this patch ?
If we stick to the current behavior and remove "&& $item->format" in TextProcessed::getValue(), then no need to change CommentAttributesTest & StandardProfileTest here.
Granted, those tests are buggy, they should:
- create comments with an actual format
- base their tests on the processed values rather than the raw value
But I'd say fixing them seems outside the scope of this specific patch.
Comment #23
BerdirNice find. agreed, let's keep the existing behavior. Removed that check. Let's see if those tests pass now.
There's a @todo in TextItemBase::applyDefaultValue() that currently defaults to NULL. We didn't have access to the field settings when we did that, but we should now use the field settings to do that correctly. Separate issue.
Comment #24
yched CreditAttribution: yched commentedSweet :-)
(+ awesome fixing a 10% perf regression...)
Yes, discussing NULL format and maybe filling in a default when none is provided in incoming values might be a good thing.
Comment #25
BerdirLooks like either my changes or recent changes in HEAD broke the caching.
Comment #26
BerdirOh, actually it still works in HEAD but I was testing with the BC decorator removed, there it breaks.
We'll need to do this in the other issue:
Still, I think we should add some test coverage on a text item to make sure that process value setting and getting works as expected, as do updates on other properties.
Comment #27
yched CreditAttribution: yched commentedSide note: just stumbled on this change notice, will need to be deleted after this goes in.
Comment #28
BerdirOk. Had to get a bit creative with the tests but this gives us important test coverage for when we're going to change how that works when we change it in other issues like the BC removal or entity-level caching.
Comment #29
yched CreditAttribution: yched commentedTests look good, cool ! Only formatting / nitpick remarks:
Two dots
Nope :-)
{@inheritdoc} ?
Commented out ?
Ubernitpick: move the helper at the end, not between two test classes that both use it ?
Indentation is off
Comment #30
BerdirThanks for the review.
- Removed second . ;)
- Yeah, using the pattern that we start using for php unit tests, where we actually reference the class we're testing instead of some random description
- Modules to enable is the current pattern for that, I don't want to break with that here...
- Yeah, I actually want a format that does nothing, didn't update it properly though
- Moved the helper method down
- Fixed indentation.
Comment #31
yched CreditAttribution: yched commentedYay. RTBC if green.
Comment #32
BerdirOh, I think we can add that @todo now :)
Comment #33
BerdirSetting back to needs review.
Comment #34
yched CreditAttribution: yched commentedHah, true.
Comment #35
catchCommitted/pushed to 8.x, thanks!
Needs a change notice.
Comment #36
BerdirCreated https://drupal.org/node/2090719
Comment #37
BerdirAlso added a reference to https://drupal.org/node/1975504.
Comment #38
yched CreditAttribution: yched commentedLooks good.