Interesting. Probably not actually related to PHP 7 but to Preprocessor_DOM -- I believe HHVM still runs Preprocessor_Hash. I've got an outstanding request to reduce the code duplication: T204945: Deprecate one of the Preprocessor implementations for 1.33.

Yep, most likely related. @Arlolra will probably write you a patch once he wakes up/gets online this am, but if you're impatient just removing the newlines from the indicated places in parserTests.txt will get you going again.

It would be a good idea to add some parser tests which exercise both modes of StringUtils::explode. If this hadn't thrown an exception for being not Countable, we probably wouldn't have noticed that the array keys shift on large articles until it made production.

Because the type hint just below it will say ?string not string|null? We should be using the one markup language which is used by PHP, not making up our own?

ExplodeIterator::key() returns $this->curPos, *not* the line number. So with the 497863 patch it fixes the exceptions (and so should be fine to backport to wmf.22) but for wikitexts over a thousand lines it will effectively "never be on the last line" and so you'll get an extra trailing newline.

See also T218816: MediaWiki.Commenting.FunctionComment.DefaultNullTypeParam wants null even if type is nullable.

Seems to be fixed. Watching builds of https://gerrit.wikimedia.org/r/464096 https://gerrit.wikimedia.org/r/497471 and https://gerrit.wikimedia.org/r/497320 to confirm.

Yes, the intention is certainly that title (in normalized form, ie spaces converted) can always be derived by removing the relative path. Furthermore, in modern Parsoid (not historically, but we can ignore that) that relative path is *always* ./. So just strip the first two characters and you've got your title.

See https://gerrit.wikimedia.org/r/#/q/topic:trail+(status:open+OR+status:merged) for the set of patches merged; scribunto probably needs a patch somewhat like https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/ParserFunctions/+/494939/

Yeah, we should just fix the scributo tests, not revert patches. This was a set of 9 or so dependent patches to merge, unrolling would be quite a chore, and the scribunto fix should just be to remove some trailing newlines from the parser tests....

There are a few patches which have actually (apparently) passes this test and gotten merged, eg https://gerrit.wikimedia.org/r/496080

Yeah, there's a more-or-less standard-but-ugly workaround that involves using mb_encode to replace everything above U+007F with an HTML entity: https://github.com/wikimedia/html-formatter/blob/5e33e3bbb327b3e0d685cc595837ccb024b72f57/src/HtmlFormatter.php#L71

I'd say there's one other use case, and it's what tidy does (AIUI): mutate a string representation of a HTML document in a "safe" way, without every building the complete DOM tree in memory. That is, "safe" string-to-string transformations. There are probably lots of weird things you could do here, but I would love to see a basic "insert X into Y" (like innerHTML) or "append X to Y" utility, done in a safe way that respected tag boundaries etc. The API of https://github.com/wikimedia/html-formatter/blob/master/src/HtmlFormatter.php could be a guide, just imagine doing it string-to-string without creating an intermediate DOM.

We don't use this (I don't think) but be aware that if you create an attribute named 'xmlns' in PHP's DOM everything breaks: https://marc.info/?l=php-internals&m=155249142123136&w=2

A performance update is at T204595#5024206

In general I would be happy to see an interface that allowed components to pass around DOM subtrees instead of strings, stringifying the tree only where needed for a legacy API. Then "composition" is (eventually) just subtree assembly, and we don't have to worry about poorly-constructed components leaking open tags into the rest of the content...

Testing with the following script (from the zest library home dir) with xdebug off:

