436877 - Make "Since your last visit" not affect performance

Status: VERIFIED FIXED

(support.mozilla.org :: General, defect)

Description

[Jason Barnabe (np)]

http://groups.google.com/group/mozilla.support.planning/browse_frm/thread/277c67bb61cb0b73/89c456a105bce7ce

Limiting the number of items it'll load should do it.

Comment 1

[myles7897]

another way could be limit it by language.  It displayed changes for all wiki-pages, including those in a different language.

Comment 2

[David Tenser [:djst]]

I'd like to put this on the radar as the functionality is important for contributors but performance is hindering us from using it.

Comment 3

[E. Cooper [:ecooper]]

Is it correct to assume we're talking about webroot/modules/mod-since_last_visit_new.php?

Comment 4

[E. Cooper [:ecooper]]

Attachment: First pass at new "since your last visit" mod

Instead of hacking the current webroot/modules/mod-since_your_last_visit_new.php, I created a new module and template. It reuses the same code architecture of the original, but adds limits for display and separates the counting query from the actual data-fetching query.

That being said, I'm still a little unsure about this, and I'm not sure how useful this will be.

I think the better alternative would be something like a devoted page contributors can visit to see changes since their last visit.

Comment 5

[E. Cooper [:ecooper]]

Attachment: Screenshot of first pass

Bottom is expanded, top is not.

Limits can be specified for each type of change with a failsafe default of 25 (which can also be changed). In the screenshot, new posts have been arbitrarily limited to displaying the latest 3.

Comment 6

[Chris Ilias [:cilias]]

Where is this enabled? I forgot. I don't see any "since last visit" module in <https://support-stage.mozilla.org/tiki-admin_modules.php>.

Comment 7

[E. Cooper [:ecooper]]

On tiki-admin_modules.php, look under the "Assign new module". The original is listed under the "Module name" drop-down as "since_last_visit_new". It's currently disabled.

The patched version isn't on staging.

Comment 8

[E. Cooper [:ecooper]]

Did some basic benchmarking. The old queries seem to be able to return large datasets in a relatively timely manner (especially compared to some of tiki's more frequently used queries). This new module is faster never the less.

If anything, the problem looks like PHP had to also loop through the entire set (which as Jason noted can be massive), do some conditionals, do some permission checks, etc.

Comment 9

[David Tenser [:djst]]

How much faster is the new module - 5%, 50%, 500%?

Comment 10

[E. Cooper [:ecooper]]

As I mentioned, the problem doesn't look like it's an issue with db queries (though, they've been made much more scalable), but with the code handling what the queries were returning.

As an example, passing 1000 new forum posts (Jason's original report said he saw 1700) to old module will cause a timeout locally. That means >60s execution time for the entire script (most likely poor smarty trying to parse and loop through a 1K array). The new module takes an extra .7s to load the page with the same data.

The real issue here is parsing and then passing massive arrays to smarty is very, very bad for script execution time.

(In reply to comment #9)
> How much faster is the new module - 5%, 50%, 500%?

Comment 11

[E. Cooper [:ecooper]]

Oh, I should also mention these numbers are local. On staging or production, performance should be even better.

Comment 12

[Paul Craciunoiu [:paulc]]

Maybe it would be more efficient to create separate handling for this in PHP and somehow split the display of the site in such a way that the array is not passed to smarty?

For example, if we smarty->display the parts before and after the array using some smarty templates, but keep the array part in php. Would this work? Perhaps through duplicating some templates content. Maybe it's worth evaluating the work costs of doing this versus not.

Comment 13

[E. Cooper [:ecooper]]

If you passing a massive array to smarty, you're probably doing something wrong on the UX side of things. ;)

In any case, is everyone on board with me committing this new module for testing/refinement on staging?

Comment 14

[David Tenser [:djst]]

Eric, can you get a review from e.g. Laura first? Otherwise I like the plan!

Comment 15

[E. Cooper [:ecooper]]

Moving this to 1.0.

I'm getting a JS error when included on the contributor homepage and *only* the contributor homepage. If I figure it out tonight, I'll move it back into 0.9.

Comment 16

[David Tenser [:djst]]

What's the status of this bug?

Comment 17

[E. Cooper [:ecooper]]

(In reply to comment #16)
> What's the status of this bug?

Should be in on staging Tuesday or Wednesday depending on the l10n dashboard progress.

Comment 18

[E. Cooper [:ecooper]]

I can't seem to reproduce the error in comment 15 after taking another look at this on two separate occasions over the last week. It's in r23339/r23340 and can be activated on staging by using the since_last_visit_lite module.

Comment 19

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

(In reply to comment #18)
> I can't seem to reproduce the error in comment 15 after taking another look at
> this on two separate occasions over the last week. It's in r23339/r23340 and
> can be activated on staging by using the since_last_visit_lite module.

Error: setfolderstate is not defined
Source File: https://support-stage.mozilla.org/en-US/kb/Contributor+Home+Page
Line: 1030

Eric, was that the error message you saw?

Comment 20

[E. Cooper [:ecooper]]

(In reply to comment #19)
> Error: setfolderstate is not defined
> Source File: https://support-stage.mozilla.org/en-US/kb/Contributor+Home+Page
> Line: 1030
> 
> Eric, was that the error message you saw?

No. I think that's related to the collapsible Admin folders in the right nav.

The error I saw prevented the module from expanding and collapsing. For some reason, an inline rule for visibility was (incorrectly) being set. I haven't seen it since, though.

Comment 21

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

I don't see comment 15's error either.

Marking verified fixed; I don't see end-user performance hits due to this, and it looks like Eric has done some performance testing too.

(Reopen if the crack I'm smoking is better than usual!)

Comment 22

[Louis-Philippe Huberdeau]

As we've been trying to reduce module duplication very recently, introducing yet an other module for recent changes is not an option. The fact that it's a separate file makes it very hard to see the changes.

One of the major changes is obviously the limitation of records displayed. Are there any other changes?

I would upstream this change by adding a parameter to the module instead.

Comment 23

[James Socol [:jsocol, :james]]

It seems to me that if LPH's solution in comment 22 isn't sufficient for us, we could always go back to using Eric's module in the SUMO installation. I'm not sure what other changes were made, but I'm pretty comfortable with whatever you think is best, Louis-Philippe.

Comment 24

[Louis-Philippe Huberdeau]

The most important part of my message was "Are there any other changes?".

Comment 25

[Louis-Philippe Huberdeau]

Applied a maximum amount of results fetched. If there are still performance issues after the port, they should be tracked individually rather than reverting to the old module.