RFC: Separate the load phase of AMP into multiple chunks. #5536

cramforce · 2016-10-12T00:48:04Z

The general idea is that AMP can be instructed to not block the UI thread during load phase for long if the document itself is not visible.

The change may be quite impactful. It does split up work into neat chunks (about 10-20ms on desktop, longer on mobile) instead of one 400ms+ block. I validated that the number of style recalc doesn't change when the experiment is off.

Current doesn't change amp-shadow. That will come in a follow up CL, but might not make as much sense.
With the experiment not active the only thing this should change is that the initialization now goes through a bunch of micro tasks. This should behave as before in browsers with native promises.

dvoytenko · 2016-10-12T16:45:51Z

src/chunk.js

+  throw errors[0];
+}
+
+class Chunk {


Would be nice to have a more aggregate name. Chunker?

Called it Chunks

dvoytenko · 2016-10-12T17:01:37Z

src/chunk.js

+  /**
+   * Run fn as a "chunk". It'll run in a micro task when the doc is visible
+   * and otherwise run it after having yielded to the event queue once.
+   * @param {function()} fn


dvoytenko · 2016-10-12T17:01:45Z

src/chunk.js

+  /**
+   * Run a task.
+   * Schedule the next round if there are more tasks.
+   * @return {boolean} Whether anything was executed.


dvoytenko · 2016-10-12T17:03:15Z

src/chunk.js

+    // Ask the viewer or try to infer whether we are visible.
+    return this.viewer_
+        ? this.viewer_.isVisible()
+        : !(/visibilityState=hidden/.test(this.win_.location.hash));


This should also include visibilityState=prerender.

This is icky.

visibilityState=prerender is the only one!

I added support for document.hidden

dvoytenko · 2016-10-12T17:05:27Z

src/chunk.js

+   */
+  run_(fn) {
+    this.tasks_.push(fn);
+    this.schedule_();


Should "schedule next" be done from the starting point of run_ here, or from the point when previous task was executed in execute_?

We always need to schedule running tasks when we add one (There might not be one) and I want microtasks to be scheduled immediately instead of nesting them. In the end this schedules too many, but that should be fine.

dvoytenko · 2016-10-12T17:06:50Z

src/amp.js

-      // new ticks in place to batch the ticks properly.
-      perf.flush();
-    }
+          maybeValidate(self);


Something like maybeValidate should definitely just be out of a critical path in its own microtask.

Its a noop in production, though and otherwise doesn't have an immediate side effect.

dvoytenko · 2016-10-12T17:09:58Z

src/runtime.js

@@ -244,9 +245,14 @@ function adoptShared(global, opts, callback) {
   */
  function installExtension(fnOrStruct) {
    if (typeof fnOrStruct == 'function') {
-      fnOrStruct(global.AMP);
+      const fn = fnOrStruct;
+      chunk(global.document, () => fn(global.AMP));


Important! This will immediately break shadow multi-doc mode since this is run before an ampdoc is available.

Went back and forth a lot here, and decided to just add an option to turn it off in this case.

dvoytenko · 2016-10-12T17:11:32Z

src/runtime.js

-      installExtension(fnOrStruct);
-    });
+    const register = function() {
+      waitForBody(global.document, () => {


It might be better to move waitForBody outside. It's a very rare thing that body is not available by this time.

Isn't that an argument to do it on the inside like it is?

dvoytenko · 2016-10-12T17:13:24Z

testing/fake-dom.js

+   * entry.
+   * @param {string} href
+   */
+  setForTesting(href) {


Everything here is for testing :) Let's call it something like setQuiet. Generally, the intention here is if you need an initially set location, you declare it in describe. That might not be applicable in all cases, so I'm fine with the change.

Called it resetHref

jridgewell · 2016-10-12T19:26:55Z

src/chunk.js

+  while (true) {
+    try {
+      if (!service.execute_()) {
+        if (errors.length) {


Why not check for this after the try catch block?

jridgewell · 2016-10-12T19:27:20Z

src/chunk.js

+      errors.push(e);
+    }
+  }
+  throw errors[0];


Is the intention to run all regardless of errors but only throw the first?

This was convenient for testing purposes so far. Will document.

jridgewell · 2016-10-12T19:33:36Z

src/chunk.js

+      return;
+    }
+    // The message doesn't actually matter.
+    this.win_.postMessage/*OK*/('amp-macro-task', '*');


Should we post to only this origin?

It really doesn't matter. We also don't care about the actual message on the receiving side. Tasks are executed on every message received by the current window.

jridgewell · 2016-10-12T19:34:10Z

src/chunk.js

+    // Ask the viewer or try to infer whether we are visible.
+    return this.viewer_
+        ? this.viewer_.isVisible()
+        : !(/visibilityState=hidden/.test(this.win_.location.hash));


This is icky.

jridgewell · 2016-10-13T20:45:56Z

src/chunk.js

+      return false;
+    }
+    // Viewers send a URL param if we are not visible.
+    return !(/visibilityState=prerender/.test(this.win_.location.hash));


I think the viewer sends visibilityState=hidden.

Indeed, changed back.

cramforce · 2016-10-17T20:19:55Z

Friendly ping

dvoytenko · 2016-10-17T20:30:02Z

src/chunk.js

+      return false;
+    }
+    // Viewers send a URL param if we are not visible.
+    return !(/visibilityState=hidden/.test(this.win_.location.hash));


Not sure if I already commented on this, but there are many other values that might be of important here: prerender, paused, inactive. Probably the most important is prerender since this is the state most of docs start in. Are you counting on the fact that it will be rare that viewer_ is not available yet to respond?

Rare is not really the right word. This happens for the first 1-2 calls to chunk on every page load. The viewer only sends this very string and nothing else, so it doesn't seem important to care about any other value. After that viewer.isVisible is what is being used.

Hmm. (/cc @jridgewell). Viewer should be sending visibilityState=prerender in these cases. I know that we were migrating viewer to this system some time ago, not sure what's the status is. It might be safer to support both hidden and prerender here for now to avoid migration pains.

We never completed it. I think testing for either prerender (since that's what they should be using) or hidden is the right choice. But we definitely need to test for hidden.

The viewer is definitely sending hidden. Added support for both.

jridgewell

LGTM.

dvoytenko · 2016-10-17T22:26:47Z

LGTM

dvoytenko · 2016-10-17T22:27:24Z

@jridgewell Probably a good time to follow up with the viewer team?

jridgewell · 2016-10-17T23:25:47Z

b/32222292.

The general idea is that AMP can be instructed to not block the UI thread during load phase for long if the document itself is not visible. The change may be quite impactful. It does split up work into neat chunks (about 10-20ms on desktop, longer on mobile) instead of one 400ms+ block. I validated that the number of style recalc doesn't change when the experiment is off. - Current doesn't change `amp-shadow`. That will come in a follow up CL, but might not make as much sense. - With the experiment not active the only thing this should change is that the initialization now goes through a bunch of micro tasks. This should behave as before in browsers with native promises.

The style installer doesn't work for unknown reasons at the stage of the iframe's lifecycle. Just adding the style works fine.

…mp_reddit_extension * 'master' of https://github.com/ampproject/amphtml: (63 commits) Position:relative on body (ampproject#5665) Fix for ampproject#5663 (ampproject#5664) Nokta Ad Server is added as an ad type (ampproject#5550) RFC: Separate the load phase of AMP into multiple chunks. (ampproject#5536) Add OWNERS files for A4A. (ampproject#5651) Update DEVELOPING.md Call original event add/remove via interceptor (ampproject#5650) Fix wording on confusing steps to protect against CSRF. (ampproject#5646) Install runtime CSS for all amp tests (ampproject#5642) Implement outgoing link URL replacements. (ampproject#5628) Wait for window to load before installing ServiceWorker. (ampproject#5638) iOS wrapper viewport implementation (ampproject#5629) Do not use AMP Version as RTV Versions (ampproject#5474) Purch Ad Support for Amp-Ads (ampproject#5464) A11Y fix for sticky ad close button. (ampproject#5640) Propagate ARIA attributes to real-elements (ampproject#5590) Ibillboard integration (ampproject#5392) Add some keywords to the NPM description of the validator. (ampproject#5633) PWA: messaging and broadcast (ampproject#5588) Carousel swipe not working well on Android Firefox (ampproject#5626) ...

…#5536) The general idea is that AMP can be instructed to not block the UI thread during load phase for long if the document itself is not visible. The change may be quite impactful. It does split up work into neat chunks (about 10-20ms on desktop, longer on mobile) instead of one 400ms+ block. I validated that the number of style recalc doesn't change when the experiment is off. - Current doesn't change `amp-shadow`. That will come in a follow up CL, but might not make as much sense. - With the experiment not active the only thing this should change is that the initialization now goes through a bunch of micro tasks. This should behave as before in browsers with native promises.

cramforce added the NEEDS REVIEW label Oct 12, 2016

cramforce assigned jridgewell and dvoytenko Oct 12, 2016

cramforce force-pushed the chunked-amp branch from 22ef6b0 to 62ea424 Compare October 12, 2016 01:04

dvoytenko suggested changes Oct 12, 2016

View reviewed changes

jridgewell reviewed Oct 12, 2016

View reviewed changes

cramforce force-pushed the chunked-amp branch from f899c56 to e657a5e Compare October 13, 2016 19:02

jridgewell reviewed Oct 13, 2016

View reviewed changes

cramforce mentioned this pull request Oct 13, 2016

Chunked AMP startup #5599

Closed

3 tasks

cramforce force-pushed the chunked-amp branch 2 times, most recently from c4f9c43 to 01f82b2 Compare October 17, 2016 02:01

cramforce force-pushed the chunked-amp branch from 01f82b2 to eab9b3a Compare October 17, 2016 20:23

dvoytenko suggested changes Oct 17, 2016

View reviewed changes

jridgewell approved these changes Oct 17, 2016

View reviewed changes

erwinmombay mentioned this pull request Oct 17, 2016

Meeting Notes 2016-10-17 #5645

Closed

cramforce force-pushed the chunked-amp branch from 9a7421e to 61fdbee Compare October 17, 2016 22:15

dvoytenko approved these changes Oct 17, 2016

View reviewed changes

cramforce added 9 commits October 17, 2016 18:04

Fix runtime tests

eb49167

Add manual test

38d0842

Fix lint.

eb273d9

Address feedback

ae0e104

Switch to value sent by viewer.

58b74dd

Fix test merged from upstream.

85bfaed

Fix test after merge

8413157

Suport both hidden and prerender values for visibility state.

be96992

Fix lint

5f049b8

cramforce force-pushed the chunked-amp branch from 61fdbee to 5f049b8 Compare October 18, 2016 01:04

Fix a never resolving promise.

a8bf6a9

The style installer doesn't work for unknown reasons at the stage of the iframe's lifecycle. Just adding the style works fine.

cramforce removed the NEEDS REVIEW label Oct 18, 2016

cramforce merged commit ffc3de9 into ampproject:master Oct 18, 2016

cramforce deleted the chunked-amp branch October 18, 2016 16:28

RFC: Separate the load phase of AMP into multiple chunks. #5536

RFC: Separate the load phase of AMP into multiple chunks. #5536

Conversation

cramforce commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko Oct 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko Oct 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell left a comment

Choose a reason for hiding this comment

dvoytenko commented Oct 17, 2016

dvoytenko commented Oct 17, 2016

jridgewell commented Oct 17, 2016

dvoytenko Oct 12, 2016 •

edited

dvoytenko Oct 12, 2016 •

edited