In #1983554-30: Remove BC-mode from EntityNG Berdir wrote:

The EntityResolverTest is ugly, that's an endless loop because the entity apparently references itself and getValue(TRUE) there causes it to be loaded again and again. And we don't want to cache the computed entity referenced, but we do want to cache the computed processed texts for example.

Maybe introduce a new method on Field(Item)Interface, something like getCache() or getCacheValues() ? And introduce a new cacheable key for property definitions?

CommentFileSizeAuthor
#57 field_cache_computed_properties-2083785-57.patch21.05 KBBerdir
#55 interdiff-2083785-54-55-do-not-test.diff1.11 KBdas-peter
#55 field_cache_computed_properties-2083785-55.patch22.78 KBdas-peter
#54 interdiff-2083785-52-54-do-not-test.diff2.78 KBdas-peter
#54 field_cache_computed_properties-2083785-54.patch22.79 KBdas-peter
#52 field_cache_computed_properties-2083785-52.patch22.8 KBBerdir
#52 field_cache_computed_properties-2083785-52-interdiff.txt4.03 KBBerdir
#50 field_cache_computed_properties-2083785-50.patch22.71 KBBerdir
#50 field_cache_computed_properties-2083785-50-interdiff.txt1.42 KBBerdir
#45 field_cache_computed_properties-2083785-45.patch22.7 KBBerdir
#45 field_cache_computed_properties-2083785-45-interdiff.txt3.48 KBBerdir
#40 field_cache_computed_properties-2083785-40.patch22.58 KBBerdir
#40 field_cache_computed_properties-2083785-40-interdiff.txt1.38 KBBerdir
#39 field_cache_computed_properties-2083785-39.patch22.54 KBBerdir
#39 field_cache_computed_properties-2083785-39-interdiff.txt1.27 KBBerdir
#37 field_cache_computed_properties-2083785-37.patch22.31 KBBerdir
#35 field_cache_computed_properties-2083785-35.patch25.92 KBBerdir
#35 field_cache_computed_properties-2083785-35-interdiff.txt8.53 KBBerdir
#29 field_cache_computed_properties-2083785-29.patch23.32 KBBerdir
#29 field_cache_computed_properties-2083785-29-interdiff.txt5.58 KBBerdir
#27 field_cache_computed_properties-2083785-27.patch17.75 KBBerdir
#18 field_cache_computed_properties-2083785-18-test-only.patch2.03 KByched
#18 field_cache_computed_properties-2083785-18.patch7.29 KByched
#18 interdiff.txt3.15 KByched
#15 field_cache_computed_properties-2083785-15-test-only.patch1.91 KByched
#15 field_cache_computed_properties-2083785-15.patch6.24 KByched
#11 field_cache_computed_properties-2083785-11.patch3 KByched
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Marking critical as this is blocking #1983554: Remove BC-mode from EntityNG .

yched’s picture

Title: Add support for determining which field values are cacheable » Add support for determining which field properties are cacheable

adjusting title

catch’s picture

Why are we allowing an entity to reference itself?

plach’s picture

That's a good question :)

However the problem here is that we are serializing full entity objects, no matter which ones.

yched’s picture

Copy/pasting what I wrote in #1983554-35: Remove BC-mode from EntityNG :

Does text 'processed' really have to be a 'computed' property ?
We'd need to make sure it's always present and to-date with the raw text values, but if it was a regular property autopopulated on load / read from cache / value change, then maybe we could stick with "cached values do not include computed properties" ?

Berdir’s picture

#1983554: Remove BC-mode from EntityNG currently removes support for caching computed properties completely, which introduces a serious performance regression that was fixed a short time ago in #2026339: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property. The test coverage added there does not correctly catch that case (as it injects the cache manually), so we need additional test coverage to verify the creation of caches.

This already is critical (due to the known performance regression, could also be classified as a bug).

Tagging as API change, not exactly sure yet what it will require (or if anything at all), but it shouldn't be anything big, possibly only an API addition/additional method on an interface.

