Do not reallocate whole input string when matching next token

[mvorisek]

Purely a performance refactoring.

The measured performance improvement is about -90% reduced runtime for the included test. With larger test/SQL, the speedup is even more dramatic as the original complexity was O(N^2).

Originally, the whole input string was reallocated after each token in:

• https://github.com/doctrine/sql-formatter/blob/1.4.0/src/Tokenizer.php#L790

such big temporary string was upper cased for each token matching in:

• https://github.com/doctrine/sql-formatter/blob/1.4.0/src/Tokenizer.php#L890

and regex patterns were constructed for each token matching freshly in: (extracted into another PR)

• https://github.com/doctrine/sql-formatter/blob/1.4.0/src/Tokenizer.php#L874

All such operations should be avoided for linear scalability and this PR addresses that.

[mvorisek]

PR is finished and the performance improvement (of the "long concat" tests with new 20k elements limit) is the following:

before:

21.370 secs Doctrine\SqlFormatter\Tests\SqlFormatterTest::testFormatLongConcat
18.474 secs Doctrine\SqlFormatter\Tests\TokenizerTest::testTokenizeLongConcat

after

5.710 secs Doctrine\SqlFormatter\Tests\SqlFormatterTest::testFormatLongConcat
1.665 secs Doctrine\SqlFormatter\Tests\TokenizerTest::testTokenizeLongConcat

So the tokenization process is newly about 10x faster and with increased query length the speedup is even bigger.

This PR is faster also on the tests/performance.php script even it uses very small queries:

before:

$ tests\performance.php                        
    <p>Formatted 54 queries</p>
    <p>Average query length of 284.37037 characters</p>
    <p>
        Took 0.02518 seconds total,
        0.00047 seconds per query,
        0.00164 seconds per 1000 characters
    </p>
    <p>Used 0 bytes of memory</p>

(2nd run)
$ tests\performance.php
    <p>Formatted 54 queries</p>
    <p>Average query length of 284.37037 characters</p>
    <p>
        Took 0.02403 seconds total,
        0.00044 seconds per query,
        0.00156 seconds per 1000 characters
    </p>
    <p>Used 0 bytes of memory</p>

after:

$ tests\performance.php
    <p>Formatted 54 queries</p>
    <p>Average query length of 284.37037 characters</p>
    <p>
        Took 0.01587 seconds total,
        0.00029 seconds per query,
        0.00103 seconds per 1000 characters
    </p>
    <p>Used 0 bytes of memory</p>

(2nd run)
$ tests\performance.php
    <p>Formatted 54 queries</p>
    <p>Average query length of 284.37037 characters</p>
    <p>
        Took 0.01543 seconds total,
        0.00029 seconds per query,
        0.00101 seconds per 1000 characters
    </p>
    <p>Used 0 bytes of memory</p>

[mvorisek]

Please split this; there seems to be changes that could be indenpendent, for instance "Remove historical comments" can be split out.

commit removed

[greg0ire]

Are there ways you could split this in several smaller PRs?

[mvorisek]

Are there ways you could split this in several smaller PRs?

Yes - done now, here we focus on "no reallocation", ie. using $offset strictly.

[greg0ire]

Is 3c2a6db about avoiding allocations? It seems to allocate more variables.

[mvorisek]

Is 3c2a6db about avoiding allocations? It seems to allocate more variables.

Such short allocations are fast and more readable over $str[$offset] and $str[$offset + 1]. Especially, single char string is always interned thus no real allocation is done. This is thanks to internal php optimization.

However I tested it - offset is slightly faster over substr($str, $offset, 1). I belive this is because specific opcode is used over regular function call/overhead.

[greg0ire]

Since it is not related (and even the opposite of what this PR is about, reducing allocations), then let's move the 2 commits in a separate PR please.

[mvorisek]

Like really, do I have to use separate PR for using separate variable when this PR is refactoring this topic and the performance was tested?

[greg0ire]

Do I have to deal with PRs with 7 commits? 7, really? What the hell?

[greg0ire]

This PR had better be strictly about removing allocations, not adding some.

[mvorisek]

done

[greg0ire]

Thanks @mvorisek !