Bug 1190171 - Revision content denormalization and other cleanups of the compare revision view.

[jezdez]

• Adds ratelimiting to the compare revision view.
• Refactors the signal handling to use new patterns and less confusing code.
• Prefetch revision tags and document for fewer queries.
• Denormalize the revision content in a separate field that is filled on save and uses tidy to do the CPU-intense work once instead of on every request.

[jezdez]

So this does a bunch of things, which we don't have to do all at once, but I wanted to offer it anyway since I think we shouldn't run tidy on every request of that view but keep the tidied revision content on file instead.

The other changes are probably less troubling.

[groovecoder]

Ah, this is a good way to preserve the submitted data (in the content field) and store the tidied content. But should we populate the tidied_content field when it's accessed via get_tidied_content() or should we just populate it when the Revision is saved?

[jezdez]

It's populated both, see https://github.com/mozilla/kuma/pull/3386/files#diff-a103da980103930e43dab15a81a2a4f5R128

[jezdez]

The idea is to not have to go through all the hundred of thousands revisions at once but spread it a bit. Updating the tidied content on access is only useful for old revisions, the post_save hook is for all new revisions.

[groovecoder]

Doh, sorry I over-looked that; I jumped straight to this models.py diff to see what you did here.

[groovecoder]

❤️ using Django 1.7 app system.

[groovecoder]

Needs a rebase now.

[groovecoder]

Intern tests pass. Spot-check on get_tidied_content() from a django shell works as expected - empty at first, triggers a celery task, then it's populated next time. Saving a new revision populates tidied_content before get_tidied_content is called.

[groovecoder]

This is still 60/m?

[jezdez]

I actually decided to increase the rate limit of the tidy_revision_content Celery task to 120/m. The reasoning is simple: we have 550k revisions right now, with a rate limit of 15/m we'd wait 12 days for even running the tasks. On the other hand, MySQL is currently able to handle UPDATE queries very fast and hasn't hit performance ceilings (not even close) with that when I looked at the last week's stats. We also are able to have the Celery nodes run through the CPU intense tidying easier given we can spread it to their collective CPUs, in other words, they aren't fully utilized right now as well.

That said I agree that there is a big risk with scheduling Celery tasks on every call to get_tidied_content since if assuming we create a big stack of tasks by normal site browsing it's guarenteed that we'll create duplicates of the tasks. That's why I've added a simple cache based check to the get_tidied_content function that prevents duplicate scheduling. It's set to timeout after 3 days, after which a call to get_tidied_content would again schedule a task. The calculation for that timeout is: (546235 / 120) / 60 / 24 = 3.1 days -- the number of days it takes in theory to run through all revisions.

[groovecoder]

Note: It's set to 120/m, which I assume is correct. (Not 120/s written in the comment. 😉)

[jezdez]

@groovecoder woops, good catch!

Edit: Fixed.