@yched: Not sure. processed seems like the perfect example of a computed property, that is computed based on other properties and not stored anywhere (except cache).

@catch: Apart from there being nothing wrong with a self-reference, that's just an example. The same would happen if you have a back reference like A -> B, B -> A. And even that is irrelevant, we don't want to cache referenced entity objects in the first place, not as part of another entity, as @plach said. That's the same craziness as we already have, where we end up caching usernames in nodes and comments.

yched’s picture

Yeah, agreed that "computed, cacheable" & "computed, not cacheable" are both valid cases we should support.

Adding a 'cacheable' flag in getPropertyDefinition() (probably defaulting to FALSE ?) looks easy, the issue is "how so we get only the set of properties we want when building data for the cache".

Maybe we could simply not use getValue(TRUE) there, and instead manually iterate on properties and individually fetch each one that is not 'computed + non cacheable'. I mean, other than building field cache data in loadFieldItems(), I'm not sure we have other use cases that would make it worth scratcing our heads to make that fit into some combination of getValue() params ?

Berdir’s picture

Yes, that's exactly what I was considering too. It would make cache writing a bit slow but it will only be called for writing caches for cacheable entities, which shouldn't happen often. Eventually when we start caching all fields of an entity, we could put that logic in __sleep() and just serialize/unseralize $entity.

Talking a bit with @msonnabaum in IRC led to the conclusion that a marker interface or a method could be an alternative to a magic definition key but the problem is that we need to check the magic computed key anyway (or instantiate all non-computed properties too), so checking a second is easier than getting the object and check it with instanceof.

yched’s picture

I guess we could even start with getValue(FALSE) to get all non computed properties in one shot, and then iterate / fetch individually just for computed cacheable ?

__sleep() : sounds sexy :-) I wonder if that would be good enough for all the cases where we serialize entities though. (e.g entity forms...)

yched’s picture

Using a marker interface or a method means cacheabilty is per property clsss, not per individual properties. I guess there could be cases for two properties using the same class, but one cacheable and not the other.
So +1 on staying with a flag in the definition.

yched’s picture

Status: Active » Needs review
FileSize
3 KB

Something like this ?

Berdir’s picture

Looks about right to me :)

We should probably add that key to TypedDataManager::create(), as weird as that is, that seems to be where all those keys are defined. Or do do you have a better suggestion? (As it's only relevant to field items, that's where it would make sense, but that doesn't define getPropertyDefinitions(). I know what you're going to say ;))

And some tests would be good to have, I guess we can put them in TextSummaryItemTest as well, basically the inverse of the existing tests there, instead of injecting a fake cache, make sure that loading an entity resulted in the correct caches being written.

msonnabaum’s picture

