Page MenuHomePhabricator
Create Task
Maniphest T228675

Remove direct access to the text table from the Translate extension.
Closed, ResolvedPublic3 Estimated Story Points
Actions

Assigned To
• Pchelolo
Authored By
daniel
Jul 22 2019, 3:11 PM
Tags
Referenced Files
F30463407: image.png
Sep 25 2019, 11:25 AM
F30453070: image.png
Sep 25 2019, 11:25 AM
Subscribers
Aklapper
daniel
gerritbot
Jdforrester-WMF
Nikerabbit
• Pchelolo
Tgr
tstarling

Description

The Translate extension relies on direct access to the text table, though joining against rev_text_id, in several places. This will no longer be possible with the new MCR schema, and should be replaced as described in T198341, T198342, and T198343.

The Translate extension is a somewhat special case as the transition to the new schema may cause problems with respect to backwards compatibility with older versions of MediaWiki, as well as performance issues, since bulk queries of page content are no longer possible.

Details

SubjectRepoBranchLines /-
Use batch access for revisions instead of Revision::getRevisionTextmediawiki/extensions/TranslateREL1_34 370 -100
Replace Revision::getRevisionText for importer and maintenance scriptsmediawiki/extensions/TranslateREL1_34 172 -74
Replace Revision::getRevisionText for importer and maintenance scriptsmediawiki/extensions/Translatemaster 172 -74
Use batch access for revisions instead of Revision::getRevisionTextmediawiki/extensions/Translatemaster 370 -100
RevisionStore: Introduce getContentBlobsForBatchmediawiki/coremaster 408 -40
Remove direct access to text tablemediawiki/extensions/Translatemaster 69 -47
Don't use rev_text_id to update translation tag for null revisionmediawiki/extensions/Translatemaster 57 -27
Customize query in gerrit

Related Objects
Search...

StatusSubtypeAssignedTask
 ResolvedNoneT231668 MCR schema migration
 ResolvedZabeT231674 [Epic] Remove support for writing to the pre-MCR schema
 ResolveddanielT198557 Remove the ability to write pre-MCR fields, limit the ability to read pre-MCR fields to migration scripts
 ResolvedCCicalese_WMFT242764 Test schema update from old versions to MCR-only schema
 ResolveddanielT231673 Make SCHEMA_COMPAT_NEW the default MCR migration schema
 ResolvedNoneT231672 Stop writing to pre-MCR fields in WMF production
 ResolvedNoneT231671 [EPIC] Ensure all direct or indirect access to pre-MCR fields is gated with the MCR migration stage and emits a warning if any pre-MCR schema fields are accessed
 ResolvedCCicalese_WMFT231693 No direct or indirect access to pre-MCR fields is ungated
 ResolveddanielT198341 Remove all references to the rev_text_id and ar_text_id fields
 Resolved• PcheloloT198342 Remove all usages of the 'text' flag in calls to Revision::getQueryInfo() and RevisionStore::getQueryInfo().
 ResolveddanielT198706 Make BackupDumper, WikiExporter and XmlDumpWriter compliant with the MCR revision retrival mechanism (main slot only)
 ResolvedBPirkleT203424 Replace the WikiExporter backup dump streaming mode with batched queries
 Resolved• PcheloloT198343 Replace all calls to Revision::getRevisionText()
 Resolved• PcheloloT228675 Remove direct access to the text table from the Translate extension.
 Resolved• PcheloloT228988 Create batch access interface for page content
 Resolved• PcheloloT233173 Make RevisionStore::newRevisionsFromBatch use BlobStore::getBlobBatch
 Resolved• PcheloloT230834 Create batch access interface for BlobStore
 ResolveddanielT234034 Create low level batch access interface for page content
 ResolveddanielT233350 ActiveAbstract: remove dependency on pre-MCR database schema
 ResolveddanielT238958 Remove pre-MCR schema fields during update
 ResolvedNoneT174047 Hide deprecated/unused fields on toolforge replica [MCR]

Event Timeline

daniel created this task.Jul 22 2019, 3:11 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 22 2019, 3:11 PM
daniel added parent tasks: T198341: Remove all references to the rev_text_id and ar_text_id fields, T198342: Remove all usages of the 'text' flag in calls to Revision::getQueryInfo() and RevisionStore::getQueryInfo()..Jul 22 2019, 3:12 PM
daniel mentioned this in T198341: Remove all references to the rev_text_id and ar_text_id fields.Jul 22 2019, 3:19 PM
daniel mentioned this in T198342: Remove all usages of the 'text' flag in calls to Revision::getQueryInfo() and RevisionStore::getQueryInfo()..Jul 22 2019, 3:22 PM
daniel mentioned this in T198343: Replace all calls to Revision::getRevisionText().
gerritbot added a comment.Jul 22 2019, 3:24 PM

