Problem/Motivation
Follow-up to #2291449: Add Twig template inheritance based on the theme registry, enable adding Twig loaders.
@Fabianx spotted the file_exists() when we are registering namespaces in \Drupal\Core\Template\Loader\FilesystemLoader.
Proposed resolution
The file_exists() is not necessary here, just remove it. This would make all modules and themes available as Twig namespaces regardless of whether a 'templates' directory exists in the module/theme.
Remaining tasks
Patch
Beta phase evaluation
Issue category | Task because it's a performance enhancement. |
---|---|
Issue priority | Major because we should not have any file_exists() calls during runtime. |
Unfrozen changes | Unfrozen because it is a performance improvement in the theme system. |
Disruption | No disruption. |
User interface changes
n/a
API changes
n/a
Comment | File | Size | Author |
---|---|---|---|
#28 | interdiff.txt | 612 bytes | star-szr |
#28 | 2383413-28.patch | 1.04 KB | star-szr |
Comments
Comment #1
star-szrPostponed until #2291449: Add Twig template inheritance based on the theme registry, enable adding Twig loaders is committed.
Comment #2
star-szrAlthough this might cause some issues (in \Twig_Loader_Filesystem) :/
Comment #3
star-szrI suppose we could add our own version of addPath() that removes that :)
Comment #4
star-szrComment #5
alvar0hurtad0I suppose it is.
Comment #7
joelpittet#2 seems to have been spot on;) Thanks for testing that theory out @alvar0hurtad0
Comment #8
alvar0hurtad0Ummmmm
what should I do?
submit the patch again or let it be?
Comment #9
joelpittet@Cottser I'm assigning to you, but maybe this is something for @Fabianx to have a look at and provide suggestions.
It looks like either an upstream change or maybe a try/catch maybe? All guesses.
@alvar0hurtad0 we'll see what they have to say before going unless you have some ideas you want to try, be my guest.
Comment #10
star-szrI suggest #3, we are already subclassing the Twig Filesystem class, we can override addPath to remove the
is_dir
check. As long as that has no nasty side effects it seems like the best way forward to get the performance we want.Make sense?
Comment #11
joelpittetWould there be any side effects to loading paths into the loader that don't exist?
Comment #12
larsmw CreditAttribution: larsmw commentedI have made a patch for overriding addPath in FilesystemLoader.
Comment #13
tadityar CreditAttribution: tadityar commentedSetting to needs review for patch to be tested.
Comment #14
joelpittetThis seems like a good solution I think... just a bit of cleanup needed here.
This should use drupal coding standards for indents. IE 2 spaces.
I'd Leave this above to still invalidate the cache even if it's not a dir, just like upstream does.
Comment #15
larsmw CreditAttribution: larsmw commentedIsn't this supposed to throw an exception instead of return FALSE? I just dont know which exception!?
Comment #16
joelpittetNot from the issue summary, but maybe it should? @Cottser thoughts?
Comment #17
star-szrI think the idea would be to just add all directories regardless of whether they exist or not, we can double check to see how Twig handles it when trying to load namespaced templates to see if it causes any issues. But right now I don't see any other way forward if we want to remove the file_exists().
Hopefully that makes sense, it's early :)
Comment #18
larsmw CreditAttribution: larsmw commentedCottser, you mean like this?
Comment #19
joelpittetNeeds review status will trigger testbot to check the patch.
The commenting of the * lines need to be indented one space.
The end of the lines should end in a Period and Start with a capitol.
This line break is not need.
Comment #20
larsmw CreditAttribution: larsmw commentedI have cleaned it up now.
Comment #21
star-szr@larsmw, yes to #18 - code looks good now, thanks! Please post interdiffs when you update the patch, great habit to get into early and makes it easier to review changes. Getting patches reviewed is most often the biggest bottleneck in the issue queues so anything you can to do help make that easier is a huge win in my opinion :)
These should be updated to match our documentation standards on params, whitespace and punctuation changes would be needed: https://www.drupal.org/node/1354#param
Minor: Missing blank line between the last method and the end of the class per https://www.drupal.org/node/608152#indenting.
Comment #22
joelpittetWhat is the reason behind adding directories to the registry that may not exist? AKA can we leave the is_dir check in for parity upstream?
Comment #23
star-szrIt's kind of a cascade, the
is_dir()
will fail without thefile_exists()
filtering out nonexistent directories. The nonexistent directories are just being added as possible namespaces, Twig should still complain on its own when you try to extend/include/whatever a template that doesn't exist, that could use a bit of manual (or automated?) testing.Comment #24
joelpittetAh, I'm thinking from Twig's perceptive having to look in folders that don't exist when searching for templates, though I guess once it's found the template it doesn't have to search through these folders again. As long as storing these potentially empty templates doesn't have an negative effect on performance as the IS is intending to solve, it's cool with me;)
Sensitive to file searching due to module_load_include issues last week in D7. #1443308: Add static cache to module_load_include()
Comment #25
star-szrBecause we have the string loader, calling a nonexistent template just prints the string, with or without the patch. So no concerns there. This also means that I'm pretty sure we don't have a good way of testing this currently (like by catching an exception) until #2369981: Not found templates are displayed literally instead of throwing an Exception due to string loader is resolved, which I just bumped to major.
Comment #26
larsmw CreditAttribution: larsmw commentedImplemented comments from #21
Comment #27
larsmw CreditAttribution: larsmw commentedComment #28
star-szrAttaching one minor change to remove a now unnecessary variable. This looks good to me now as far as documentation and coding style, and solves the stated task, RTBC. I don't think this needs additional test coverage.
--
For this one profiling on an SSD is probably not the best way to test, but I was able to confirm that we are cutting down on calls to
file_exists()
(andis_dir()
).Scenario: Standard profile, Bartik homepage, added 17 search blocks to bring the total of search blocks up to 18. Gave anonymous users the 'search content' permission.
Reduced function calls:
file_exists()
: -45is_dir()
: -25There are 21 more calls to
rtrim()
with this approach, but those are extremely cheap.http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=54e0a6d45fe2a&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=54e0a6d45fe2a&...
Comment #29
webchickCan't think of a reason we would not want to do this, but booting over to catch since it touches performance stuff.
Comment #30
catchLooks great. Committed/pushed to 8.0.x, thanks!