Could we at least encapsulate the knowledge of the those flags in methods? Getting an array back and then making decisions based on its keys is very procedural :(

Berdir’s picture

Methods on what? We'd have to instantiate all properties of a field item to be able to call a method on them (or check an interface), that messes with the lazy-instantiation logic of those classes.

yched’s picture

Actually, this should let us do less iterations in the most frequent case where there are no cacheable computed properties.

+ Added doc in TypedDataManager::create() - I won't say what you know I'm going to say since you know I'm going to say it :-)
+ updated testProcessedCache()

@msonnabaum: agreed with @Berdir, + this is consistent with how the rest of core deals with definitions from getPropertyDefinitions(), changing that seems way out of the scope of this issue.

Berdir’s picture

+++ b/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php
@@ -119,26 +119,26 @@ function testProcessedCache() {
-    // Inject values into the cache to make sure that these are used as-is and
-    // not re-calculated.
-    $data = array(
+    // Check that the processed values are correctly computed.
+    $this->assertEqual($entity->summary_field->processed, $value);
+    $this->assertEqual($entity->summary_field->summary_processed, $summary);

I'd suggest to add additional tests, not replace them. There are two different things here, a) that things are written into cache (currently not covered by tests), b) that writing them back from the cache actually works as expected and they're not re-computed.

Status: Needs review » Needs work

The last submitted patch, field_cache_computed_properties-2083785-15.patch, failed testing.

yched’s picture

@Berdir: right, added back the existing tests.

The warnings in NodeValidationTest were caused by a non-set body field.
Empty fields still have an item object with NULL properties, and the code populating the cache in loadFieldItems() tried to get the processed values for that.
This then failed in weird ways because check_markup() with a NULL format triggers filter_fallback_format(), which requires the filter config to be installed, which is not the case in NodeValidationTest (DrupalUnitTest).

Updated patch includes two fixes (each fixes the fails separately, but each make sense on their own IMO):
- run $items->isEmpty() before trying to put stuff in the cache
- optimize the case of NULL / empty string in TextProcessed (D7's text_field_load() had a similar stopgap optim)

yched’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Performance, -Needs tests, -API change, -sprint, -Entity Field API

The last submitted patch, field_cache_computed_properties-2083785-18.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
Issue tags: +Performance, +Needs tests, +API change, +sprint, +Entity Field API
Berdir’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests, -API change

This looks good to me. Looking the code and xhprof, I think this pretty well optimized and it only runs when entities are put into the cache, compared to actually loading the values, this takes no time at all.

@msonnabaum was talking to me about possibly moving the "get cacheable fields into a methods" of FieldInterface $items, but I'm not sure about that. It's not really business logic to me, it's storage logic. The business logic is what each computed property does. Such a method would be on FieldInterface (or ConfigFieldInterface), where it would be visible to anyone (e.g. widgets and formatters, which receive FieldInterface $items).

If a method, then I rather think it would be a protected helper method in FieldableEntityStorageControllerBase, available only to subclasses of that.

Either way, I'd vote to get this in like this and discuss making it re-usable/encapsulate it in a follow-up issue. I'd like to see for example a MongoDB storage controller implementing this logic (not sure where, possibly write cacheable computed properties right into the storage?) and then check if there is something that can be re-used?

yched’s picture

Either way, I'd vote to get this in like this and discuss making it re-usable/encapsulate it in a follow-up issue. I'd like to see for example a MongoDB storage controller implementing this logic (not sure where, possibly write cacheable computed properties right into the storage?) and then check if there is something that can be re-used?

Agreed. #597236-94: Add entity caching to core points out refactorings that should happen around the field cache - with similar questions like "what would Mongo storage do ?". Let's see where they put us and reconsider if / where to put a helper method then.

fago’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -103,7 +103,11 @@ public function createInstance($plugin_id, array $configuration, $name = NULL, $
+   *   - cache: (only supported for FieldItem objects) A boolean specifying

It's not the whole field item object that's cached, but the field item properties - not? So the docs are misleading here. -> Needs work.

Howsoever, extending this key here which only makes sense in the field item property context sucks - imho we should avoid doing something like that.

So why not go with a method on the field item class that controls which properties can be cached? By default it would be all non-computed properties, but implementations can easily override it to add stuff like the processed property.

It's not really business logic to me, it's storage logic. The business logic is what each computed property does.

We've got prepareCache() on field item classes already as well as other storage related stuff like presave(), insert() and update() methods. It's not storage-engine dependent logic though so imho it's ok.

yched’s picture

extending this key here which only makes sense in the field item property context sucks - imho we should avoid doing something like that.

Agreed that is sucks, but "everything is a TypedData" :-). getFieldDefinition() is the place where you provide the metadata about your properties. Having to declare the specific "is cacheable" metadata in a different method seems sad.

However, it's true that there might be something to be done with FieldItem::prepareCache() - what we're doing here is exactly "preparing the cache". The problem is that what we currently do in prepareCache() is also alter the current $item by preparing the loaded values for the entity in the current request. We could repurpose prepareCache() slightly to both return an array of data to cache (a base implementation would return the "non computed + cacheable computed") and alter the $item properties if needed. Might be a bit confusing though.

This would also mean we can stop special casing prepareCache() in a separate PrepareCacheInterface, since there would always be a prepareCache implementation and we would create the Field/FieldItem objects anyway on cache miss - but that is what the current patch is doing anyway, and I don't see how we can avoid that if we want to access computed properties to put them in the cache...

andypost’s picture

something to be done with FieldItem::prepareCache()

we can stop special casing prepareCache() in a separate PrepareCacheInterface

Also I'd like to mention about the need to load multiple calculated properties for: comment statistics and history markers

Berdir’s picture

Status: Needs work » Needs review
FileSize
17.75 KB

Wow. See, this is why fago is the maintainer and am I only co-maintainer ;)

I *really* like this.. Yes, the separate interface for prepare cache was a nice idea, but right now unnecessary as we initialize anything anyway, especially on a cache miss (we want to avoid doing it on a cache hit but we don't either)

Two remarks:
1. Not sure if I got the legacy implementation right, the only implementation left is field_test_field_load (please RTBC that :)), which will probably fail anyway due to the second point.
2. This will only call the prepare cache stuff on cached entity types. That will very likely cause some fails, hopefully only in test_field, because it's doing weird stuff in there. We could still call the method for entity types that don't use the cache, or we could just document it to work like this? After all, they're class now, can compute on-demand and what not.. Seems better to not try to calculate stuff in advance that we might not need and won't cache?

Status: Needs review » Needs work

The last submitted patch, field_cache_computed_properties-2083785-27.patch, failed testing.

Berdir’s picture

Ok, we had a look at those test fails:

- As expected, the additional key stuff failed as it's only called when an entity is cached and the logic explicitly checks loading revisions too. That's no longer necessary, as you can implemented computed properties if you need something like that. So we agreed that this is by design and removed that assertion. Also changed another test to use a cached entity type.
- DateTimeItem is ugly, as it's relying on a date property that is not defined. Added some hacks to try to get it to work somehow, but doesn't quite work yet. Will need a computed property ...

yched’s picture

Yeah, those date fixes spilling in the various widgets & formatters are a bit ugly right now. We might push that to a followup, but then at least we should denote those places with a @todo marker

fago’s picture

I'm not sure about removing the optional interface + instantiating all the objects during case miss.

Yes, that is what is happening right now - but I think we should/can fix/optimize the code to not do that. It should be perfectly fine to load all the values, build a flat array of values and pass it on to entity object + cache things. The only field item objects we really have to instantiate are those with custom-caching behaviour -> so we check for the interface.

Also, I do not think the possibility to customize caching should be limited to configurable fields as field caching will have to be replaced by entity caching - #597236: Add entity caching to core.

Status: Needs review » Needs work

The last submitted patch, field_cache_computed_properties-2083785-29.patch, failed testing.

Berdir’s picture

Yeah, I think we should fix date properly here, not exactly sure how yet.

Yes, that is what is happening right now - but I think we should/can fix/optimize the code to not do that. It should be perfectly fine to load all the values, build a flat array of values and pass it on to entity object + cache things. The only field item objects we really have to instantiate are those with custom-caching behaviour -> so we check for the interface.

Yeah, but it's the field item that implements the interface, not the field. I don't see how that could possibly work.

Also, I do not think the possibility to customize caching should be limited to configurable fields as field caching will have to be replaced by entity caching - #597236: Add entity caching to core.

Absolutely. But not here. So adding that method here and then move it up to FieldItemInterface over there...:)

fago’s picture

Yeah, but it's the field item that implements the interface, not the field. I don't see how that could possibly work.

That's the way it already works in HEAD? It just gets the class and checks for the interface - cannot we keep doing that?

Berdir’s picture

Yes, but we either need to call getValue() anyway so doing this separately seems useless to me, it would be a condition of calling getValue() (on the field, but that wouldn't change anything, the items are still created) or prepareCache(). While we should possibly update the documentation or even rename the method, this seems way easier.

And even after we have a single $values array, we still need to instantiate the entity object to get those that are computed and we very likely need to run his code after we call the load hooks, so I expect it could be quite confusing if we use the original $values array and mix in stuff from other calls.

I can change it back if you insist on it, but I'm not sure to what exactly, to be honest? Mixing getValue() and prepareCache() (or getCacheableValues() or whatever) seems confusing to me.

Added a datetime_computed field, I think our getDateTime() stuff is broken and should return a Drupal date time object, I think the error comes from there, but the I don't know how we could handle the storage stuff anyway.. that's weird.

Berdir’s picture

Status: Needs review » Needs work

Conclusion from the discussion with @fago:
- We will keep the interface, that it's easier to optimize it later again
- Rename prepareCache() to getCacheValues()
- We don't need bother with checking the interface on the class name right now, because we lost some hidden optimization when we removed the BC decorator ( I think that caused some of the confusion in here)
- We'll however keep the simple logic for now

We should also try to get rid of getValues(TRUE), theme function #items should switch to use the field item list, the legacy stuff is going away, the only remaining case that uses it is then some sorting and I don't think that's necessary there.

Will re-roll tonight.

Berdir’s picture

Status: Needs work » Needs review
FileSize
22.31 KB

Let's see if this works. Interdiff would probably bigger than the patch, so useless.

Status: Needs review » Needs work

The last submitted patch, field_cache_computed_properties-2083785-37.patch, failed testing.

Berdir’s picture

Accidently implemented writing to cache for all fields, not just configurable :)

I hope the other test fails are a testbot problem.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: -Performance, -sprint, -Entity Field API

The last submitted patch, field_cache_computed_properties-2083785-40.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Performance, +sprint, +Entity Field API

The last submitted patch, field_cache_computed_properties-2083785-40.patch, failed testing.

yched’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Field/PrepareCacheInterface.php
    @@ -10,8 +10,9 @@
    + * If a field type implements this interface, the getCacheValue() method will be
    + * invoked instead of getValue().
    + *
    

    Suggestion :
    "If a field type implements this interface, this method will be used instead of the regular getValue() to collect the data to include in the cache of field values."

    + extraneous empty line

  2. +++ b/core/lib/Drupal/Core/Entity/Field/PrepareCacheInterface.php
    @@ -25,7 +26,13 @@
    +   *
    +   * The method is also only called if the entity has field caching enabled. The
    +   * recommended way to implement it is by providing a computed field item
    +   * property that an also hold a value set with setValue(). See
    +   * \Drupal\text\Plugin\field\field_type\TextItemBase and the corresponding
    +   * computed property Drupal\text\TextProcessed for an example.
    

    That whole docblock can be refactored IMO.
    Suggestion:

      /**
       * Returns the data to store in the field cache.
       *
       * This method is called if the entity type has field caching enabled, when an
       * entity is loaded and no existing cache entry was found in the field cache.
       *
       * This method should never trigger the loading of fieldable entities, since
       * this is likely to cause infinite recursions. A common workaround is to
       * provide a base formatter class implementing the prepareView() method
       * instead.
       *
       * The recommended way to implement it is to provide a computed field item
       * property that can accepts setting a value through setValue(). See
       * \Drupal\text\Plugin\field\field_type\TextItemBase and the corresponding
       * computed property Drupal\text\TextProcessed for an example.
       */
    

    (the last paragraph is can still be enhanced IMO, but this is what I have for now)

  3. +++ b/core/modules/datetime/lib/Drupal/datetime/DateTimeComputed.php
    @@ -0,0 +1,74 @@
    +    catch (\Exception $e) {
    +      // @todo Handle this.
    +    }
    

    Should we dig on this here, or in a followup ?

  4. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/field_type/DateTimeItem.php
    @@ -114,24 +124,16 @@ public function instanceSettingsForm(array $form, array &$form_state) {
    -  public function prepareCache() {
    +  public function getCacheValue() {
    +    $values = $this->getValue();
    

    Not sure $values is the best param name here. It's about return data to cache.
    maybe prepareCacheData() / $cache_data ?

  5. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/field_type/DateTimeItem.php
    @@ -142,4 +144,19 @@ public function isEmpty() {
    +    // Notify the parent of changes.
    +    if (isset($this->parent)) {
    +      $this->parent->onChange($this->name);
    +    }
    

    Do we really need to provide code for this ? isn't it provided in a parent implementation ?

  6. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/formatter/DatetimeDefaultFormatter.php
    @@ -103,11 +103,8 @@ public function viewElements(FieldInterface $items) {
    +      if ($item->date) {
    

    'date' is a computed property, right ? so when would $item->date == FALSE ?

  7. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/widget/DatetimeDatelistWidget.php
    @@ -126,7 +126,7 @@ public function formElement(FieldInterface $items, $delta, array $element, array
    +    if ($items[$delta]->date) {
    

    same here (and in DateTimeDefaultWidget)

  8. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.php
    @@ -344,4 +344,11 @@ public function getUploadValidators() {
    +  public function prepareCache() {
    +    return $this->getValue();
    +  }
    

    Why do we need that implementation if it just does as the "default" behavior ?

Berdir’s picture

Thanks, great review as usual :)

1. Fixed.
2. Updated accordingly. Reads quite fine to me like this.
3. I just moved this from the previous prepareCache() implementation. I don't think it's something we need to fix here.
4. Not sure, prepareCacheData() a) returns a new term (data, we use values elsewhere) and b) doesn't describe that it returns something IMHO
5. Yes, we probably can, makes sense.
6. & 7. Yes, but the field item can be empty.
8. Left-over, forgot to delete this, removed.

andypost’s picture

+++ b/core/modules/datetime/lib/Drupal/datetime/DateTimeComputed.php
@@ -0,0 +1,74 @@
+    try {
+      $date = DrupalDateTime::createFromFormat($storage_format, $value, DATETIME_STORAGE_TIMEZONE);
...
+    catch (\Exception $e) {
+      // @todo Handle this.

needs issues link

Status: Needs review » Needs work
Issue tags: -Performance, -sprint, -Entity Field API

The last submitted patch, field_cache_computed_properties-2083785-45.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Performance, +sprint, +Entity Field API

The last submitted patch, field_cache_computed_properties-2083785-45.patch, failed testing.

Berdir’s picture

Wrong interface check.

yched’s picture

@Berdir: yes, sorry, agreed about *prepare*CacheData() being less clear, getCacheData() was what I actually had in mind.

Then I think the method name *not* using "Value/Values()", as we do in other places, is actually a feature :-), because what we do here is different from what we do in other places. We're building data for the cache, not values for the runtime FieldItem.
So, still +1 on getCacheData() / $cache_data (or $data ?) as a "typical" var name in implementations.

Berdir’s picture

yched’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php
    @@ -90,12 +90,13 @@ protected function loadFieldItems(array $entities, $age) {
    +                  // If the field item needs to be prepare the cache data, call
    

    typo / language - ".. needs to prepare..." ?

  2. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/field_type/DateTimeItem.php
    @@ -124,7 +124,7 @@ public function instanceSettingsForm(array $form, array &$form_state) {
    +  public function getCacheData() {
         $values = $this->getValue();
    
    +++ b/core/modules/text/lib/Drupal/text/Plugin/field/field_type/TextItemBase.php
    @@ -70,7 +70,7 @@ public function isEmpty() {
    +  public function getCacheData() {
         $values = $this->getValue();
    

    s/$values/$data ? :-)

Just nitpicking - this is RTBC anyway.

das-peter’s picture

Addressed ycheds nitpicks ;) Should remain RTBC.

das-peter’s picture

yched just told me there's another nitpick in DateTimeItem - so here's the adjustment.
Still keeping it RTBC.

yched’s picture

;-)

Berdir’s picture

That's what I get for messing with the same functions in 3 (or 4?) different criticals :)

Re-roll, re-removed the invoke prepare cache method. Patch is smaller because there's less code to remove.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks great. Committed/pushed to 8.x, thanks!

Berdir’s picture

Issue tags: -sprint

Removing sprint tag.

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

Wim Leers’s picture