Change 524593 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/extensions/Translate@master] Remove direct access to text table

https://gerrit.wikimedia.org/r/524593

gerritbot added a project: Patch-For-Review.Jul 22 2019, 3:24 PM
daniel added a project: Language-Team.Jul 22 2019, 7:28 PM

Tagging the language team for input on performance implications and backwards compatibility. @Nikerabbit, any thoughts?

• WDoranWMF triaged this task as High priority.Jul 22 2019, 10:05 PM
• WDoranWMF added a project: Platform Team Workboards (Clinic Duty Team).
• WDoranWMF moved this task from Inbox to Later on the Platform Team Workboards (Clinic Duty Team) board.
• WDoranWMF moved this task from Later to Ready (WIP:5) on the Platform Team Workboards (Clinic Duty Team) board.Jul 22 2019, 11:16 PM
tstarling subscribed.Jul 24 2019, 9:05 PM

"Bulk queries of page content are no longer possible" -- a performance regression like this this is unacceptable in my view.

• WDoranWMF lowered the priority of this task from High to Medium.Jul 24 2019, 9:47 PM
• WDoranWMF moved this task from Ready (WIP:5) to Later on the Platform Team Workboards (Clinic Duty Team) board.
• WDoranWMF moved this task from MCR to mop on the Platform Engineering board.Jul 26 2019, 6:38 PM
• WDoranWMF edited projects, added Core Platform Team Initiatives (MCR); removed Platform Engineering (MCR).
CCicalese_WMF edited projects, added Core Platform Team Initiatives (MCR Schema Migration); removed Core Platform Team Initiatives (MCR).Aug 1 2019, 3:35 AM
• WDoranWMF moved this task from Later to Inbox on the Platform Team Workboards (Clinic Duty Team) board.Aug 6 2019, 3:20 PM
CCicalese_WMF removed a subtask: T228988: Create batch access interface for page content.Aug 30 2019, 10:37 PM
CCicalese_WMF edited projects, added Platform Team Workboards (Architecture Review Workboard); removed Platform Team Workboards (Clinic Duty Team).Sep 17 2019, 4:07 PM
• WDoranWMF moved this task from Backlog to Next Sprint on the Platform Team Workboards (Architecture Review Workboard) board.Sep 17 2019, 4:07 PM
• WDoranWMF moved this task from Next Sprint to Engineering Tasks Ready for Estimation on the Platform Team Workboards (Architecture Review Workboard) board.
gerritbot added a comment.Sep 17 2019, 4:19 PM

Change 537483 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/Translate@master] WIP: use batch access for revisions

https://gerrit.wikimedia.org/r/537483

• WDoranWMF set the point value for this task to 3.Sep 17 2019, 4:30 PM
• WDoranWMF moved this task from Engineering Tasks Ready for Estimation to Ready on the Platform Team Workboards (Architecture Review Workboard) board.
• Pchelolo subscribed.Sep 17 2019, 10:49 PM

The patch above handles usages in production code. There's 3 usages left in maintenance scripts and import action.

Those are quite fixable as well, but I'm inclined to separate those into it's own patch. Also, it would be nice to know if those scripts are still used/useful and whether maintaining pre-1.34 compatibility is useful (are those expected to be used by third-party wikis?)

• Pchelolo claimed this task.Sep 17 2019, 10:55 PM
• Pchelolo moved this task from Ready to Waiting for Review on the Platform Team Workboards (Architecture Review Workboard) board.
Nikerabbit added a comment.Sep 18 2019, 7:22 AM

There's 3 usages left in maintenance scripts and import action.

Could you name the files?

Pginer-WMF edited projects, added Language-Team (Language-2019-July-September); removed Language-Team.Sep 18 2019, 8:08 AM
Nikerabbit moved this task from Backlog to In Review on the Language-Team (Language-2019-July-September) board.Sep 18 2019, 8:11 AM
• Pchelolo added a comment.Sep 18 2019, 2:56 PM
In T228675#5502172, @Nikerabbit wrote:

There's 3 usages left in maintenance scripts and import action.

Could you name the files?

scripts/fuzzy.php, scripts/populateFuzzy.php and MessageWebImporter

Nikerabbit added a comment.Sep 18 2019, 3:22 PM

MessageWebImporter is used by Special:ImportTranslations which is used by translators that prefer to work with offline translation tools. It needs to keep BC per MLEB release policy.

fuzzy.php is used by translation admins in translatewiki.net, often as our only option for cleaning up bad translations. It is a command line script so it is less performance sensitive and may not require batch loading of contents. I'm fine forgoing BC code here if it makes things simpler, but would be nice to have it.

