intersect-resources: Support prerender mode, patch for stale rects #27176

dreamofabear · 2020-03-10T17:22:00Z

Partial for #25428.

Keep using a single observer at 3vp and compute intersection with 1vp. In prerender mode, queue intersections outside of 1vp.
Addresses the stale layout box issue by reverting to continue to use the current system i.e. requesting measure on mutations, scheduling passes after element size changes, etc.
Removes some of the edge case handling for simplicity. I'll restore them if needed later.

Will add tests next.

/to @dvoytenko @jridgewell /cc @ampproject/wg-runtime

Add comment about first intersection callback in Chrome 82. Only prerender 1 viewport (if prerenderSize > 0). Create 3vp observer after first 1vp observer callback. Wait until visible to start 3vp observer. First firstCallback. Avoid double-handling by early-out when isIntersecting didn't change. Add comment. Simplify handling of first intersection callback. Tweak comment. Use ampdoc instead of viewer. Add comments.

src/service/resources-impl.js

dreamofabear

Good questions.

src/service/resources-impl.js

dreamofabear · 2020-03-16T20:31:06Z

@jridgewell Friendly ping.

dvoytenko · 2020-03-16T20:41:41Z

src/service/resources-impl.js

@@ -193,8 +193,17 @@ export class ResourcesImpl {
      this.ampdoc.getVisibilityState()
    );

-    /** @private {?IntersectionObserver} */
-    this.intersectionObserver_ = null;
+    /**


The bigger question I have: if this continues to diverge in major ways, would it be easier to just fork this whole manager for the InOb experiment?

Good question. I only have a few small changes left to do, so this is nearly the final state.

Also I'm concerned about bundle size since we can't leverage module/nomodule or other tricks easily.

dvoytenko · 2020-03-17T23:40:50Z

src/service/resources-impl.js

+      for (let i = 0; i < this.resources_.length; i++) {
+        const r = this.resources_[i];
+        if (r.getState() == ResourceState.NOT_BUILT && !r.isBuilding()) {
+          this.buildOrScheduleBuildForResource_(r, /* checkForDupes */ true);


We are blocking build? We've not blocked build before - only loading. This is certain to be positive, but might have lots of regressions with the elements that do some early work on build?

Also, does the whenBuilt never fail for any individual elements, or all together? Overall this seems like a pretty strict rule. What if a 1vp element is very slow in download?

Yea, I was originally planning to split this out but I think this should be safe to do since we already do something similar in prerender mode with our quota system.

Good points on being bottlenecked by slow outliers. I'll think about how to mitigate.

src/service/resources-impl.js

jridgewell

I think the handling of the two observers is weird. Why not just register both, and record what the actual distance to viewport is? Make you decisions from there.

dreamofabear · 2020-03-19T00:13:30Z

I think the handling of the two observers is weird. Why not just register both, and record what the actual distance to viewport is? Make you decisions from there.

Heh, I assume you're talking about the lazy instantiation of 3vp observer? After observing an element, there's an initial intersection entry/callback (even if not intersecting). So lazy instantiation (1) allows the browser to defer the work and (2) avoids the need for us to store/queue the entry.

Alternatively, we could try just using one observer and always compute viewport distance. But this trades the complexity of two observers for a more complex handling with one observer. It's not clear to me that it's better, though I can try it out.

dreamofabear · 2020-03-19T23:34:22Z

Changed back to use one observer. Much simpler code, though at the cost of a few extra calculations and potentially staler rects due to queueing during prerender.

Also removed the deferral of build on upgrade (will do that separately).

dreamofabear · 2020-03-21T00:34:12Z

@dvoytenko @jridgewell PTAL, the diff is much less scary now. :)

In other news, the spec change was supposed to land in Chrome 81 which has now been postponed unfortunately. The experiment can still activate on non-iframed pages.

dvoytenko

LGTM with one q.

dvoytenko · 2020-03-24T17:47:22Z

src/service/resources-impl.js

