intersect-resources: Refactor to a lower risk integration #27497
Conversation
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. Don't handle prerenderSize_ == 0. Refactor stuff. Less efficient but much more simple. Missing ivar init. Fix types. Remove logs for apply sizes/media query. Remove commented out code. Refactor to use one observer and calculate 1vp. Support 25% larger 'visible rect'. Move log, fix types. Fix missing experiment guard. Add assert in resource.measure, early out if resource was removed.
| this.state_ = ResourceState.NOT_LAID_OUT; | ||
| // With IntersectionObserver, measure can happen before build | ||
| // so check if we're "ready for layout" (measured and built) here. | ||
| if (this.intersect_) { |
There was a problem hiding this comment.
Nit: you don't need to experiment-gate this if you make hasBeenMeasured() return false outside the experiment
There was a problem hiding this comment.
I think it's better to be explicit here rather than hide it a subfunction. In the classical system, resources sequentially follow the phases in ResourceState (e.g. build -> measure -> layout) which is somewhat subverted here.
Also hasBeenMeasured() is used in many places.
There was a problem hiding this comment.
whoops, i was mixing up hasBeenMeasured and hasBeenPremeasured
| element.whenUpgraded().then(() => { | ||
| this.intersectionObserver_.observe(resource.element); | ||
| }); | ||
| this.intersectionObserver_.observe(element); |
There was a problem hiding this comment.
Why do we no longer care about waiting for whenUpgraded?
There was a problem hiding this comment.
Similar to removal of other minor edge case handling -- I thought it's better to keep things simple for now to catch the bigger problems first. E.g. this was complicating reasoning about the reparenting race condition. Might need to reintroduce later though.
There was a problem hiding this comment.
What edge case did it handle? Or was it a perf optimization?
There was a problem hiding this comment.
The edge case is that the IO callback happens too early and the resource stores a stale width/height. One possible fix to fix is it to delay observation (e.g. applyStaticLayout happens before upgrade). Another way is to use a ResizeObserver.
| */ | ||
| measure(opt_premeasuredRect) { | ||
| measure(usePremeasuredRect = false) { |
There was a problem hiding this comment.
Would it make sense to always use premeasuredRect if available? When will there ever be a premeasured rect that we don't intend on using?
There was a problem hiding this comment.
Yes, sometimes we still do explicit measures e.g. after a component requests a size-change. The only time we use the "premeasured rect" is in the pass immediately after an intersection callback.
There was a problem hiding this comment.
Yes, sometimes we still do explicit measures e.g. after a component requests a size-change.
In that case, there shouldn't be a premeasuredRect though, since it is set to null after being used. Unless you are worried about a race condition between a size-change and IO callback
There was a problem hiding this comment.
Yea this is a defensive check e.g. make sure the premeasuredRect is not accidentally consumed by a different codepath.
|
Thanks for the review! |
Partial for #25428. An alternative to #27176.
I originally was hoping to eventually remove the "measure pass" (
discoverWork_()) from Resources, because once we no longer need explicit measures we won't need to carefully phase measures/mutates to avoid layout thrashing. However, since discovering a tricky race condition (#27483), I'm more wary about diverging from the "pass" system in Resources due to other hidden dragons that may be lurking.This refactor changes IntersectionObserver to simply store client rects on a Resource such that it'll be consumed during the next measure pass. Reconstructed resources (element reparenting) will be unobserved/observed and receive a fresh callback and client rect.
Here are some flame charts on examples/article.amp.html on the Nokia 2.3. It's hard to get a consistent baseline across runs due to noise, but you can see the width of several flames are much smaller in the "after".
Before
After
/to @dvoytenko @jridgewell