General and performance tweaks #614

aidantwoods · 2018-04-12T20:00:19Z

This might be the first PR in a while that reduces the size of the codebase very slightly ;p

As measured by a few markbench runs, this PR makes Parsedown about 15-20% faster on the github sample profile, and about 10% faster when run without a profile specified. These being run on PHP 7.2.4, and performance diffs are in comparison to 1.8.0-beta-1.

Its probably worth cherry-picking some of these commits over some others. Adjustments to the inlineText function have major performance improvements, v.s. things like swapping regular expressions for loops – which doesn't appear to offer much of an observable benefit on my system (latest PHP version at present).

Adjustments towards regular expressions may be beneficial where Parsedown would previously encounter catastrophic backtracking with certain inputs.

Adjustments to string concatenation syntax appear to offer no observable difference in Parsedown in my testing (despite historic benchmarks from the PHP community apparently indicating a difference).

We gain quite a bit of a speed boost by working with text here since this is a very common function

… be quick

Also remove unused function

PhrozenByte · 2018-04-12T20:30:32Z

Parsedown.php

+                $beforeTab = strstr($line, "\t", true);
+                $beforeTab !== false;
+                $beforeTab = strstr($line, "\t", true)
+            ) {


while (($beforeTab = strstr($line, "\t", true)) !== false) { // ... }

PhrozenByte · 2018-04-12T20:31:45Z

Parsedown.php

@@ -216,7 +207,7 @@ protected function linesElements(array $lines)

            if (isset($CurrentBlock['continuable']))
            {
-                $Block = $this->{'block'.$CurrentBlock['type'].'Continue'}($Line, $CurrentBlock);
+                $Block = $this->{"block{$CurrentBlock['type']}Continue"}($Line, $CurrentBlock);


readability:

$methodName = 'block' . $CurrentBlock['type'] . 'Continue'; $Block = $this->$methodName($Line, $CurrentBlock);

+ all other occurences below

👍

As mentioned in:

Adjustments to string concatenation syntax appear to offer no observable difference in Parsedown in my testing (despite historic benchmarks from the PHP community apparently indicating a difference).

I think I'll probably change this back to single quotes (as you've mentioned for readability), and since it doesn't offer any observable performance advantage in my testing. (I recall a long time ago that using . over interpolation had an observable negative difference – I guess things have changed now.)

I don't really aim for string interpolation vs. string concatenation here, but rather for defining a complex method name separately.

I don't think that there's a "real" performance difference between string interpolation and string concatenation. I personally prefer string concatenation because I think it's way more readable, especially for complex strings. However, others might say the complete different, so this is basically just a preference.

PhrozenByte · 2018-04-12T20:45:46Z

Parsedown.php

                {
-                    $Inline['element']['nonNestables'][] = $non_nestable;
+                    $Inline['element']['nonNestables'][] = $nonNestable;
                }


$Inline['element']['nonNestables'] = array_merge($Inline['element']['nonNestables'], $nonNestables);

Appending to undefined key creates it so we also have to check that the key exists with this method, e.g.:

$Inline['element']['nonNestables'] = isset($Inline['element']['nonNestables']) ? array_merge($Inline['element']['nonNestables'], $nonNestables) : $nonNestables ;

Still, I wouldn't be surprised if the behaviour currently in use gets warnings added to it at some point. And even if it doesn't, adding the check is still a good thing

aidantwoods · 2018-04-14T14:53:38Z

Parsedown.php

+            "<br />\n",
+            $safeText
+        );
+        $Inline['element']['allowRawHtmlInSafeMode'] = true;


I'm still in two minds about this change – it's quite a bit faster because plaintext is pretty common and it saves us creating several arrays for each chunk of it, however it blinds any AST post-processor to the content structure. It also perhaps risks encouraging use of rawHtml + allowRawHtmlInSafeMode over the AST, which is less than ideal where it can be trivially avoided.

I'm not so sure whether breaks are structural elements or rather styling elements... Paragraphs are structural elements without any doubt, but single line breaks? Furthermore, isn't this a great example of AST processing adding unnecessary complexity? I don't think that we encourage people to use rawHtml + allowRawHtmlInSafeMode - it's rather a great example of how and under which circumstances it can and should be used. We shouldn't condemn allowRawHtmlInSafeMode 😉

I'm not so sure whether breaks are structural elements or rather styling elements...

Yes, you're correct in that they're used for style. I was more going for "structure" here to mean that breaks represent a different kind of object in the syntax tree than plaintext does. I don't really have much concern for why the elements are used (rather that they are elements) :p

I definitely don't want to condemn allowRawHtmlInSafeMode, but if someone needs to create something simple like foobar, then I'd much rather they wrote it like:

$Element = array('name' => 'p', 'text' => 'foobar');

than

$Element = array('rawHtml' => 'foobar', 'allowRawHtmlInSafeMode' => true);

Especially so when foobar becomes a variable.

Even if someone does all the escaping correctly, it's much easier to not to have to if it isn't required.

Perhaps an example of how to use raw HTML in a safe way like this is valuable though.

I definitely don't want to condemn allowRawHtmlInSafeMode, but if someone needs to create something simple like foobar, then I'd much rather …

Perhaps an example of how to use raw HTML in a safe way like this is valuable though.

Full ACK 👍

aidantwoods added 19 commits April 9, 2018 18:08

Use mutating loop instead of array_map

6f13f97

No need to unset if not set

f232702

General readability

90ad738

Use non-nestable values as keys for O(1) lookup

70f5c02

The AST has high complexity here (and so traversal is hard anyway)

dc5cf87

We gain quite a bit of a speed boost by working with text here since this is a very common function

Optimise commonly used regexes to fail fast

d6e306d

String interpolation is slightly faster than concat

d4f1ac4

Avoid recomputation

107223d

Make some regexes possesive

b42add3

Remove unneeded complete function

2faba6f

Use standard library over while loop

c45e419

Remove use of array

3ea0814

Use standard library function

b53aa74

In theory PHP stores the length of strings, so looking this up should…

e74a5bd

… be quick

Remove regex from fenced code block

d2dd736

Also remove unused function

Remove regex from block rule

7e15d99

More expensive statement last

450a74f

Sanity checks before starting regex engine

726d4ef

Rewrite as one statement

88a3f31

aidantwoods added experimental WIP labels Apr 12, 2018

PhrozenByte reviewed Apr 12, 2018

View reviewed changes

aidantwoods added 3 commits April 12, 2018 22:16

Readability improvements, thanks @PhrozenByte

3e70819

PHP 5.3 == 💩

7f4318d

Remove complex string interpolation expressions

a9764ec

aidantwoods commented Apr 14, 2018

View reviewed changes

aidantwoods removed WIP experimental labels May 7, 2018

aidantwoods added this to the 1.8.0 milestone May 7, 2018

aidantwoods merged commit 8d172a2 into erusev:master May 7, 2018

aidantwoods deleted the enhancement/performance-tweaks branch May 7, 2018 10:18

aidantwoods mentioned this pull request May 8, 2018

Preserve plain-text in AST to avoid blinding extensions to it #628

Merged

dregad mentioned this pull request Jun 7, 2018

BC break: Removal of blockFencedCodeComplete() method in 1.8.0-beta-2 #640

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General and performance tweaks #614

General and performance tweaks #614

aidantwoods commented Apr 12, 2018 •

edited

Loading

PhrozenByte Apr 12, 2018

aidantwoods Apr 12, 2018

PhrozenByte Apr 12, 2018 •

edited

Loading

aidantwoods Apr 12, 2018

PhrozenByte Apr 12, 2018

PhrozenByte Apr 12, 2018 •

edited

Loading

aidantwoods Apr 12, 2018 •

edited

Loading

aidantwoods Apr 14, 2018

PhrozenByte Apr 14, 2018

aidantwoods Apr 14, 2018

PhrozenByte Apr 14, 2018

General and performance tweaks #614

General and performance tweaks #614

Conversation

aidantwoods commented Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhrozenByte Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhrozenByte Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

aidantwoods Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aidantwoods commented Apr 12, 2018 •

edited

Loading

PhrozenByte Apr 12, 2018 •

edited

Loading

PhrozenByte Apr 12, 2018 •

edited

Loading

aidantwoods Apr 12, 2018 •

edited

Loading