-  intersects_(entries, unusedObserver) {
+  intersects_(entries) {
+    // TODO(willchou): Remove assert once #27167 is fixed.
+    devAssert(this.prerenderSize_ == 1);


What's the value of the devAssert() for this?

Mostly as an FYI for readers. Assert would trigger for the few people that are playing with prerenderSize in development (maybe just me).

dreamofabear · 2020-03-30T16:59:51Z

Update: discovered a preexisting race condition (#27483) that affects the current IntersectionObserver implementation. Fixing this race is easy but I'm leaning towards a lower impact/risk integration that keeps most of the discoverWork_ codepath intact for MVP.

Closing this PR for now.

William Chou added 2 commits March 10, 2020 11:49

Don't handle prerenderSize_ == 0.

9d20841

googlebot added the cla: yes label Mar 10, 2020

amp-bundle-size bot requested a review from samouri March 10, 2020 17:31

samouri reviewed Mar 12, 2020

View reviewed changes

src/service/resources-impl.js Outdated Show resolved Hide resolved

dreamofabear requested review from dvoytenko and jridgewell March 12, 2020 16:45

samouri reviewed Mar 12, 2020

View reviewed changes

src/service/resources-impl.js Outdated Show resolved Hide resolved

samouri reviewed Mar 12, 2020

View reviewed changes

src/service/resources-impl.js Outdated Show resolved Hide resolved

samouri reviewed Mar 12, 2020

View reviewed changes

src/service/resources-impl.js Outdated Show resolved Hide resolved

dreamofabear commented Mar 12, 2020

View reviewed changes

src/service/resources-impl.js Outdated Show resolved Hide resolved

src/service/resources-impl.js Outdated Show resolved Hide resolved

src/service/resources-impl.js Outdated Show resolved Hide resolved

src/service/resources-impl.js Outdated Show resolved Hide resolved

dvoytenko suggested changes Mar 16, 2020

View reviewed changes

William Chou added 4 commits March 17, 2020 01:41

Refactor stuff.

89c9db8

Less efficient but much more simple.

548534e

Missing ivar init.

c26f6cc

Fix types.

1dec647

dreamofabear changed the title ~~intersect-resources: Support prerender mode~~ intersect-resources: Support prerender mode, revert to fix stale layout boxes Mar 17, 2020

dreamofabear changed the title ~~intersect-resources: Support prerender mode, revert to fix stale layout boxes~~ intersect-resources: Support prerender mode, patch for stale rects Mar 17, 2020

William Chou added 2 commits March 17, 2020 18:41

Remove logs for apply sizes/media query.

0f7ddd2

Remove commented out code.

a4d2e90

dvoytenko reviewed Mar 17, 2020

View reviewed changes

kristoferbaxter reviewed Mar 18, 2020

View reviewed changes

src/service/resources-impl.js Outdated Show resolved Hide resolved

jridgewell reviewed Mar 18, 2020

View reviewed changes

William Chou added 2 commits March 19, 2020 18:47

Refactor to use one observer and calculate 1vp.

8e54612

Support 25% larger 'visible rect'.

3ea7feb

dreamofabear mentioned this pull request Mar 19, 2020

IntersectionObserver: Prerender support with one observer #27327

Closed

dvoytenko approved these changes Mar 24, 2020

View reviewed changes

Move log, fix types.

7148444

dreamofabear force-pushed the intobs-two-observers branch from 9d506d1 to 7148444 Compare March 25, 2020 00:46

Fix missing experiment guard.

bd07d62

dreamofabear closed this Mar 30, 2020

dreamofabear mentioned this pull request Mar 30, 2020

intersect-resources: Refactor to a lower risk integration #27497

Merged

dreamofabear deleted the intobs-two-observers branch April 3, 2020 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

intersect-resources: Support prerender mode, patch for stale rects #27176

intersect-resources: Support prerender mode, patch for stale rects #27176

dreamofabear commented Mar 10, 2020 •

edited

Loading

dreamofabear left a comment

dreamofabear commented Mar 16, 2020

dvoytenko Mar 16, 2020

dreamofabear Mar 16, 2020

dvoytenko Mar 17, 2020

dreamofabear Mar 18, 2020

jridgewell left a comment

dreamofabear commented Mar 19, 2020

dreamofabear commented Mar 19, 2020

dreamofabear commented Mar 21, 2020

dvoytenko left a comment

dvoytenko Mar 24, 2020

dreamofabear Mar 24, 2020

dreamofabear commented Mar 30, 2020

intersect-resources: Support prerender mode, patch for stale rects #27176

intersect-resources: Support prerender mode, patch for stale rects #27176

Conversation

dreamofabear commented Mar 10, 2020 • edited Loading

dreamofabear left a comment

Choose a reason for hiding this comment

dreamofabear commented Mar 16, 2020

dvoytenko Mar 16, 2020

Choose a reason for hiding this comment

dreamofabear Mar 16, 2020

Choose a reason for hiding this comment

dvoytenko Mar 17, 2020

Choose a reason for hiding this comment

dreamofabear Mar 18, 2020

Choose a reason for hiding this comment

jridgewell left a comment

Choose a reason for hiding this comment

dreamofabear commented Mar 19, 2020

dreamofabear commented Mar 19, 2020

dreamofabear commented Mar 21, 2020

dvoytenko left a comment

Choose a reason for hiding this comment

dvoytenko Mar 24, 2020

Choose a reason for hiding this comment

dreamofabear Mar 24, 2020

Choose a reason for hiding this comment

dreamofabear commented Mar 30, 2020

dreamofabear commented Mar 10, 2020 •

edited

Loading