Speed up via shortcutting token file_entity_file_load

This can be a major performance hog that I have not seen for the first time.

Especially without static file caching, it can be a major performance hog.

Still needs work - against an older version of file_entity. But just updating the patch a bit for now.

Maybe it passes now?

Nope, it needs re-rolling against the latest file_entity.

Here is a new patch, which patches the remaining token_replace, but changes the API a little bit more.

Looks great to me.

This actually didn't work at all due to a mismatch of variable names. Here is a fix for that.

Not bothering to set this to "needs review" because (like the others) it's currently applied on top of #2473621: Alt and title text don't appear in the correct language when viewing a file in a language other than the current page language rather than against the dev version of File Entity.

RTBC on the interdiff, but works now, so setting actually RTBC status.

Reroll, I think?

Ran into this problem where token_replace was being ran almost six hundred times on a page due to file_entity_set_title_alt_properties(), which was quite daft.

The site's front page went from calling token_replace() 580 times to 26 times with the patch in #14.

A little more stats for this site I rerolled the patch for. The front page has 40 img tags output using File Entity. The page load time (timed using "curl") went from roughly eight seconds to three seconds when I applied the patch.

Committed to dev branches.
I believe we do already have simpletest test coverage for these alt and title tokens.

Thanks

tagged and released as 7.x-2.18

Please let us know if you have problems.

Sweet, thanks @joseph.olstad!

Hi,

I have a problem with this version. The title/alt properties have disappeared in all my images fields. When I go back to the previous version (2.17), everything works correctly and title/alt properties come back.

Thanks

Same problem here. Our image title fields are not printed anymore, like reported in this issue: #2918049: Image field alt and title text overridden by file_entity_alt and file_entity_title

The node edit forms show empty title fields, while the previous values are still stored in the database. If you save theses nodes in this state, the original content will be overridden, either empty or with the new values, while not being visible on node edit.

We will switch back to 2.17 for now.

ok, for now I will revert this commit and release 7.x-2.20 without it.
any objections? be quick

reverted
and released 7.x-2.20

https://www.drupal.org/project/file_entity/releases/7.x-2.20

Hello,

with version 7.x-2.20 the title/alt properties missing problem, has been fixed.

Thanks

ok, good two confirmations that it's fixed with 2.20, that means the above patch needs work, this issue will stay open.
it will not get into the next release unless fixed.

Ok, so lets work on adding test coverage to uncover the problem.

related issues:
wonder if there's an easy way to fix this patch.