$ psysh 
Psy Shell v0.9.9 (PHP 7.3.2-3 — cli) by Justin Hileman
>>> require 'vendor/autoload.php';
=> Composer\Autoload\ClassLoader {#2}
>>> require('./tests/ZestTest.php');
=> 1
>>> $html100 = file_get_contents('./obama.html'); strlen($html100);                       => 2592386
>>> timeit -n10 \Wikimedia\Zest\Tests\ZestTest::parseHTML($html100, [ 'suppressHtmlNamespace' => true, 'ignoreErrors' => true ]) && true;
=> true
Command took 0.389295 seconds on average (0.376101 median; 3.892954 total) to complete.

In T204595#4615610, @cscott wrote:

Could you post your test scripts somewhere? To be fair we should probably factor out process startup and file read times out of the measurements (the 350ms overhead you're measuring). It seems like we should dig into the slow Remex performance on large documents more, though, to figure out if there are some O(N^2) tree-mutation algorithms we need to kill, and if so figure out how hard they will be to fix (ie, are the bugs in Remex, in the PHP DOM extension).

I like this section from Symfony's page, which describes why it's nicer for long-term maintenance to use CSS selectors in the code:

I'm finding performance issues with the PHP DOM implementation in T212543: RemexHtml DOM construction performance increases non-linearly wrt HTML size as well; we could watch for those in the audit as well. (In particular, setting the namespace on a node in the PHP DOM seems to trigger a nonlinear slowdown.)

Some notes copied from chat:

T217849: Remex needs documentation of how to use its API as well. The spec requires \r stripping but IIRC MW also does newline stripping so we're guaranteed that any article we fetch from the DB already has newlines stripped, which is why there's an optimization in remex to avoid unnecessary work. I wonder if the time savings is actually significant enough to merit the developer cost of maintaining a separate option. In any case, we need to document this stuff better.

The relative # of calls appear to scale linearly between the 10% and 100% benchmarks:

Found another slowdown:

<cscott> huh, we're also spending a lot of time in CachingStack->dump() -- that's not right, that's pure debug code, we should never be in there (!)
<cscott> oh no, $this->trace( "AFE\n" . $afe->dump() . "STACK\n" . $stack->dump() );
<cscott>         private function trace( $msg ) {
<cscott>                 // print "[AAA] $msg\n";
<cscott>         }
<cscott> sigh
<cscott> removing the debugging code from TreeBuilder::adoptionAgency brings us down to 6.16s.  getting there! one more second shaved, five more to go.

Part of the slowness appears to be the call to dom_reconcile_ns in node.c -- callgrind seems to confirm that the xmlSearchNsByHref call is slow. Hacking around things by changing createElementNS to createElement in DOMBuilder::createNode (which is, incidentally, something requested as a short-term workaround by T217708: Remex should offer an option to not set namespaceURI) makes DOMElement->insertBefore() disappear from the hot functions list, and seems to reduce time spent parsing [[en:Barack Obama]] from 6.8s to 4.8s on my machine.

Profiling [[en:Barack Obama]] with xdebug seems to confirm that DOMElement->insertBefore() is the slow call; I just need to figure out how to get a C-level profile to get a little more insight into which path through https://github.com/php/php-src/blob/master/ext/dom/node.c#L925 is the slow one.

DOMBuilder.php uses $parent->insertBefore( $node, $refNode ) which appears to call xmlAddPrevSibling in libxml which ought to be constant time (nodes are stored as a linked list). The quadratic performance makes me think that something in here is operating on the child node *array* instead of a linked list, but I can't find it (yet).

@Catrope: https://github.com/wikimedia/parsoid/blob/34964e38e431238ecada0f052b4b81a5a19db84d/src/Wt2Html/XMLSerializer.php but @ssastry says it's not terribly fast right now. We should look into that, maybe there are performance tweaks that are possible.

In T217867#5011836, @Tgr wrote:

An alternative is to port domino/etc to C directly and have it be usable as a PHP extension so we get good perf as well. If it used libxml's nodes underneath you could still do fast XPath queries, etc, using the existing DOMXPath package.

AIUI most of the bugs come from libxml, not PHP directly, so that wouldn't improve the situation much.

Another useful note, while I'm brain-dumping. Part of the task would be to define an appropriate PHP binding to WebIDL. There's a good start in packagist -- https://packagist.org/packages/esperecyan/webidl -- but it's implementation based. Someone should write a brief document describing how WebIDL maps to PHP. Unfortunately, the only non-JavaScript language that appears to have a format WebIDL binding description is Java, and they have "stopped work" on it and published it as a W3C note.

@Catrope: yes, what @Tgr said -- your test case should be "<pre>\n\nfoo\n\nbar\n\n</pre>". (The middle and trailing ones are just for completeness, it's really the leading newlines that would be mangled.)

One note about porting domino in particular -- it uses meta-programming to generate classes corresponding to all the different HTML element types (HTMLAnchorElement, etc) from a compact specification (in htmlelts.js). Since neither PHP nor C support that kind of metaprogramming (eval doesn't count), that part of domino would have to be rewritten as a code-generator which runs during the build-phase instead. Probably not a huge deal, just something to keep in mind.

In theory (with infinite resources, etc) the best of all possible worlds would be a pure PHP implementation coupled with a "native" extension with more speed. Since (again in theory) both are implementing the exact same DOM API anyway, this would allow us to avoid adding an extension to mediawiki's required dependencies.

We've got our own XMLSerializer for Parsoid, that is probably part of the long-term solution.

Weird. Luckily (?) for Parsoid, this seems to be a bug in saveHTML(), which I don't think we're actually using? setAttribute/getAttribute handle the "true" (unencoded) value of the attribute fine. PHP:

$ psysh
Psy Shell v0.9.9 (PHP 7.3.2-3 — cli) by Justin Hileman
>>> $doc = new DOMDocument();
=> DOMDocument {#2308}
>>> $doc->loadHTML('<p>Hello</p>');
=> true
>>> $el = $doc->getElementsByTagName('p')->item(0);
=> DOMElement {#2325}
>>> $el->setAttribute('data-foo', '"<!-- foo&bar -->');
=> DOMAttr {#2311}
>>> $el->getAttribute('data-foo');
=> ""<!-- foo&bar -->"
>>> $doc->saveHTML($el);
=> "<p data-foo='"<!-- foo&bar -->'>Hello</p>"

vs domino (which should match browsers and the spec):

$ node
> var domino = require('./');
undefined
> var doc = domino.createDocument('<p>Hello</p>');
undefined
> el = doc.querySelector('p');
HTMLParagraphElement {}
> el.setAttribute('data-foo', '"<!-- foo&bar -->');
undefined
> el.getAttribute('data-foo');
'"<!-- foo&bar -->'
> el.outerHTML;
'<p data-foo="&quot;<!-- foo&amp;bar -->">Hello</p>'

The HTMLFormatter library itself is pretty general purpose and might be the basis of a more general lib, especially if it were updated (eventually) to use Remex and Zest (T217360: Replace libxml/xpath in HtmlFormatter with Remex/zest). I think Remex still has a performance problem to fix (T212543: RemexHtml DOM construction performance increases non-linearly wrt HTML size) which is probably going to prevent it from replacing DOMDocument::loadHTML in the immediate short term (sigh).

Seems to come from libxml: https://gitlab.gnome.org/GNOME/libxml2/blob/master/HTMLparser.c#L2323

Maybe relevant, maybe not:
@Tgr said "Turns out DOMNamedNodeMap did not implement the Countable interface before PHP 7.2; also pre 7.2 count() does not warn if you give it a non-countable value, it just shrugs and returns 1. That made the attribute walking logic throw up.)" on a different bug (https://gerrit.wikimedia.org/r/#/c/mediawiki/services/parsoid/+/486835/).
and @Anomie added on IRC, "I do see that https://secure.php.net/manual/en/class.domnodelist.php says Countable wasn't implemented until 7.2.0."

Yeah, that sounds right. Option to DOMBuilder.

Another fun bug -- if you use DOMDocument::loadHTML(), the top-level node (DOMDocument) has nodeType of 13 (which is not defined in any spec ever, not even DOM level 1) instead of 9 (which is what the spec says it should be).

Looks like the implementation is missing the "or the template is at the start of the output" clause from the spec.

@Catrope also look at the HTMLFormatter library; the mobile team already have figured out a bunch of weird workarounds for PHP's DOM bugs.

Sigh.

We should probably bump Parsoid to 7.2 since (a) the zest port is using 7.2 and (b) mediawiki is planning to skip 7.1 anyway.