populateFuzzy.php is an old maintenance script to populate revtag table. It can be removed, unless there is a trivial fix to keep it working (even slowly or without BC).

gerritbot added a comment.Sep 18 2019, 7:22 PM

Change 537736 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/Translate@master] Replace Revision::getRevisionText for importer and maintenance scripts

https://gerrit.wikimedia.org/r/537736

gerritbot added a comment.Sep 19 2019, 8:54 PM

Change 538107 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/Translate@master] Don't use rev_text_id to update translation tag for null revision

https://gerrit.wikimedia.org/r/538107

daniel added a comment.EditedSep 23 2019, 11:48 AM

I just realized that old_text and old_flags are also referenced directly in a couple of places. That needs to be fixed as well. Search:
https://codesearch.wmflabs.org/search/?q=[ '">.=]old_(id|flags|text)&i=nope&files=&repos=Extension:Translate

• Pchelolo added a comment.Sep 23 2019, 4:44 PM

All of the above are addressed by the existing patches.

daniel added a comment.Sep 23 2019, 4:51 PM
In T228675#5516794, @Pchelolo wrote:

All of the above are addressed by the existing patches.

Thank you for double checking!

gerritbot added a comment.Sep 23 2019, 6:51 PM

Change 538107 merged by jenkins-bot:
[mediawiki/extensions/Translate@master] Don't use rev_text_id to update translation tag for null revision

https://gerrit.wikimedia.org/r/538107

ReleaseTaggerBot added a project: MW-1.34-notes (1.34.0-wmf.24; 2019-09-24).Sep 23 2019, 7:00 PM
abi_ mentioned this in T233200: Unable to save change on any page using Translate git master: "Call to undefined method WikitextContent::getText()" on MW1.32/1.33.Sep 24 2019, 7:44 AM
Nikerabbit added a comment.Sep 25 2019, 11:25 AM

Here is a profiling visualization. Do note that this was taken with Ib33b4afc14f1098 cherry-picked on top of my optimization with reduces use of Title::makeTitleSafe (T230100). Still it manages to be about 20x slower.

image.png (1×2 px, 1 MB)

Compare with before both patches:

image.png (1×3 px, 613 KB)

gerritbot added a comment.Sep 25 2019, 12:04 PM

Change 524593 abandoned by Daniel Kinzler:
Remove direct access to text table

Reason:
superseded

https://gerrit.wikimedia.org/r/524593

daniel added a comment.Sep 25 2019, 12:09 PM
In T228675#5522097, @Nikerabbit wrote:

Here is a profiling visualization. Do note that this was taken with Ib33b4afc14f1098 cherry-picked on top of my optimization with reduces use of Title::makeTitleSafe (T230100). Still it manages to be about 20x slower.

The added layers of abstraction will make things a bit slower, but I'm surprised by the slowdown. I would expect this to be I/O-bound. Does the profiling reflect CPU time or wall-clock time?

My guess is that there must be something wrong with the DB queries - either we are running way to many, or one of them got really inefficient. The first thing I'd try is to count the number of queries. It's going to be larger, but should not be x20 larger. Maybe x2.

What's the test setup? What do I have to do to reproduce this?

daniel added a comment.Sep 25 2019, 12:12 PM

...looking at the picture, I see a lot of calls to Title::newFromId, and RevisionStore::loadSlotRecords. Both should not be called much when RevisionStore::newRevisionsFromBatch(). It's designed to avoid exactly this. So either there is more code that needs to be changed to use newRevisionsFromBatch(), or there is a bug that prevents the optimization from being effective.

Nikerabbit added a comment.Sep 25 2019, 12:42 PM
In T228675#5522153, @daniel wrote:
In T228675#5522097, @Nikerabbit wrote:

Here is a profiling visualization. Do note that this was taken with Ib33b4afc14f1098 cherry-picked on top of my optimization with reduces use of Title::makeTitleSafe (T230100). Still it manages to be about 20x slower.

The added layers of abstraction will make things a bit slower, but I'm surprised by the slowdown. I would expect this to be I/O-bound. Does the profiling reflect CPU time or wall-clock time?

I don't know honestly. Default xdebug output. It is definitely not IO bound with profiling enabled, and judging by the 20x increase, I don't think that is explained by 20x more IO.

What's the test setup? What do I have to do to reproduce this?

FWIW we chatted about this on IRC briefly. I was measuring our production (twn that is) mediawiki core export that we are optimizing anyway in T230100.

In T228675#5522156, @daniel wrote:

...looking at the picture, I see a lot of calls to Title::newFromId, and RevisionStore::loadSlotRecords. Both should not be called much when RevisionStore::newRevisionsFromBatch(). It's designed to avoid exactly this. So either there is more code that needs to be changed to use newRevisionsFromBatch(), or there is a bug that prevents the optimization from being effective.

