Page MenuHomePhabricator

Vector performance budget's should consider modules added by hooks
Closed, ResolvedPublic3 Estimated Story Points

Description

We recently had a performance regression relating to Phonos and OOUI being added to all page loads (T345414). Despite having a performance budget inside Vector and Minerva, the modules included in the performance budget have to be manually captured inside bundlesize config

Assuming hooks are executed, we should instead generate the list of modules that are loaded on page load dynamically and instead use a single number as the budget.

We hope this will reduce the amount of time the web team react to performance Grafana alerts (and identify the source of regression) and instead allow us to proactively detect them and discuss implications with the appropriate teams.

Developer notes

You should be able to identify all modules on a page by calling OutputPage::getModuleStyles (CSS) or OutputPage::getModules (JS)

QA steps

The following patches should be voted by Jenkins as -1:

It would be useful to check the error message on Jenkins and confirm it is clear what action is needed by someone who doesn't understand what we've done here.

Once this has been verified please tag as Verified and move to sign off as it does not need further review.

QA Results

ACStatusDetails
1โœ…T346813#9309592
2โœ…T346813#9309592

Event Timeline

ovasileva set the point value for this task to 3.Oct 12 2023, 5:26 PM

@Jdlrobson to sync with @Mabualruz further on this re: integration testing of skins - patch and/or followup discussion

Change 967918 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/skins/Vector@master] Performance budget's should consider modules added by hooks

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

Mabualruz updated Other Assignee, added: Mabualruz.

Change 970826 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/core@master] Performance budget's should consider modules added by hooks

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

Change 970827 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/skins/MinervaNeue@master] Performance budget's should consider modules added by hooks

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

Change 970828 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/skins/Vector@master] Performance budget's should consider modules added by hooks

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

Change 970829 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/skins/MinervaNeue@master] Only Minerva: Performance budget's should consider modules added by hooks

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

Change 970861 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] PerformanceBudgetTest: Use existing title

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

Change 967918 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Performance budget's should consider modules added by hooks

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

Change 970861 merged by jenkins-bot:

[mediawiki/skins/Vector@master] PerformanceBudgetTest: Use existing title

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

Jdlrobson updated Other Assignee, added: Edtadros; removed: Mabualruz.
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated Other Assignee, added: Mabualruz; removed: Edtadros.
Jdlrobson renamed this task from Minerva and Vector performance budget's should consider modules added by hooks to Vector performance budget's should consider modules added by hooks.Nov 2 2023, 4:53 PM

Change 971508 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Restore test and increase budget with debug information

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

This was disabled by @Tgr in T350338. My guess would be that Vector runs its integration tests with less extensions installed than CentralAuth for performance reasons (@Tgr would that make sense to you?) [I'm debugging this as we speak]

If that's the case than this test should probably check for the worse case and we should increase the budget, however this would mean the performance budget would not catch changes in extensions loaded in the core set of tests.

Tldr: FlaggedRevisions has an existing performance bug. We need to find a way to disable it for purpose of tests

Vector patch [ 14650 bytes]

  • codex-search-styles
  • skins.vector.styles
  • skins.vector.user.styles
  • skins.vector.icons
  • site.styles
  • noscript
  • user.styles

Related Articles (24802 bytes)

  • codex-search-styles
  • skins.vector.styles
  • skins.vector.user.styles
  • skins.vector.icons
  • ext.flaggedRevs.basic
  • codex-styles [added by test]
  • ext.relatedArticles.styles
  • ext.visualEditor.desktopArticleTarget.noscript
  • site.styles
  • noscript
  • user.styles

So the additional bloat is coming from ext.flaggedRevs.basic, ext.relatedArticles.styles and ext.visualEditor.desktopArticleTarget.noscript

This seems to suggest FlaggedRevisions, RelatedArticles and VisualEditor extensions are being included in the Vector CI build (how do we change that?).

CentralAuth patch:
CentralAuth test is consistent with RelatedArricles but flags that FlaggesRevisions also adds codex-styles to the page.

I suggest 3 actions:

  • for now budget includes FlaggedRevisions (or we block on FlaggedRevisions follow up)
  • Follow up to get Vector, RelatedArticles included in main CI build for Vector
  • Follow up task to remove ext.flaggedRevs.basic from the test run and budget calculation
Jdlrobson added a subscriber: Edtadros.

Follow up. I've managed to track this down to T350514 and have added a workaround to our code and restored the test. https://gerrit.wikimedia.org/r/c/971508
I included some debug information for easier debugging in future.

Mabualruz updated Other Assignee, removed: Mabualruz.

Change 971508 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Restore test and increase budget with debug information

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

Test Result - Gerrit

Status: โœ… PASS
Environment: Gerrit

Test Artifact(s):

QA Steps

โœ… AC1: The following patches should be voted by Jenkins as -1:

Both patches are failing CI.

Screenshot 2023-11-06 at 9.41.13 AM.png (122ร—2 px, 61 KB)

โœ… AC2: It would be useful to check the error message on Jenkins and confirm it is clear what action is needed by someone who doesn't understand what we've done here.

Screenshot 2023-11-06 at 9.40.45 AM.png (922ร—2 px, 287 KB)

(Minor: there is a full stop missing)

Jdlrobson added a project: Verified.
Jdlrobson updated the task description. (Show Details)

It wasn't intended, but I think it's giving you good information- that the module is probably too large to load on page load. I'll comment on that ticket.

Change 970829 abandoned by Mabualruz:

[mediawiki/skins/MinervaNeue@master] Performance budget's should consider modules added by hooks

Reason:

Seems we will focus on the vector test to streamline and stabilise the tests then we will have it added to more skins

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