Fix GH-11507: String concatenation performance regression in 8.3

[nielsdos]

When the code was moved to solve the uaf for memory overflow, this caused the refcount to be higher than one in some self-concatenation scenarios. This in turn causes quadratic time performance problems when these concatenations happen in a loop.

[nielsdos]

I just hope the nullifying won't cause the benchmarks to slow down... At least we know the root cause now so that's a start.

[andypost]

Used to build with this patch but it does not fix

php-src$ sapi/cli/php ~/perf-concat.php 
Iteration 0 - 2.3009777069092 ms
Iteration 1 - 2.8760433197021 ms
Iteration 2 - 8.0420970916748 ms
Iteration 3 - 8.4400177001953 ms
Iteration 4 - 9.9489688873291 ms
Iteration 5 - 16.154050827026 ms
Iteration 6 - 19.223928451538 ms
Iteration 7 - 20.49708366394 ms
Iteration 8 - 20.027875900269 ms
Iteration 9 - 24.285078048706 ms

[iluuu1994]

Looks correct, thanks for fixing this!

[iluuu1994]

Release/free first, right?

[nielsdos]

Yes of course 🤦 clearly not awake anymore :p

[iluuu1994]

@andypost Sure? It did for me.

[andypost]

I was wrong, somehow patch is not applied, rebuilding

[andypost]

yes, it works, thank you!

[nielsdos]

@iluuu1994 Although this might be correct, this worsens the benchmark results. It probably makes sense to find a more optimal way to fix this.

[iluuu1994]

@nielsdos How big was the difference? Was it a timed benchmark or instruction count?

[nielsdos]

@nielsdos How big was the difference? Was it a timed benchmark or instruction count?

Instruction count. Actually, never mind... It was not this PR that caused this. It was an earlier commit today that caused the slowdown (see https://github.com/php/php-src/actions/runs/5343735245/jobs/9687223322) in particular WordPress is now 128427072 instead of 124665438
It might be coincidence... although I've seen PCRE high up in profiles so I wouldn't be surprised if that PCRE change caused this slowdown.

[andypost]

Current PR reports 128432662 https://github.com/php/php-src/actions/runs/5349418998/jobs/9700608975?pr=11508#step:10:38

but 2 weeks ago it was 124643600 - 4M ops added

[nielsdos]

Current PR reports 128432662 https://github.com/php/php-src/actions/runs/5349418998/jobs/9700608975?pr=11508#step:10:38

but 2 weeks ago it was 124643600 - 4M ops added

Yeah, the root cause is this commit I think: 466fc78. Starting from the merge from that to master it slowed down with 4M ops.

[iluuu1994]

It might be coincidence... although I've seen PCRE high up in profiles so I wouldn't be surprised if that PCRE change caused this slowdown.

This is probably caused by the allocation in the else case 🙁 We'll need to revert that and solve it some other way. We can use separate caches for JIT and non-JIT. That should avoid it.

[iluuu1994]

I reverted this commit for now. I'll try a different solution tomorrow.

[nielsdos]

It might be coincidence... although I've seen PCRE high up in profiles so I wouldn't be surprised if that PCRE change caused this slowdown.

This is probably caused by the allocation in the else case slightly_frowning_face We'll need to revert that and solve it some other way. We can use separate caches for JIT and non-JIT. That should avoid it.

Or have an extra field in pcre_cache_entry to indicate whether it was JITed or not?

[iluuu1994]

@nielsdos Good idea, we'll have to postpone to 8.3 due to the ABI break then. But that's fine, I don't want to risk another issue.

[andypost]

Thank you! As I see "4M ops" are fixed in latest HEAD https://github.com/php/php-src/actions/runs/5350588341/jobs/9703387273#step:10:38