Does this depend on any recent core changes? I was testing with about week old core code.

• Pchelolo added a comment.EditedSep 25 2019, 4:07 PM

Does this depend on any recent core changes? I was testing with about week old core code.

Only newRevisionsFromBatch introduced in T228988 and a further optimization in T233173, so anything after 1.24 should be good.

I've uploaded a new version of the patch that attempts to mitigate the performance degradation. The problem was that we were still using the anti-pattern of creating a batch of revisions looping through db rows, just indirectly via ThinMessage

gerritbot added a comment.Sep 26 2019, 12:26 PM

Change 539324 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] RevisionSTore: Introduce getSlotRowsForBatch

https://gerrit.wikimedia.org/r/539324

daniel added a subtask: T228988: Create batch access interface for page content.Sep 27 2019, 11:37 AM
daniel added a subtask: T230834: Create batch access interface for BlobStore.
daniel mentioned this in T234034: Create low level batch access interface for page content.Sep 27 2019, 11:45 AM
daniel moved this task from Waiting for Review to Blocked on the Platform Team Workboards (Architecture Review Workboard) board.Sep 27 2019, 2:45 PM
daniel raised the priority of this task from Medium to High.Sep 30 2019, 2:38 PM
daniel edited projects, added Platform Team Workboards (Clinic Duty Team); removed Platform Team Workboards (Architecture Review Workboard).

Bumping to high, since this blocks 1.34

daniel moved this task from Inbox to Blocked Externally on the Platform Team Workboards (Clinic Duty Team) board.Sep 30 2019, 2:38 PM
gerritbot added a comment.Sep 30 2019, 5:46 PM

Change 539324 merged by jenkins-bot:
[mediawiki/core@master] RevisionStore: Introduce getContentBlobsForBatch

https://gerrit.wikimedia.org/r/539324

daniel closed subtask T234034: Create low level batch access interface for page content as Resolved.Sep 30 2019, 5:47 PM
gerritbot added a comment.Oct 7 2019, 9:08 AM

Change 537483 merged by jenkins-bot:
[mediawiki/extensions/Translate@master] Use batch access for revisions instead of Revision::getRevisionText

https://gerrit.wikimedia.org/r/537483

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.1; 2019-10-08).Oct 7 2019, 10:00 AM
gerritbot added a comment.Oct 7 2019, 7:05 PM

Change 537736 merged by jenkins-bot:
[mediawiki/extensions/Translate@master] Replace Revision::getRevisionText for importer and maintenance scripts

https://gerrit.wikimedia.org/r/537736

• Pchelolo closed this task as Resolved.Oct 7 2019, 7:07 PM
• Pchelolo moved this task from Blocked Externally to Done on the Platform Team Workboards (Clinic Duty Team) board.

All access has been either removed or gated. Resolving.

Maintenance_bot removed a project: Patch-For-Review.Oct 7 2019, 7:11 PM
Nikerabbit moved this task from In Review to Done on the Language-Team (Language-2019-July-September) board.Oct 8 2019, 6:56 AM
Nikerabbit mentioned this in T235027: Translate does not update content page when saving units.Oct 16 2019, 6:43 AM
gerritbot added a comment.Oct 30 2019, 10:04 PM

Change 547327 had a related patch set uploaded (by Jforrester; owner: Ppchelko):
[mediawiki/extensions/Translate@REL1_34] Replace Revision::getRevisionText for importer and maintenance scripts

https://gerrit.wikimedia.org/r/547327

gerritbot added a project: Patch-For-Review.Oct 30 2019, 10:04 PM

Change 547327 abandoned by Jforrester:
Replace Revision::getRevisionText for importer and maintenance scripts

https://gerrit.wikimedia.org/r/547327

Maintenance_bot removed a project: Patch-For-Review.Oct 30 2019, 10:11 PM
gerritbot added a comment.Dec 21 2019, 4:48 PM

Change 560081 had a related patch set uploaded (by Lens0021; owner: Ppchelko):
[mediawiki/extensions/Translate@REL1_34] Use batch access for revisions instead of Revision::getRevisionText

https://gerrit.wikimedia.org/r/560081

gerritbot added a project: Patch-For-Review.Dec 21 2019, 4:48 PM
gerritbot added a comment.Mar 17 2020, 9:27 AM

Change 560081 merged by jenkins-bot:
[mediawiki/extensions/Translate@REL1_34] Use batch access for revisions instead of Revision::getRevisionText

https://gerrit.wikimedia.org/r/560081

Maintenance_bot removed a project: Patch-For-Review.Mar 17 2020, 10:10 AM
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:40 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits