Page MenuHomePhabricator

A cached server-side HTML template should update when you change a partial template which it includes
Closed, ResolvedPublic5 Estimated Story Points

Description

https://gerrit.wikimedia.org/r/#/c/223165/ has a top-level template Skin.mustache that includes the partial template sidebar.mustache using {{>sidebar}}.

I notice on my local wiki if I edit Skin.mustache and view a page with the skin, I see the change to it (good!), but if I edit the partial sidebar.mustache, I don't see the change to it, even if I use ?action=purge (bug!).

Looking at includes/TemplateParser.php in core, it does a simple

// Read the template file
$fileContents = file_get_contents( $filename );

// Generate a quick hash for cache invalidation
$fastHash = md5( $fileContents );

this doesn't notice changes to partials included by $filename.

(This is probably hard to fix. A workaround is to make cosmetic changes to all parent templates that include the edited partial template. The bug and workaround is documented in https://www.mediawiki.org/wiki/Manual:HTML_templates#Caching.)

Developer notes

We'd like to restrict these changes to Vector for the time being to check the performance implications of such a change. We could use a custom template partial validator if necessary or allow the passing of an alternative cache key.

QA steps

Quick smoke test to check obvious UI elements are still there.
View https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page

  • Verify edit tabs are present
  • Verify side bar contains links to random, interaction, tools, in other projects, languages
  • With correct permissions you should be able to see the more menu with delete/move
  • personal tools (username in top right) is present
  • there is still a footer
  • an article is rendered correctly

QA Results

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@Krinkle that sounds sensible to me. Is there a way we can trigger a notice/log warning when partials are used?

Change 572933 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] WIP: Template should consider partials

https://gerrit.wikimedia.org/r/572933

This would mean that the place where the sub-template is rendered, an HTML (or text) parameter is used instead and the value set to to the expanded template. This could lead to better separation of concerns and better re-use

'Could' yes however my experience is that it doesn't, although I recognize it's hard to articulate that value. My experience with not using template partials is readability and maintainability. From experience template partials are much more pleasurable to work with then HTML strings when you are reusing template code. I've found less benefits where template partials exist purely to reduce large templates

For example using template partials here I consider bad:

<div>
{{>Header}}
</div>

whereas here it's useful:

<div>
{{#navigationHeader}}
{{>Header}}
{{/navigationHeader}}
</div>
<div>
{{#articleHeader}}
{{>Header}}
{{/articleHeader}}
</div>

While I cannot provide a full essay right now on why template partials are useful, I can provide some recent experiences in Vector.

Right now in Vector the template partial VectorMenu is used in two places - renderVariantsComponent and renderActionsComponent. Both of these menus show conditionally. Now if you were changing renderVariantsComponent you have to remember to check the renderActionsComponent. If you however reference VectorMenu inside the master template it becomes obvious right away that these can be used.

When template partials are used e.g. it's much clearer to see the 2 usages and how they might impact each other and there is less risk of regressions IMO.

Also when using template partials, you completely separate data from rendering inside your functions (you use a single entry point for rendering) and it becomes much clearer where further refactoring can occur e.g. renderVariantsComponent and renderNamespacesComponent in Vector the template data despite being identical uses different keys.

ovasileva changed the point value for this task from 8 to 5.Feb 24 2020, 7:12 PM

Change 574739 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/core@master] [WIP] TemplateParser: Make caching consider partials

https://gerrit.wikimedia.org/r/574739

@Jdlrobson Following on from our post-standup chat about this task and your patch, the above patch makes the TemplateParser keep track of the files being accessed during compilation and uses them to produce a hash which can then be checked when fetching a cached compilation result at a later date. This is very much the approach that @Krinkle suggested in T113095#2675078.

Even in the best case, the change increases the number of roundtrips to the FS and increases the amount of data read from the server-local object cache (APC or APCu) (and then deserialized). I suggest benchmarking the performance of the modified TemplateParser to better inform the discussion of the approach's merits.

Change 572933 abandoned by Jdlrobson:
Template should consider partials

Reason:
After talking to Roan and looking at Sam's alternative patch - https://gerrit.wikimedia.org/r/c/574739 seems to be preferable since it copies what we do for LESS in ResourceLoader.

https://gerrit.wikimedia.org/r/572933

Change 575228 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/core@master] TemplateParser: Document/normalize exception messages

https://gerrit.wikimedia.org/r/575228

Change 575229 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/core@master] TemplateParser: Make cache value include metadata

https://gerrit.wikimedia.org/r/575229

Change 575230 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/core@master] TemplateParser: Invalidate cache if partial changes

https://gerrit.wikimedia.org/r/575230

Change 574739 abandoned by Phuedx:
[WIP] TemplateParser: Make caching consider partials

Reason:
Abandoned in favour of I948fdaec and the two preceding commits.

https://gerrit.wikimedia.org/r/574739

Change 575252 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/core@master] Do Not Merge: Add TemplateParser benchmark

https://gerrit.wikimedia.org/r/575252

Here are some results from the benchmark above:

$ git rev-parse --abbrev-ref HEAD
master
$ mwscript maintenance/benchmarks/benchmarkTemplateParser.php --wiki wiki --count 50000
Running PHP version 7.2.26-1+0~20191218.33+debian9~1.gbpb5a340+wmf1 (x86_64) on Linux 4.9.0-9-amd64 #1 SMP Debian 4.9.168-1+deb9u2 (2019-05-13)

withCaching
   count: 50000
    rate:    754.9/s
   total: 66232.28ms
    mean:     1.32ms
     max:    45.04ms
  stddev:     0.54ms
Current memory usage: 48 MB
   Peak memory usage: 48 MB

withoutCaching
   count: 50000
    rate:    237.9/s
   total: 210202.07ms
    mean:     4.20ms
     max:    40.82ms
  stddev:     0.82ms
Current memory usage: 76 MB
   Peak memory usage: 76 MB

$ git checkout review/phuedx/T113095
$ mwscript maintenance/benchmarks/benchmarkTemplateParser.php --wiki wiki --count 50000
Running PHP version 7.2.26-1+0~20191218.33+debian9~1.gbpb5a340+wmf1 (x86_64) on Linux 4.9.0-9-amd64 #1 SMP Debian 4.9.168-1+deb9u2 (2019-05-13)

withCaching
   count: 50000
    rate:    668.8/s
   total: 74761.68ms
    mean:     1.50ms
     max:    45.25ms
  stddev:     0.66ms
Current memory usage: 48 MB
   Peak memory usage: 48 MB

withoutCaching
   count: 50000
    rate:    231.9/s
   total: 215611.64ms
    mean:     4.31ms
     max:    61.39ms
  stddev:     0.98ms
Current memory usage: 76 MB
   Peak memory usage: 76 MB

Change 575252 abandoned by Phuedx:
Do Not Merge: Add TemplateParser benchmark

https://gerrit.wikimedia.org/r/575252

Change 575228 merged by jenkins-bot:
[mediawiki/core@master] TemplateParser: Document and normalize exceptions

https://gerrit.wikimedia.org/r/575228

Change 575229 merged by jenkins-bot:
[mediawiki/core@master] TemplateParser: Make cache value include metadata

https://gerrit.wikimedia.org/r/575229

Expanding on T113095#5923507 somewhat: there's a 13.6% decrease in the performance of TemplateParser::processTemplate with caching enabled.

IMO this should be acceptable as a cached compiled template is now correctly invalidated when a partial changes!

Change 575230 merged by jenkins-bot:
[mediawiki/core@master] TemplateParser: Invalidate cache if partial changes

https://gerrit.wikimedia.org/r/575230

I'm assuming some follow ups are needed following our conversation yesterday. I'd advise capturing what that follow up work is in a new task or this task's acceptance criteria so the scope is as clear as possible.

Change 576828 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/core@master] TemplateParser: Test server-local cache interaction

https://gerrit.wikimedia.org/r/576828

Thanks for the reminder, @Jdlrobson.

I've tagged https://gerrit.wikimedia.org/r/576828 here as getting TemplateParser's cache invalidation logic under test is well within scope (since I've just changed it…).

Change 576828 merged by jenkins-bot:
[mediawiki/core@master] TemplateParser: Test server-local cache interaction

https://gerrit.wikimedia.org/r/576828

Change 576828 merged by jenkins-bot:
[mediawiki/core@master] TemplateParser: Test server-local cache interaction

https://gerrit.wikimedia.org/r/576828

This is causing CI breakages on https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/578071/ when trying to update lightncandy

18:20:09 FILE: /workspace/src/tests/phpunit/includes/TemplateParserTest.php
18:20:09 ----------------------------------------------------------------------
18:20:09 FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
18:20:09 ----------------------------------------------------------------------
18:20:09  9 | WARNING | Duplicate class name "TemplateParserTest" found; first
18:20:09    |         | defined in
18:20:09    |         | /workspace/src/tests/phpunit/integration/includes/TemplateParserTest.php
18:20:09    |         | on line 10
18:20:09    |         | (Generic.Classes.DuplicateClassName.Found)
18:20:09 ----------------------------------------------------------------------

tests/phpunit/integration/includes/TemplateParserTest.php and tests/phpunit/includes/TemplateParserTest.php both have the same class name, both not namespaced

Change 578084 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] Rename TemplateParserTest.php -> TemplateParserIntegrationTest.php