1. +++ b/file_entity.file.inc
   @@ -269,41 +269,9 @@ function file_entity_set_title_alt_properties_on_file_fields($entities, $entity_
   -  if (!isset($language)) {
   -    $language = $GLOBALS['language'];
   -  }
   ...
   -  $replace_options = array(
   -    'clear' => TRUE,
   -    'sanitize' => FALSE,
   -    'language' => $language,
   -  );
   ...
   +    $file->title = file_entity_replace_title($file);
   +    $file->alt = file_entity_replace_alt($file);
   

   We need to actually pass this replace_options to the helper functions.

   This is likely the reason why the patch broke.

2. +++ b/file_entity.file.inc
   @@ -269,41 +269,9 @@ function file_entity_set_title_alt_properties_on_file_fields($entities, $entity_
   -        $file->alt = decode_entities($output);
   ...
   -        $file->title = decode_entities($output);
   

   We need to replicate that broken behavior in the two helpers - else we risk XSS problems.

FabianX, if I understand your comment #33 correctly, this patch is what you had in mind?

see interdiff_n2642764-14_to_34.txt
and new patch 34

I'm not sure about the decoded entities part. haven't tested it.

FYI everyone, in addition to patch #34 above, there's also another recent performance patch for file_entity, would appreciate at least one review.
#2966026-2: Performance fix - rename file_entitycache_file_load to file_entity_entitycache_file_load

Performance issues are my pet peeve, would appreciate assistance reviewing these. Feel the need, the need for speed.

I just manually tested patch 34 in combination with patch 2 from the other issue, functionally both work.

And, the alt and title fields token replacement is working, I tested by using a date token , here is the markup generated:

<div class="content">
    <a href="https://localhost/sites/default/files/2018/04/example.jpg"><img typeof="foaf:Image" src="https://localhost/sites/default/files/2018/04/example.jpg" alt="test alt-04/28/2018 - 17:20" title="test title-04/28/2018 - 17:20" width="2880" height="1920"></a>  
</div>

My test installation is using French as the default language.

might be good to have another test with two languages configured and try both languages. But ya, seems that this should work.

Nice work @FabianX!

*EDIT*Here's the two patches I used: I committed the other issue performance patch, 2966027-2

https://www.drupal.org/files/issues/2018-04-28/file_entity-n2642764-34.patch

https://www.drupal.org/files/issues/2018-04-25/file_entity-performance_o...

Ok, I re-tested again this time with english and french file entities, I enabled translation (entity_translation 1.0) on the alt and title fields, translated them, put them in a view, reviewed the values with and without default tokens, everything looks fine to me.

The perplexing part is, I tried the previous version of the patch and it works find too. I'm not sure why people had an issue with this performance patch in the first place.

both of them seem to work for me.

I've tested patch 34 it works fine in a unilingual setup as well and also a bilingual translated site. I tried using views and without views. I switched languages, translated the alt text and title text, switch back and forth clear cache before and after patching. looks ok

The only issue I noticed with alt text and title text is when the token module is disabled, in that case the alt text and title text was not picked up but I think this is normal because we're relying on token to bring this field information in.

I'd like to know what the issue the others brought up, because I am unable to reproduce it with either of the patches above.
patch 34 works for me. Maybe re-reading some of the other threads might shed some light on this.

NOTE: 7.x-2.21 was released yesterday
contains another performance fix here:
#2966026: Performance fix - rename file_entitycache_file_load to file_entity_entitycache_file_load

I did not include patch 34 (above)

It would be nice to also have some more reviews from those that noticed issues last time we tried to fix this performance issue.
#2642764-34: Speed up via shortcutting token file_entity_file_load

We've been using this patch for 2 weeks now and it seems quite fine to us. The performance optimization is noticeable.

Hi @angel.h, when you say noticeable performance improvement, could you please provide performance profiling stats on that before and after patch?

Thanks for reporting, this patch needs tests as it was previously reported to have caused failures , we previously pushed an older version of this patch into a previous release and had to revert it. I made some recommended changes (as recommended by FabianX) but I'm still not 100% confident that introducing this change will work for everyone.

I'm not even sure what exactly to test though because those reporting the issues aren't providing me with much detail about their setups. It could be that this latest patch is ok, I personally didn't have a problem with the previous patch , this latest patch should be safer.

Oh. This has been live for a week or two without any issues. If people experience issues they should give details indeed. Moving to "Needs work"

performance stats would be appreciated too, even ballpark +-500 ms

@angel.h moved from Needs review to RTBC in #40, but then when he knew some people had problems with previous patch he changed state to Needs work. I'm changing back the state to Needs Review.

Current status is: first patch worked until some people reported problems; a second patch was contributed with some fixes over first patch, but after testing both patches by @joseph.olstad he found that both worked. So it's not sure second patch addresses issues from first patch.

We need more testing on second patch, specially from those who reported first patch was failing (@afsolano and @ABaier), if possible.

IMHO it may be possible that the first patch failed because some interaction with other modules/core issues at that time.

We tested the #34 patch with xdebug profiler, and the calls to the token_generate function decrease substantially.
I think we need to accept this patch to improve the module performance.

This image shows the number of callings to token_generate function, 207 calls.

This image shows the number of callings to token_generate function with the patch applied, 15 calls.

yes that's a lot of function calls to token, DamienMcKenna wants to see a simpletest for this, it would be nice to have.

Meanwhile, I could try testing this patch out again and see if it works with multilingual for alt and title attributes, this seemed to be an issue previously but is probably fixed with the latest patch.

If this passes my tests, I was thinking of pushing this fix to the 7.x-3.x branch which has far fewer installs, lower risk change.

Ideally someone would write the simpletest for this and we could push it to the 7.x-2.x branch with higher confidence. The simpletest would have to check the alt and title attributes in english and another language making sure that both are as expected.

ok patch #34 appears to work for my use case with image file entity that has alt and title fieldable translation enabled.

To repeat use case, follow these steps
#2838315-10: Support for translating file entities

Translate one file entity alt and title fields

and then create a view of unformated fields with the article type
add the image field
save the view

hover your mouse over the image in the default language , the default language hover should show the image title in that language
switch language, hover the mouse over the image and this language (in my case French) the hover should display the image title in french, which it does both before and after patching.

I'm going to push this to the 7.x-3.x branch.

Giving credit to FabianX for the author as his feedback was critical to the successful patch. Everyone else gets credit as well thanks!

(Note: catch is the one that first wrote the patch so he would also deserve credit equally but I cannot put two so...)

Thanks everyone!

Now setting back to 7.x-2.x , do we want to hold off commit and wait for someone to write the simple test for this before bringing this change into 7.x-2.x? keeping in mind, 7.x-2.x has a huge number of installs.

Ah good enough, pushing this to 7.x-2.x
let all this simmer in the dev branches for a bit.

for followup see
#3006276: add test coverage for alt and title fields in more than one language

ok, this patch needs work, already someone reported an issue with it after tagging a release.

I will revert the patch again and push a new release.

tagged 7.x-2.24 without this

Dangit :-\

So what was the reported problem?

wondering if this was just isolated incident or a wider affected radius.

see details:

#3012152-2: Image Alt and Title fields not saving on node edit page (again)

Just a quick response:

Our problem was, that the title was not loaded in node view and node edit. Saving the title seemed to work (but afterwards not visibly again).

Bumping. What's the status with this? I see that it was fully reverted in 7.x-2.25.

If I'm reading the comments correctly, it was reverted due to the issue mentioned in #62. But then it looks like a commit was made in #64 and #65 in November 2018 (10 months ago). Does that mean it's ready to be added to the next release?

If you look closely at the commit message it says "Fully revert Speed up via shortcutting token file_entity_file_load"

so it has been reverted.

It's also in the release notes for 7.x-2.25
https://www.drupal.org/project/file_entity/releases/7.x-2.25

Ohh, my bad... I obviously wasn't looking closely :)

If your site is only using one language you can go ahead and use the patch, it is probably going to work for you however we won't be committing something that breaks a site that displays in more than one language.

having said that, try the patch yourself in a test environment first, please report your findings.
Thanks

You may need to reroll the patch as there's been changes in the dev branch since the patch. Also I cannot say exactly which patch you should start with.

Great, thank you.

In my case, I was just performing updates on a few client sites, and wanted to make sure I wasn't running into any regression issues. I don't believe I have a specific case to test this issue on, so I can't move the issue forward at this time. Good luck.

I've just encountered this performance issue and added the language arguments to the attached patch.

That said, I had not really done extensive testing on this and am only sure this works for our implementation.

Can anyone confirm that if using this latest performance patch with a two or more language site that the other language alt / title loads with the correct language values?

here is how to test, follow the README.txt instructions included with this module

Ok I just tested this, it seems to work fine when translation is enabled.
I followed the README.txt instructions
https://git.drupalcode.org/project/file_entity/blob/HEAD/README.txt
enabled translation on file_entity , enabled translation on alt , title and description fields for image type

and then created a view showing file alt and file title

translated values, they came out in each language as expected.

actually, still reviewing this.

hmm, yes using the latest patch above I reviewed this some more, everything works as expected according to my tests.
I cannot see a regression here, maybe I'll run some more tests.

I've tested this patch quite a bit today, I can't find a regression before/after, seems to be good.

I've put this in the dev branch, I don't have new tests for this but extensive manual tests seemed to pass.

let this simmer in the dev branch for a bit