WordPress.org

Plugin Directory

jetpack

Opened 4 years ago

Closed 4 years ago

#2031 closed enhancement (duplicate)

devicepx: improve layout thrashing potential in Jetpack

Reported by: tollmanz Owned by: tmoorewp
Priority: normal Severity: normal
Plugin: jetpack Keywords: devicepx retina hdpi
Cc: blobaugh, andy, mbijon, tollmanz, TJNowell, jeremy+wp@…

Description

In debugging some jankiness in a theme, I came across a concerning issue with JetPack.

Problem

The devicepx is increasing the probability of layout thrashing by running an event every single second that causes a layout event in Webkit browsers.

Reproduce or Observe the Issue

  1. Load http://twentythirteendemo.wordpress.com/ in Google Chrome
  2. Open Web Inspector
  3. Click Timeline
  4. Click Frames in the upper left hand pane
  5. At the bottom left of the browser, click the black dot (hovering over it will reveal the “Record” label)
  6. Allow it to run for about 5 seconds.
  7. Click the same button again (which is now red)
  8. In the upper right hand frame, make sure that the scrubber thingy (not sure what to call it) is extended to show the whole 5 seconds that you recorded.
  9. In the “Records” pane, scroll down.
  10. Notice that a “Layout” event happens every second
  11. Bonus: note that there are approximately 11 +/-3 timed events happening ever second. This is not good, but not all of these are JP related. Only one of those events is related to this bug report.

For the tl;dr crowd, this image shows what these steps will reveal:

http://f.cl.ly/items/1N0c1G3Q3W2X3D1X2q3X/Screen%20Shot%202013-11-07%20at%2010.08.54%20AM.png

A couple important things to note:

  • My reproduce steps use WordPress.com, which has a host of other scripts running. I use it just to make it easy to see this right away.
  • The issue is also available (and even a little clearer) with a .org site running Twenty Thirteen and Jetpack (should occur without any modules installed)
  • The “devicepx” script is hosted on WordPress.com and not distributed with the package.

Why This Matters

Unchecked layout events are a known performance issue, especially when they are triggered outside of an animation frame. Fortunately, the paint event that this issue causes is fast (< 1ms), so its immediate effects are fairly well masked. When you pair this with other layout events (e.g., a slider animating an image change), jank will ensue.

I’ve traced this layout event to http://s0.wp.com/wp-content/js/devicepx-jetpack.js. On .com, this is mushed into another script and on .org, it is served directly. Because there is no source file, only a minified version, available, it’s hard for me to pinpoint line numbers to explain this, so I’ll point out actual code. You will need to unminify the source to follow along (or view my gist)

The problem is in the “init” function:

init: function () {
    var t = this;
    try {
        t.zoomImages();
        t.timer = setInterval(function () {
            t.zoomImages();
        }, t.interval);
    }
    catch (e) {
    }
}

You’ll see that an interval is set to run zoomImages() every second (t.internal === 1000). zoomImages() ends up running detectFunction(). Whenever detectFunction() evaluates to webkit, this will cause the webkit function to fire. Note that this issue is thus only present in Chrome and Safari (I think). This function looks like:

var webkit = function () {
	var important = function (str) {
		return str.replace(/;/g, " !important;");
	};
	var div = document.createElement('div');
	div.innerHTML = "1<br>2<br>3<br>4<br>5<br>6<br>7<br>8<br>9<br>0";
	div.setAttribute('style', important('font: 100px/1em sans-serif; -webkit-text-size-adjust: none; text-size-adjust: none; height: auto; width: 1em; padding: 0; overflow: visible;'));
	var container = document.createElement('div');
	container.setAttribute('style', important('width:0; height:0; overflow:hidden; visibility:hidden; position: absolute;'));
	container.appendChild(div);
	document.body.appendChild(container);
	var zoom = 1000 / div.clientHeight;
	zoom = Math.round(zoom * 100) / 100;
	document.body.removeChild(container);
	return{zoom: zoom, devicePxPerCssPx: zoom * devicePixelRatio()};
};

The reason that the layout is being triggered is due to innerHTML and setAttribute methods being executed. This is a write that will cause the browser to invalidate the layout and redraw. Unfortunately, the layout scope for this event is the whole document, which means the browser is being asked to do a lot. What’s worse is that this is happening every single second without fail.

The problem is that this can easily kill performance when other CSS or JS causing layout changes as this script greatly increases the chances of layout trashing. As a plugin/theme developer, I have to just accept this performance issue and cannot work around it.

