Build regexes only once

[mvorisek]

Huge speedup when the Tokenizer is created many times (not cached/reused in user app).

[derrabus]

Huge speedup when the Tokenizer is created many times (not cached/reused in user app).

Is there a reason why I would want to destroy and recreate the Tokenizer all the time?

[mvorisek]

Is there a reason why I would want to destroy and recreate the Tokenizer all the time?

See https://github.com/atk4/data/blob/6fbe7e23a4/src/Persistence/Sql/Expression.php#L437 - the regex build is quite slow, so if someone use it like this, this PR will help.

[derrabus]

I'd rather fix the code you've linked, tbh.

[mvorisek]

Sure, we already did, and let's fix it here too.

[derrabus]

No.

[mvorisek]

@greg0ire can you please reopen this PR - I did a test with:

for ($i = 0; $i < 100_000; $i++) {
    (new Tokenizer())->tokenize('select 1');
}

and the speed difference is 500x - currently, single Tokenizer bootstrap takes ~1.5 ms!

Creating the regexes from huge string lists is quite slow process and even when the Tokenizer is reused a few times, this PR imrpoves real world performance by a big amount!

Imagine usecases like:

• the Tokenizer used in many user libs within a single app - impossible to be fixed by user
• the Tokenizer instanciated in some class that is instanciated many times too - I belive we do not want to recommend assigning the Tokenizer/SqlFormatter into static property because of this
• SqlFormatter created with multiple highligters - https://github.com/doctrine/sql-formatter/blob/1.4.0/src/SqlFormatter.php#L36 - also impossible to be fixed by user

[greg0ire]

None of these use cases seem realistic.

[mvorisek]

Please, the improvement is really huge and (new SqlFormatter())->format($query) syntax is very common, actually the popular doctrine/migrations does use it itself - https://github.com/doctrine/migrations/blob/3.7.4/lib/Doctrine/Migrations/Generator/SqlGenerator.php#L58

I hope this provides strong evidence so let me rephrase the question differently, what is againt this PR - the class is final, so the regexes can be easily hold within static property.

[derrabus]

actually the popular doctrine/migrations does use it itself

We should indeed fix that as well.

Sorry, I certainly don't want a static cache here. And honestly, if you put the tokenizer on a hot path and create and destroy 100k instances of that class instead of reusing a single instance, you had it coming.

[greg0ire]

actually the popular doctrine/migrations does use it itself

We should indeed fix that as well.

Even then, I doubt anybody will notice the resulting performance improvement. Maybe generating the initial migration of a project will get slightly faster, but after that… meh.

[mvorisek]

@greg0ire the speed difference is 500x and although the migratins speedup might be only ~100 ms, as long as someone would use it for web, even if for debug only, the unwanted slowdown should be avoided - please, is there really anything againts this PR?

[greg0ire]

as long as someone would use it for web

What do you mean by that? Do people generate migrations through a web interface nowadays?

[mvorisek]

In atk4, we use it to format SQL debug/log queries, witch this feature (and not reusing the SqlFormatter instance), the slowdown can be easily the whole page render.. So given how easy but powerful this PR is, I do not see a major problem by making it fast when not used perfectly.

[derrabus]

I do not see a major problem

I do see a major problem with static caches. This is a no, please stop wasting our time.

[mvorisek]

OK - because of memory kept allocated?

[derrabus]

actually the popular doctrine/migrations does use it itself

We should indeed fix that as well.

doctrine/migrations#1436