484100 - Optimize webroot/tiki-editpage.php and associated code

Status: VERIFIED FIXED

(support.mozilla.org :: Knowledge Base Software, task)

Description

[E. Cooper [:ecooper]]

It seems that our timeout troubles on staging are being caused by the code in tiki-editpage.php.

We're seeing out of control execution times:
[Wed Mar 18 23:00:01 2009] [error] [client 10.2.81.4] PHP Fatal error:  Maximum execution time of 30 seconds exceeded in /data/www/support-stage.mozilla.com/webroot/setup_smarty.php on line 107, referer: https://support-stage.mozilla.org/tiki-editpage.php?page=jkdlsf&quickedit=Edit

And high memory usage:
PHP Fatal error: Allowed memory size of 33554432 bytes exhausted (tried to
allocate 40961 bytes)

You can also view the timeout bug here for other bits and pieces <https://bugzilla.mozilla.org/show_bug.cgi?id=480415>.

I'm assigning this to me, but if someone finds time on their hands, we can tag team this.

Comment 1

[Paul Craciunoiu [:paulc]]

I'm not sure about time, but I'd be willing to help out. If you want, you can try to sub-assign a specific piece of code to me once you get going.

Comment 2

[Paul Craciunoiu [:paulc]]

Eric, how's this? I could spend some time on it, mostly tomorrow though. If you've got some input or want me to take it, let me know!

Comment 3

[E. Cooper [:ecooper]]

I see this as a general 'benchmark and see' sort of bug...trim the fat, if you will. So, find the slow, wasteful parts and make them faster. ;)

That being said, I haven't had a chance to do any benchmarks on it yet. That'd be a good start. Or if you want to see if the function at setup_smarty:107 can be optimized, that'd be good too.

Comment 4

[Laura Thomson :laura]

Paul, Eric - this is the most important bug still open on 1.0.  Without it we can't edit the l10n dashboard.  Please drop everything else and work on this one.

Comment 5

[E. Cooper [:ecooper]]

Waiting for input from Nelson regarding the poll data fetched in webroot/tiki-editpage.php that's only displayed for non-KB categories.

As I've mentioned, I don't understand why we would need it. Getting rid of that seems to get us out of the red for that page. 

For additional reference: see webroot/tiki-editpage.php:968-995.

Comment 6

[Laura Thomson :laura]

Eric, can you post a patch for testing?

Comment 7

[Laura Thomson :laura]

PS this is blocking 1.0 since we can't edit the l10n dashboard.

Comment 8

[Nelson Ko [:nkoth]]

For sumo, we have custom behavior in poll_categorize.php that creates CSAT polls, it is commented as

// for Mozilla, wiki pages in the knowledge base has polls of certain questions

This should not depend on there being any polls set in tiki-editpage.php. Your recommended disabling of handling poll amendment from tiki-editpage.php seems ok.



(In reply to comment #5)
> Waiting for input from Nelson regarding the poll data fetched in
> webroot/tiki-editpage.php that's only displayed for non-KB categories.
> 
> As I've mentioned, I don't understand why we would need it. Getting rid of that
> seems to get us out of the red for that page. 
> 
> For additional reference: see webroot/tiki-editpage.php:968-995.

Comment 9

[E. Cooper [:ecooper]]

Attachment: Get rid of poll processing in tiki-editpage.php

So this patches gets rid of the fetching of poll info that occurs during every load of tiki-editpage.php.

Some notes about the patch: 
* The diff of tiki-editpage.tpl is used to show what will be different about the tiki-editpage.tpl that is being created in /templates/styles/mozad/. Showing the new template wouldn't be helpful. Since the default tiki-editpage.tpl is used currently, this patch can be used for testing directly, however.
* preg_match is called 12417 vs 96025. That's a huge difference.
* We need to test to make sure CSAT stuff isn't affected on KB articles. It shouldn't be, but, it's something to check.

Comment 10

[Laura Thomson :laura]

Comment on attachment 369565 [details] [diff] [review]
Get rid of poll processing in tiki-editpage.php

wfm.  Let's get it into trunk so everybody can hit it on stage.

Comment 11

[Laura Thomson :laura]

Also: it seems hella faster.

Comment 12

[E. Cooper [:ecooper]]

This is in trunk at r23844.

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #12)
> This is in trunk at r23844.

I've tested both page creation and page editing, and both are expedient and I don't see any regression from this patch (on staging).

Comment 14

[Laura Thomson :laura]

Merge please!  :)

Comment 15

[E. Cooper [:ecooper]]

At r23854 in prod.

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

Seems to be faster when it works, but we're also still affected by bug 480415, as cilias points out; not sure what the criteria are for this bug's verifcation, in light of that.

Comment 17

[Laura Thomson :laura]

I'm ok with calling this one done since it intended to be l10n dashboard specific, and focusing on that one.  I'm hoping the database upgrade will help as well.

Comment 18

[Stephen Donner [:stephend] Not actively reading bugmail]

OK, thanks -- I agree, and every new (tigher-focused) bug should help us make specific and more measurable tweaks.

Comment 19

[Louis-Philippe Huberdeau]

Not to be upstreamed. The patch is essentially commenting out a block not used on SUMO.

Comment 20

[Louis-Philippe Huberdeau]

I've looked a bit deeper in the issue.

The code was modified since, but there are possible solutions. To begin with, there are no indexes on the table. As most searches are done on the active flag, indicating type or object, no index on that field can't be good on large volumes.

I also have the feeling the filtering is just broken as the poll name is sent as the second parameter, which is a date filter.

I would need a database dump with the usage patterns on SUMO to work with this issue.

Comment 21

[Louis-Philippe Huberdeau]

As expected, the bad filter clause was causing the problem. I added an index in case to avoid a table scan, but just fixing the filter changed the proportion of execution time from 23% to negligible.