I was able to find out that the detectZoom code is actually an open source project (note that I could not find any attribution for this anywhere in JP or in the devicepx script and that is likely a license violation). I do understand that the writes that are causing the layout redraws are from the library; however, the event that is firing is from JP/Automattic code. From looking at the code, I think that it is looking to see if images need resizing (perhaps for retina displays) every second. The only reason I can imagine that this polling is necessary is for responding to DOM changes where images are introduced (e.g., Infinite Scroll); however, there are way better ways to do this that do not incur this massive performance issue (e.g., with Inifnite Scroll, bind to the post-load event). I fully accept that I may be way off on that assumption and would love some insight into what this is doing.

To be clear, the culprit is not necessarily zoomImages(), but running zoomImages() every second. I think it’s sensible to run this on page load.

What to do

In bold, at the top of the detect zoom repo’s readme.md, the author states:

Detect-zoom is currently unusable for desktop

In the past few months both Mozilla and Google made some changes to their browsers that make it almost impossible to do what detect-zoom is here to do

It seems like this function is not even working any more :(

I propose the following solutions in order of most to least preferred:

  1. Remove the detect zoom script and refactor to perform the functionality another way (if it is even needed). I cannot advise on this as it is not clear to me what this is supposed to do.
  2. Reassess the necessity of this interval action and remove it. My hunch is that there is an actual event that this can be bound to which would be a more sensible way to trigger the action.
  3. Move from using setInterval to requestAnimationFrame (and a corresponding polyfill). This will go a long way towards improving the layout thrashing issues.

Change History (12)

comment:1 @blobaugh4 years ago

  • Cc blobaugh added

comment:2 @andy4 years ago

I wrote devicepx.js. It loads larger images to match the device resolution on high-res displays and zoomed-in browsers.

http://wordpress.com/wp-content/js/devicepx.js?minify=false

The interval is used to detect new images and changed zoom levels.

Do you know another way to accomplish this?

comment:3 @andy4 years ago

  • Cc andy added

comment:4 @mbijon4 years ago

  • Cc mbijon added

comment:5 @tollmanz4 years ago

  • Cc tollmanz added

@andy Thanks a lot for the context! Is there something I can do to test this? In other words, is there something I should expect to see with this enable versus when it is disabled?

I've not encountered this issue before, so I am not sure about a fix. What was the original problem that was being solved? Is it something with the tiled galleries? Why is it needed? I am just trying to get more context so I can help think of a solution.

comment:6 @tollmanz4 years ago

Also, I just realized that the script is licensed under WTFPL, so ignore the license quip; although, it still might be a good idea to add attribution.

comment:7 @westonruter4 years ago

The only reason I can imagine that this polling is necessary is for responding to DOM changes where images are introduced (e.g., Infinite Scroll)

You'll also need to trigger a re-zoom if you drag a browser window from a lo-DPI screen (e.g. Thunderbolt Display) over to the screen on a connected Retina MacBook Pro.

comment:8 @TJNowell4 years ago

  • Cc TJNowell added

comment:9 @matt4 years ago

Thank you for the super-thorough ticket.

comment:10 @jeherve4 years ago

  • Cc jeremy+wp@… added
  • Keywords devicepx retina hdpi added
  • Summary changed from Improve layout thrashing potential in Jetpack to devicepx: improve layout thrashing potential in Jetpack

comment:11 @KZeni4 years ago

I stumbled into devicepx purely for it causing noticeable stuttering when scrolling (due to the setInterval initiating a paint event on a visual-heavy page, like has been stated). The hitching was even noticeable on an above-average desktop computer (Mac Pro). I had to find a way to dequeue the script in order for it to not cause issues on the site (http://wordpress.org/support/topic/plugin-jetpack-by-wordpresscom-unnecessary-java-script-call).

A Thought

Couldn't this functionality be called out as a HiDPI module that can be easily enabled/disabled? It seems odd the other features Jetpack has can be activated/deactivated per the plugin's settings, but this one needs to be manually intercepted & dequeued in order to prevent this performance hit. Why not promote this (otherwise unrelated to / independent of the other modules) feature by making it a module?

I'd doubly enjoy this feature being broken out into a module as I've actually opted to use WP Retina 2x (http://wordpress.org/plugins/wp-retina-2x/) to manage & display HiDPI images so this functionality is both redundant of a more advanced alternative and actively hinders performance unless intercepted & disabled. If it can't really be optimized... why not make it surfaced as an option like the other modules?

Thought I'd mention the idea.

comment:12 @jeherve4 years ago

  • Resolution set to duplicate
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.