Page MenuHomePhabricator

Expensive viewport size access in Reference Drawers
Closed, ResolvedPublic

Description

The Event Timing API origin trial revealed that the click handler for reference links can be quite slow (occurrences taking > 50ms). The reason is most likely that the reference drawer contains expensive viewport size access, which cause a styles recalc and layout.

The offending code: https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/917d7bd07e1b19dbb2e22aca4199bb9c8ff9bca9/src/mobile.startup/references/ReferencesDrawer.js#L80-L87

	postRender: function () {
		var windowHeight = util.getWindow().height();

		Drawer.prototype.postRender.apply( this );
		this.$el.find( '.references-drawer__header' ).append( [
			new Icon( {
				isSmall: true,
				name: 'citation-invert',
				additionalClassNames: 'references-drawer__title',
				hasText: true,
				label: mw.msg( 'mobile-frontend-references-citation' )
			} ).$el,
			icons.cancel( 'gray' ).$el
		] );
		// For lazy loading references - if no text append a spinner
		if ( !this.options.text ) {
			this.$el.append( icons.spinner().$el );
		}

		// make sure the drawer doesn't take up more than 50% of the viewport height
		if ( windowHeight / 2 < 400 ) {
			this.$el.css( 'max-height', windowHeight / 2 );
		}

		this.on( 'show', this.onShow.bind( this ) );
		this.on( 'hide', this.onHide.bind( this ) );
	},

This doesn't seem like something that's critical, and I'm pretty sure something equivalent can be achieved with a CSS media query. This code should be removed and a CSS-only solution/compromise found, in order to avoid responsiveness issues when users open a reference. To be clear, this can be slow all the time, not just when the conditional kicks in. It's getting the viewport size that's costly.

Profile

Dev notes

To review analysis on this, see https://phabricator.wikimedia.org/T226025#5342756

Since the drawer uses fixed positioning, I think we can remove the forced synchronous layout by removing the JS that checks the window's height and simply using the following CSS. Something like:

.drawer {
  max-height: 50%
}

As found in https://phabricator.wikimedia.org/T209129#4885940 and discussed more in this ticket, there is also a memory leak associated with the drawers that can cause extraneous JS to execute. That might be too much to tackle in this ticket, however, so the AC is scoped to fixing the forced synchronous layout.

Acceptance Criteria

  • Replace the JS that queries the window's height with a CSS-only solution so that the forced synchronous layout is removed
  • Look more into the memory leak and make a new ticket for fixing that with notes from your review.

Event Timeline

Gilles created this task.Jun 18 2019, 2:19 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 18 2019, 2:19 PM

@nray could you analyze this one?

nray claimed this task.Jun 18 2019, 3:43 PM
ovasileva triaged this task as Normal priority.Jun 24 2019, 6:40 PM
Gilles moved this task from Inbox to Radar on the Performance-Team board.Jun 24 2019, 8:01 PM
Gilles edited projects, added Performance-Team (Radar); removed Performance-Team.
nray added a comment.Jun 24 2019, 11:32 PM

@Gilles Can you please clarify that the problematic click handler this task is referring to is when you click on a reference link like the one pictured below and then black reference drawer pops up?

I think that's what you meant, but wanted to make sure because "Lazy Loaded references" is actually a beta feature on the mobile site that is off by default

And I think the problematic code you've referenced in the description can run in both cases (with the beta feature turned on or off) as long as the reference link is clicked. Therefore I'm assuming this problem is not unique to the beta feature being enabled.

Is that correct?

Yes, it's the overlay at the bottom of the screen that happens when clicking on a reference link. It's affecting everyone, not just beta users.

Niedzielski added a subscriber: Niedzielski.

So far as I know, reference previews are strictly in the Popups extension and nothing to do with MobileFrontend, so I've removed that tag.

nray renamed this task from Expensive viewport size access in lazy loaded references to Expensive viewport size access in ReferenceDrawers.Jun 25 2019, 6:39 PM
nray updated the task description. (Show Details)

@Niedzielski Thank you for removing that tag

@Gilles Thank you for clarifying. I've edited the title and description to reflect that

nray renamed this task from Expensive viewport size access in ReferenceDrawers to Expensive viewport size access in Reference Drawers.Jun 25 2019, 6:41 PM
nray updated the task description. (Show Details)
nray updated the task description. (Show Details)Jun 25 2019, 6:55 PM

@nray could we just use vh units here for max-height instead?

nray added a comment.EditedJul 17 2019, 7:00 PM

There are multiple things to the Drawers that contribute to the performance problems. Here is the profile of it on the Obama page when the first reference is clicked using 6x throttle to simulate a mobile device:


https://github.com/nicholasray/references-drawer-analysis/blob/master/1st-click.json

Memory Leak

First, there is a memory leak in the Drawer and ReferenceDrawer's postRender methods. Within those methods, event listeners are setup. The leak occurs because with each click of a reference, the postRender methods are called in the init script, and after the gateway promise resolves. The postRender methods are also called when the ReferenceDrawer first initializes. Minerva uses the same reference drawer instance for each reference so the event listeners are added and never removed with each click of a reference.

Bottom line: Many extraneous setTimeout calls can be executed because of this memory leak.

Forced Synchronous Layout

When the reference drawer first displays, the Drawer's util.docReady callback is executed before the gateway resolves. The docReady invalidates the document by appending the drawer to the DOM. When the gateway resolves, it calls drawer.render causing the ReferencesDrawer postRender() method to call util.getWindow().height(); which triggers a forced synchronous layout.

	postRender: function () {
		var windowHeight = util.getWindow().height();

		Drawer.prototype.postRender.apply( this );
		this.$el.find( '.references-drawer__header' ).append( [
			new Icon( {
				isSmall: true,
				name: 'citation-invert',
				additionalClassNames: 'references-drawer__title',
				hasText: true,
				label: mw.msg( 'mobile-frontend-references-citation' )
			} ).$el,
			icons.cancel( 'gray' ).$el
		] );
		// For lazy loading references - if no text append a spinner
		if ( !this.options.text ) {
			this.$el.append( icons.spinner().$el );
		}

		// make sure the drawer doesn't take up more than 50% of the viewport height
		if ( windowHeight / 2 < 400 ) {
			this.$el.css( 'max-height', windowHeight / 2 );
		}

		this.on( 'show', this.onShow.bind( this ) );
		this.on( 'hide', this.onHide.bind( this ) );
	},

We should be able to replace the drawer max-height logic with CSS alone (by setting a max-height on the .drawer class:

.drawer  {
   max-height: 50%;
}

Bottom line: By removing the JS code that queries the window height and replacing it with CSS, it should be enough to get rid of the FSL!.

Large Recalc Style (unrelated to the forced synchronous layout)

This is mostly likely caused by having a large DOM and will be difficult to change.

nray added a comment.EditedJul 17 2019, 7:01 PM

@nray could we just use vh units here for max-height instead?

@Jdlrobson Since the drawer uses fixed positioning, I think we could actually just use max-height here alone and it would get rid of the FSL. See my analysis above for more info!

nray updated the task description. (Show Details)Jul 17 2019, 7:25 PM

Sounds like a plan! It would also simplify the Drawer code making it easier to refactor.

nray updated the task description. (Show Details)Jul 17 2019, 7:33 PM
nray updated the task description. (Show Details)Jul 17 2019, 7:37 PM