Looks fine

Looks good, should be RTC.

Hello @hordijk

Thank you for your contribution.
Please provide clear test instructions to be able to test this issue.
Does we need to create a big article (>1Mb), or something else ?

Thanks for understanding!

@garstud It's just a little performance tweak. max_execution_error is only a side effect. For testing you should load a large document (e.g. write a very large article and look at it on frontend).

Tried to get a time out with a massive article but failed so I cant test

@brianteeman You don't need to get a time out. Just compare the execution time.

And of course SEF has to be enabled.

The instructions and title both say " Fix max_execution_timeout exception"
Not compare execution time

On 25 May 2015 at 19:43, Nils Rückmann notifications@github.com wrote:

And of course SEF has to be enabled.

—
Reply to this email directly or view it on GitHub
#5741 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@brianteeman As max_execution_time is thrown after a certain time limit, it seems obvious to compare the execution time.

Clearly it wasnt obvious to me

On 25 May 2015 at 21:24, Nils Rückmann notifications@github.com wrote:

@brianteeman https://github.com/brianteeman As max_execution_time is
thrown after a certain time limit, it seems obvious to compare the
execution time.

—
Reply to this email directly or view it on GitHub
#5741 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Test Instructions:

1. Write an article with a significant amount of internal links.
2. Make sure that SEF is enabled.
3. Browse to your article in the frontend
4. Measure the time to load the page (for example with debug-mode)
5. Apply the patch
6. Measure the time to load the page again.
7. Compare results.

Expectation:

After applying the patch, the page should be a bit faster without any side effects.

Tried to test it but the results are changing... :/

Tried to test it but I cannot verify an improvement.

I have tested this item successfully on 19bf76b

before patch:
regex search for 1 or more spaces
after patch:
regex check if 1 space exists

the improvement is obvious

Thank you for creating this. It’s been some time since you created this and there are now some merge conflicts that prevent a direct merge. Could you please update your PR? Thank you!

@hordijk Did you see the message above?

Ok with 1000 cases of 5000 spaces before src="..."

for ($i=0; $i < 1000; $i++)
{
    echo '<img '.str_repeat(' ', 5000).' src="http://www.some_domain_test.org/img.png" />';
}

Application afterRender

Before: 36,000 miliseconds 15 KB (compressed) / 15 KB (uncompressed)
After: 60 miliseconds 5 KB (compressed) / 4956 KB (uncompressed)

It is enough to accept on code review, it is really waste of CPU cycles to put a preg_match that starts on (without a good reason): \s+

but i also tested, (easy to test just use the code above ...)

A comment , it is not ONLY cases of space before src

• it is ALL the cases of consecutive "white-spaces" characters that TRIGGER the regexp

think of it, it starts reading whitespace and then finds a word e.g. hello and it stops, but it has started to read the whitespace, that is the reason that this PR needs to be tested and accepted !

e.g. this will have the same effect

for ($i=0; $i < 1000; $i++)
{
    echo str_repeat(' ', 5000).' hello ';
}

I have tested this item ✅ successfully on 19bf76b

Please read my previous comment and test, the more spaces you have on a large document, the slower your joomla page gets , and this happens for no good reason
(and reason for removing spaces to make page small at a terrible cpu cycle cost, and only of the case before src/href/... is silly)

Sorry for the delay, I finally updated the PR.

I have tested this item ✅ successfully on 96fe585

Please see my above comment about code to test performance, about correctness you can do a core review too

https://github.com/joomla/joomla-cms/pull/5741/files

(set back to pending as the merge conflicts were resolved)

This had 2 successful tests but due to rebase ,
it show only 1 test

Also it had code reviews and the fix is obvious too,

Please see my analysis above ,
on why this is a big performance issue in large documents, the effect becomes exponential

#5741 (comment)
#5741 (comment)

This can be merged
Also this is a good reminder to avoid regexp on html that do not start with a fixed text !

I have tested this item ✅ successfully on 96fe585

Ok now this had 3 tests

Setting RTC as we have multiple good tests.