Hello all,
After the new version of LabeledSectionTransclusion (LST) was deployed on itwikisource, performance issues popped up. itwikisource's main page makes heavy use of LST, and the new version is clearly heavier than the old one.
In this mail, I'll try to describe the aims of the new version, how the old version worked and how the new version works.
Aims ------- In the old situation, it was possible to transclude sections of pages by marking them with <section> tags. However, it was impossible to include those tags from within a template. I.e. given
page P: something before <section start='a'>something with a</section end='a'> something after page Q: {{#lst:P|a}}
then Q was rendered as
something with a
However, it was not possible to do something like:
page O: ===<section start='header'>{{{1}}}</section end='header'>=== page P: {{O|Some header text}} page Q: {{#lst:P|header}}
Changes in the #lst parser -------------------------------------- This was because in the old situation, the #lst mechanism did something along these lines: 1) get DOM using $parser->getTemplateDom( $title ); - note that this is a non-expanded DOM, as in templates are not expanded 2) traverse this DOM, find section tags, and call $parser->replaceVariables(....) on the relevant sections
In the new situation, the #lst mechanism does something like: 1) get expanded wikitext using $parser->preprocess("{{:page_to_be_transcluded}}") 2) get the DOM by calling $parser->preprocessToDom() on the expanded wikitext 3) traverse this DOM, find section tags, and call $parser->replaceVariables(....) on the relevant sections (unchanged)
One obvious performance issue is that (1) and (2) are not cached - not within one response (so if a page {{#lst}}'s the same page twice, that page is processed twice), and not between responses (no caching). In general, I think it would be preferrable not to do a full parse, but just to expand the DOM of the templates. Unfortunately, I have not been able to find a simple way to do this: PPFrame::Expand expands the templates to their final form, not to an 'expanded DOM'.
I don't know MediaWiki caching well enough to say something about which caches are used (or not), and what would be an effective caching strategy.
Any ideas on how to do LST without bluntly doing a full page parse for every transcluded page, or on caching strategies, would be very welcome.
Best, Merlijn
On Fri, Nov 30, 2012 at 10:07 AM, Merlijn van Deen valhallasw@arctus.nl wrote:
After the new version of LabeledSectionTransclusion (LST) was deployed on itwikisource, performance issues popped up. itwikisource's main page makes heavy use of LST, and the new version is clearly heavier than the old one.
As a sidenote: because of the performance issues, the most recent changes to the LST extension will probably be reverted today (Friday, November 30).
If you made changes to articles or templates to accommodate the new version or benefit from new features, you may want to revert those changes temporarily.
On Fri, Nov 30, 2012 at 4:09 AM, Guillaume Paumier gpaumier@wikimedia.org wrote:
On Fri, Nov 30, 2012 at 10:07 AM, Merlijn van Deen valhallasw@arctus.nl wrote:
After the new version of LabeledSectionTransclusion (LST) was deployed on itwikisource, performance issues popped up. itwikisource's main page makes heavy use of LST, and the new version is clearly heavier than the old one.
As a sidenote: because of the performance issues, the most recent changes to the LST extension will probably be reverted today (Friday, November 30).
If you made changes to articles or templates to accommodate the new version or benefit from new features, you may want to revert those changes temporarily.
Aaron just reverted this a little earlier: https://gerrit.wikimedia.org/r/#/c/36316/
I think the way that he did this, though, means that if we do nothing, then we'll be redeploying this starting December 10th with the start of the 1.21wmf6 cycle. There's a reasonable amount of time between now and then, so we can leave it like this until next week, depending on the result of this conversation.
It would be nice to get some feedback from Merlijn about how to solve the problems he's trying to solve in a more efficient way. For those wanting to see the code in question, Merlijn's main commit is here: https://gerrit.wikimedia.org/r/#/c/31330/
For my part, it seems that even just making sure that there is only one extra parse per referenced page might be enough to make this perform acceptably, even if it means keeping all of the parsed transcluded pages around in memory. I'm not sure the preferred method these days (e.g. global, singleton, context object, or attaching to some other existing object), but that may be worth exploring. This is just a wild guess, though; I have no idea if that would be too heavy on our memory usage, and I'm only suggesting it because it seems relatively easy compared to the alternatives.
Rob
Looks like I misspoke here. I said "It would be nice to get some feedback from Merlijn about how to solve the problems he's trying to solve in a more efficient way." I meant to say "It would be nice to get some feedback FOR Merlijn about how to solve the problems he's trying to solve in a more efficient way."
So, I'm bumping this thread for two reasons: 1. Still would like to see some feedback for Merlijn 2. Reminder that we need to either fix or revert Merlijn's changes on master prior to deploying 1.21wmf6, lest we bring down itwikisource again. I'm going to guess that without more feedback, Merlijn won't be able to fix this, so we should plan on reverting on Thursday or so if this thread goes stale.
Rob ---------- Forwarded message ---------- From: Rob Lanphier robla@wikimedia.org Date: Fri, Nov 30, 2012 at 5:03 PM Subject: Re: [Wikitech-l] LabeledSectionTransclusion performance problems To: Wikimedia developers wikitech-l@lists.wikimedia.org
On Fri, Nov 30, 2012 at 4:09 AM, Guillaume Paumier gpaumier@wikimedia.org wrote:
On Fri, Nov 30, 2012 at 10:07 AM, Merlijn van Deen valhallasw@arctus.nl wrote:
After the new version of LabeledSectionTransclusion (LST) was deployed on itwikisource, performance issues popped up. itwikisource's main page makes heavy use of LST, and the new version is clearly heavier than the old one.
As a sidenote: because of the performance issues, the most recent changes to the LST extension will probably be reverted today (Friday, November 30).
If you made changes to articles or templates to accommodate the new version or benefit from new features, you may want to revert those changes temporarily.
Aaron just reverted this a little earlier: https://gerrit.wikimedia.org/r/#/c/36316/
I think the way that he did this, though, means that if we do nothing, then we'll be redeploying this starting December 10th with the start of the 1.21wmf6 cycle. There's a reasonable amount of time between now and then, so we can leave it like this until next week, depending on the result of this conversation.
It would be nice to get some feedback from Merlijn about how to solve the problems he's trying to solve in a more efficient way. For those wanting to see the code in question, Merlijn's main commit is here: https://gerrit.wikimedia.org/r/#/c/31330/
For my part, it seems that even just making sure that there is only one extra parse per referenced page might be enough to make this perform acceptably, even if it means keeping all of the parsed transcluded pages around in memory. I'm not sure the preferred method these days (e.g. global, singleton, context object, or attaching to some other existing object), but that may be worth exploring. This is just a wild guess, though; I have no idea if that would be too heavy on our memory usage, and I'm only suggesting it because it seems relatively easy compared to the alternatives.
Rob
On 3 December 2012 21:16, Rob Lanphier robla@wikimedia.org wrote:
- Reminder that we need to either fix or revert Merlijn's changes on
master prior to deploying 1.21wmf6, lest we bring down itwikisource again. I'm going to guess that without more feedback, Merlijn won't be able to fix this, so we should plan on reverting on Thursday or so if this thread goes stale.
I'll give some more background on my analysis of why this happened. In any case, I'll create a revert patch before thursday, so it shouldn't be a problem to deploy it.
The reason itwikisource died is a combination of how LST is used on itwikisource and how the new LST implementation worked. I'll try to give a short description here.
On most wiki's, the Main Page consists of several transcluded pages, i.e. 'Main Page/header', 'Main Page/Section Something', 'Main Page/Section Something Else'. This is not the case on itwikisource: instead of using seperate pages, it uses a single page, and LST's the relevant sections - so Pagina Principale transcludes *sections *from "Pagina principale/Sezioni" - eight of them.
In the original LST implementation, this was not a problem: just the transcluded sections were rendered, so overall, 'Pagina principale/Sezioni' was only parsed once - but in a eight seperate pieces. However, in the * current* situation, the whole of 'Pagina principale/Sezioni' is rendered for *every* transclusion, which means that the parsing times goes up by a factor of 8. To make matters worse, this structure is also used further down the template tree, which means the overall parsing time goes up to ~ 30s, the parser's limit. (the current parsing time is ~2s).
As such, I think Rob's suggestion of caching the parsing step might be enough to solve the issues. The parsing will still be more resource intensive, but at least it shouldn't reach bring itwikisource to it's knees. However, I don't have the setup to test this currently, nor do I really have the time to set this up this week.
So -- I'll first make sure there is a working revert patch submitted. After that, I'll try to work on creating a more resource-friendly implementation. Hopefully there will be some comments on how to do that in an even better way by then.
Best, Merlijn
On 04/12/12 09:01, Merlijn van Deen wrote:
In the original LST implementation, this was not a problem: just the transcluded sections were rendered, so overall, 'Pagina principale/Sezioni' was only parsed once - but in a eight seperate pieces. However, in the * current* situation, the whole of 'Pagina principale/Sezioni' is rendered for *every* transclusion, which means that the parsing times goes up by a factor of 8. To make matters worse, this structure is also used further down the template tree, which means the overall parsing time goes up to ~ 30s, the parser's limit. (the current parsing time is ~2s).
Judging by the itwikisource jobs I just killed on most of the job runners, the parse time went up by a factor of infinity, not a factor of 8. They had been running for 3.5 days with --maxtime=300.
However, it was not possible to do something like:
page O: ===<section start='header'>{{{1}}}</section end='header'>=== page P: {{O|Some header text}} page Q: {{#lst:P|header}}
So don't do that. I find it hard to believe that allowing such a feature will be an amazing win for the wiki's usability.
In the new situation, the #lst mechanism does something like:
- get expanded wikitext using
$parser->preprocess("{{:page_to_be_transcluded}}") 2) get the DOM by calling $parser->preprocessToDom() on the expanded wikitext 3) traverse this DOM, find section tags, and call $parser->replaceVariables(....) on the relevant sections (unchanged)
Double-parsing will break the syntax, {{!}} etc. will not work.
-- Tim Starling
Judging by the itwikisource jobs I just killed on most of the job runners, the parse time went up by a factor of infinity, not a factor of 8. They had been running for 3.5 days with --maxtime=300.
This surprises me. First of all, because a copy of Pagina Principale (with all it's transclusions) took ~30s to render on test2 (compared to 0.2s now). Secondly, because I would think that 3.5 days ago, itwikisource was already back on wmf4.
So don't do that. I find it hard to believe that allowing such a
feature will be an amazing win for the wiki's usability.
I constructed the most basic example I could think of. An example would be a template containing something like this:
==={{{1}}}=== {{#tag:section|begin={{{1}}}}}
In addition, see the relevant bugzilla thread: https://bugzilla.wikimedia.org/show_bug.cgi?id=21339
Double-parsing will break the syntax, {{!}} etc. will not work.
I'm not quite sure why this would be the case, but I'll take a look.
Merlijn
On 04/12/12 19:08, Merlijn van Deen wrote:
Judging by the itwikisource jobs I just killed on most of the job runners, the parse time went up by a factor of infinity, not a factor of 8. They had been running for 3.5 days with --maxtime=300.
This surprises me. First of all, because a copy of Pagina Principale (with all it's transclusions) took ~30s to render on test2 (compared to 0.2s now).
The jobs were probably not parsing Pagina Principale, they were probably parsing some page with an infinite loop of #lst invocations. Sorry, I didn't get a backtrace.
Secondly, because I would think that 3.5 days ago, itwikisource was already back on wmf4.
Aaron switched itwikisource back to wmf4 at 00:03 on December 1. I killed the jobs at 00:36 on December 4. So 3 days before I killed the jobs, itwikisource was on wmf5.
-- Tim Starling
This is obvious, but if the "new lst" allows infinite loops, it MUST NOT be deployed. Not even if it rendered the Pagina Principale acceptably.
On 5 December 2012 05:19, Tim Starling tstarling@wikimedia.org wrote:
The jobs were probably not parsing Pagina Principale, they were probably parsing some page with an infinite loop of #lst invocations. Sorry, I didn't get a backtrace.
On 6 December 2012 00:51, Platonides Platonides@gmail.com wrote:
This is obvious, but if the "new lst" allows infinite loops, it MUST NOT be deployed. Not even if it rendered the Pagina Principale acceptably.
I actually interpreted Tim's mail as a metaphorical 'infinite loop' - something nested very deeply which takes a long time - not as a real infinite loop (especially as that will just exhaust the stack and . Just to be clear about this: there is a mechanism that prevents recursive transclusions, and the page parses on test2wiki, so I'm guessing the origin was not a real infinite loop - just too much computational effort.
On the code deployment side of things - Aaron had already reverted all changes since the last known working revision. I have cherry-picked all non-parser changes that were committed after that point in time, and most of these have now been merged again (thanks for those improvements, Brad, Reedy & tpt!).
I also just realized there is no link to the actual commit in this thread - please see https://gerrit.wikimedia.org/r/gitweb?p=mediawiki/extensions/LabeledSectionT... that.
Best, Merlijn
wikitech-l@lists.wikimedia.org