https://gerrit.wikimedia.org/r/578084

Change 578084 merged by jenkins-bot:
[mediawiki/core@master] Rename TemplateParserTest.php -> TemplateParserIntegrationTest.php

https://gerrit.wikimedia.org/r/578084

Coulda sworn that I'd namespaced the integration test. Thanks for the follow up @Reedy and for the merge @Krinkle.

Coulda sworn that I'd namespaced the integration test. Thanks for the follow up @Reedy and for the merge @Krinkle.

Heh. It happens. Feel free to revert the rename and set a namespace if that's preferred (I didn't look for too long for how things were done, beyond seeing another test using "IntegrationTest" in their name).

I did file T247209 as a followup to work out why the tests didn't complain until I seemingly touched the vendor dependancy :)

Edtadros subscribed.

Test Result

Status: ✅ PASS
OS: macOS Catalina
Browser: Chrome
Device: MBP

Test Artifact(s):

QA steps

Quick smoke test to check obvious UI elements are still there.
View https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page

❓AC1: Verify edit tabs are present
The Edit tabs are not present on the main page, but they are on article pages.

✅ AC2: Verify sidebar contains links to random, interaction, tools, in other projects, languages

❓ AC3: With correct permissions, you should be able to see the more menu with delete/move
I may not have the correct permissions, but I cannot see the delete option.

✅ AC4: personal tools (username in top right) is present

✅ AC5: there is still a footer
Not shown in the images, but the footer was present on both Main_Page and a random article.

✅ AC6: an article is rendered correctly
I checked a number of articles. They looked like they rendered correctly.

Main_PageRandom Article
en.wikipedia.beta.wmflabs.org_wiki_Main_Page(iPad).png (2×1 px, 752 KB)
en.wikipedia.beta.wmflabs.org_wiki_Dog(iPad).png (2×1 px, 1 MB)

Looks good, thanks @Edtadros. Could see the more dropdown & you are correct, edit does not appear on the main page

Looks like there was an issue with the implementation causing T248010.

From IRC:

$tp->processTemplate( 'index', $params );
2:46 PM The parameter is literally just the string "index"
2:46 PM that's not globally unique
2:46 PM the cache key appears not to consider the full file path?
2:46 PM <phuedx> Sam Smith I see it
2:47 PM <Krinkle> Timo Tijhof It should call getTemplateFilename() earlier and use it in the cache key
2:47 PM <phuedx> Sam Smith As you wrote it. I saw it
2:47 PM Yup. That
2:47 PM <Krinkle> Timo Tijhof Not sure if that explains it but certainly a bug in its own right

Change 581107 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/core@master] TemplateParser: Include template dir in cache key

https://gerrit.wikimedia.org/r/581107

Change 581114 had a related patch set uploaded (by Krinkle; owner: Phuedx):
[mediawiki/core@wmf/1.35.0-wmf.23] TemplateParser: Include template dir in cache key

https://gerrit.wikimedia.org/r/581114

Change 581115 had a related patch set uploaded (by Krinkle; owner: Phuedx):
[mediawiki/core@wmf/1.35.0-wmf.24] TemplateParser: Include template dir in cache key

https://gerrit.wikimedia.org/r/581115

Change 581107 merged by jenkins-bot:
[mediawiki/core@master] TemplateParser: Include template dir in cache key

https://gerrit.wikimedia.org/r/581107

Change 581114 merged by jenkins-bot:
[mediawiki/core@wmf/1.35.0-wmf.23] TemplateParser: Include template dir in cache key

https://gerrit.wikimedia.org/r/581114

Change 581115 merged by jenkins-bot:
[mediawiki/core@wmf/1.35.0-wmf.24] TemplateParser: Include template dir in cache key

https://gerrit.wikimedia.org/r/581115

phuedx claimed this task.

The two patches above didn't change the behaviour of the TemplateParser class, only formalised its dependency on some cache, which in turn makes it easier to test.

Per T113095#5963251.