Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Workspace: deferred build mode #32002

Closed
wants to merge 1 commit into from
Closed

Workspace: deferred build mode #32002

wants to merge 1 commit into from

Conversation

dvoytenko
Copy link
Contributor

@dvoytenko dvoytenko commented Jan 15, 2021 •
edited
Loading

Partial for #31915.
Possibly conflicts with #31591.

The key points:

  • A custom element has the static layout applied, but it's not constructed or built until later.
  • An IntersectionObserver checks an element's parsed state and intersection with 3.5 viewports and builds it then.
  • An element can be built manually.
  • Prerender mode is supported.
  • The current attempt implements "auto-layout". We need to discuss whether this is a good idea or not.
  • The onload/onerror structure allows to pick up multiple loading events when src changes, an iframe is paused, connect/reconnect happened, etc.

jridgewell
jridgewell reviewed Jan 15, 2021
View reviewed changes
Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need experiment checks, too.

builtins/amp-img.js Outdated Show resolved Hide resolved
extensions/amp-video/0.1/amp-video.js Outdated Show resolved Hide resolved
src/custom-element.js Outdated Show resolved Hide resolved
src/custom-element.js Outdated Show resolved Hide resolved
src/custom-element.js Outdated Show resolved Hide resolved
src/custom-element.js Outdated Show resolved Hide resolved
src/element-stub.js Outdated
return false;
}
}
export class ElementStub extends BaseElement {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could simplify the upgrade code if we make ElementStub a build-deferred extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that way. But I actually in the end liked the idea of not allocating any new object at all. And a bunch of if (this.impl_) {} I ended up not hating too much. But I can be convinced otherwise.

src/service/deferred-builder.js Outdated
this.unschedule(target);
// DO NOT SUBMIT: check the perform of this `setTimeout`. How else to ensure
// we do not over-build in a same task?
this.ampdoc_.win.setTimeout(() => target.build());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setTimeout is extremely slow when called multiple times in a tick.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I'm still investigating this part. Not strictly necessary, but seemed like it'd be a missed opportunity to defer the work, but then still introduce a long task. Do you have any suggestions? I'd like to return control to the browser at least some of the time.

src/custom-element.js Outdated Show resolved Hide resolved
@dvoytenko dvoytenko marked this pull request as draft January 27, 2021 20:59
This was referenced Feb 2, 2021
Merged
Merged
@dvoytenko dvoytenko force-pushed the run3/deferred-build branch 2 times, most recently from 3b3b3fb to 8329d8f Compare February 4, 2021 19:23
@dvoytenko dvoytenko mentioned this pull request Feb 4, 2021
Merged
@dvoytenko dvoytenko force-pushed the run3/deferred-build branch 5 times, most recently from ecc3a69 to 1a90eb8 Compare February 9, 2021 22:31
@samouri
Copy link
Member

samouri commented Feb 12, 2021

Close this in favor of #32568 ?

@dvoytenko
Copy link
Contributor Author

Close this in favor of #32568 ?

This is a draft. I will keep it open until I move all features into other PRs. There are still a few of them here outside of #32568.

@dvoytenko dvoytenko removed the request for review from samouri February 18, 2021 20:13
@dvoytenko dvoytenko force-pushed the run3/deferred-build branch 3 times, most recently from 1cbdec1 to 8658520 Compare March 4, 2021 01:34
@dvoytenko dvoytenko changed the title Deferred build mode Workspace: deferred build mode Mar 4, 2021
@dvoytenko dvoytenko force-pushed the run3/deferred-build branch 2 times, most recently from 208cac9 to 5e5efed Compare March 4, 2021 01:43
@stale
Copy link

stale bot commented Sep 21, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Sep 21, 2022
@stale stale bot closed this Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jridgewell jridgewell jridgewell left review comments

Assignees
No one assigned
Labels
Stale Inactive for one year or more
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dvoytenko @samouri @jridgewell