Workspace: InOb-based load scheduler #31591

dvoytenko · 2020-12-14T20:33:00Z

Partial for #31915.

This is an alternative to intersect-resources launch, but it address the following key goals:

It makes element-by-element launch possible. E.g. failures in ads wouldn't block other elements.
It eliminates need for any measurements in the loading path.
It recentralizes API slightly around the custom element, thus reducing differences between AMP, Bento, FIE, and in-a-box.
This change should help us speed up removal of other measurement APIs.
Immediately eliminates the need of owner for most of cases.

Key implementation notes:

The main theme: all the scheduling/loading code is extracted from the Resources class and moved to the LoadScheduler.
Load scoring and timeouts are left mostly unchanged from the current version.
It's implemented via a set of IntersectionObservers, each got different ranges of loading: 1 viewport, 3 viewports, etc.
Only elements that return true from isLoadableV2 go into this loading path.

TODO:

Update inabox resource manager
Tests
Improve docs

lgtm-com · 2020-12-14T20:56:50Z

This pull request introduces 1 alert when merging 2db1d1e into 3a31bb2 - view on LGTM.com

new alerts:

1 for Missing 'this' qualifier

lgtm-com · 2021-01-12T18:26:13Z

This pull request introduces 1 alert when merging 337e8e9 into e1e8a4c - view on LGTM.com

new alerts:

1 for Missing 'this' qualifier

dvoytenko · 2021-01-14T22:05:46Z

Switching this to draft to look into deferred build approach.

samouri · 2021-01-15T00:26:17Z

would this help get rid of child-layout-manager? Would help with various layout issues, like #30719

dvoytenko · 2021-01-15T01:17:15Z

Could we get rid of child-layout-manager? Would help with various layout issues, like #30719

That'd probably be resolved with #31948

samouri · 2021-01-15T00:57:08Z

src/base-element.js

+   *
+   * @return {!Promise|undefind}
+   */
+  loadCallback() {


I feel that this (and build/layout) should not have callback in their names. I gave the full reasoning here #31046 (comment). The tldr is that we are asking an element to load as opposed to informing it that load has happened.

I understand the desire to match CE's naming scheme, but IMO it doesn't make sense for these methods

samouri · 2021-01-15T03:40:38Z

src/custom-element.js

@@ -396,6 +409,15 @@ function createBaseCustomElementClass(win) {
      return this.signals_.whenSignal(CommonSignals.BUILT);
    }

+    /**
+     * Returns the promise that's rewsolved when the element has been loaded.


nit: rewsolved --> resolved

samouri · 2021-01-15T03:42:30Z

src/custom-element.js

@@ -877,6 +905,9 @@ function createBaseCustomElementClass(win) {
    /** @private */
    connected_() {
      if (this.built_) {
+        if (this.isLoadableV2()) {


Isn't one of the big points here that we don't need to branch on isBuilt() anymore? I.e. scheduleLoad can be extracted from the condition since the definition of load() is build().then(() => this.layout()))

samouri · 2021-01-15T03:44:58Z

src/custom-element.js

@@ -1226,6 +1256,9 @@ function createBaseCustomElementClass(win) {
      const isReLayoutNeeded = this.implementation_.unlayoutCallback();


nit: isReLayoutNeeded --> isRelayoutNeeded

samouri · 2021-01-15T03:50:36Z

src/service/load-scheduler.js

+      const {win} = this.ampdoc_;
+      const opts = {
+        root: win.document,
+        rootMargin: `${range * 100}% ${range * 25}%`,


can you explain the horizontal margin?

samouri · 2021-01-15T03:57:10Z

src/service/load-scheduler.js

+
+    // Unschedule to reschedule with the new parameters.
+    if (this.tasks_.has(id)) {
+      this.unschedule_(resource, /* canceled */ true);


Should the be canceled=false?

samouri · 2021-01-15T04:08:37Z

src/service/resource.js

@@ -672,6 +700,9 @@ export class Resource {
   * @return {!../layout-rect.LayoutRectDef}
   */
  getLayoutBox() {
+    if (this.loaderRectV2_) {


Does v2 not need to manually move fixed elements? What about intersect-resources?

CLAassistant · 2023-05-25T15:18:42Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Dima Voytenko seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

dvoytenko requested a review from samouri December 14, 2020 20:33

dvoytenko force-pushed the run3/new-resources branch from 2db1d1e to 8d5c939 Compare January 12, 2021 18:00

InOb-based load scheduler

a33b707

dvoytenko force-pushed the run3/new-resources branch from 337e8e9 to a33b707 Compare January 13, 2021 01:21

dvoytenko requested a review from jridgewell January 13, 2021 01:25

dvoytenko marked this pull request as ready for review January 13, 2021 01:31

Dima Voytenko added 2 commits January 13, 2021 12:59

ensure that measure APIs are not needed for loaderV2

e5c63a0

do not recompute media/sizes for loaderV2

b6bba50

dvoytenko marked this pull request as draft January 14, 2021 22:05

samouri reviewed Jan 15, 2021

View reviewed changes

dvoytenko mentioned this pull request Jan 15, 2021

Workspace: deferred build mode #32002

Closed

dvoytenko changed the title ~~InOb-based load scheduler~~ Workspace: InOb-based load scheduler Mar 22, 2021

jridgewell removed their request for review August 31, 2022 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workspace: InOb-based load scheduler #31591

Workspace: InOb-based load scheduler #31591

dvoytenko commented Dec 14, 2020 •

edited

Loading

lgtm-com bot commented Dec 14, 2020

lgtm-com bot commented Jan 12, 2021

dvoytenko commented Jan 14, 2021

samouri commented Jan 15, 2021 •

edited

Loading

dvoytenko commented Jan 15, 2021

samouri Jan 15, 2021 •

edited

Loading

samouri Jan 15, 2021

samouri Jan 15, 2021 •

edited

Loading

samouri Jan 15, 2021

samouri Jan 15, 2021 •

edited

Loading

samouri Jan 15, 2021

samouri Jan 15, 2021

CLAassistant commented May 25, 2023

		@@ -1226,6 +1256,9 @@ function createBaseCustomElementClass(win) {
		const isReLayoutNeeded = this.implementation_.unlayoutCallback();

Workspace: InOb-based load scheduler #31591

Are you sure you want to change the base?

Workspace: InOb-based load scheduler #31591

Conversation

dvoytenko commented Dec 14, 2020 • edited Loading

lgtm-com bot commented Dec 14, 2020

lgtm-com bot commented Jan 12, 2021

dvoytenko commented Jan 14, 2021

samouri commented Jan 15, 2021 • edited Loading

dvoytenko commented Jan 15, 2021

samouri Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

samouri Jan 15, 2021

Choose a reason for hiding this comment

samouri Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

samouri Jan 15, 2021

Choose a reason for hiding this comment

samouri Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

samouri Jan 15, 2021

Choose a reason for hiding this comment

samouri Jan 15, 2021

Choose a reason for hiding this comment

CLAassistant commented May 25, 2023

dvoytenko commented Dec 14, 2020 •

edited

Loading

samouri commented Jan 15, 2021 •

edited

Loading

samouri Jan 15, 2021 •

edited

Loading

samouri Jan 15, 2021 •

edited

Loading

samouri Jan 15, 2021 •

edited

Loading