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

Optimize preconnect callbacks #23557

Merged
merged 16 commits into from
Aug 12, 2019
Merged

Optimize preconnect callbacks #23557

merged 16 commits into from
Aug 12, 2019

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented Jul 26, 2019 •
edited
Loading

Closes #22975

Before:
image

After (using startupChunk):
image

@Enriqe Enriqe self-assigned this Jul 26, 2019
@Enriqe Enriqe force-pushed the pre-fcp-perf-improvements branch from 9b8676a to dda1360 Compare July 26, 2019 21:19
@Enriqe Enriqe changed the title Batch preconnect timer and clean AmpEvents.BUILT [wip] Batch preconnect timer and clean AmpEvents.BUILT Jul 29, 2019
jridgewell
jridgewell requested changes Jul 29, 2019
View reviewed changes
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/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
extensions/amp-fx-flying-carpet/0.1/amp-fx-flying-carpet.js Outdated
@@ -102,7 +101,13 @@ export class AmpFlyingCarpet extends AMP.BaseElement {
});
if (this.firstLayoutCompleted_) {
this.scheduleLayout(this.children_);
listen(this.element, AmpEvents.BUILT, this.layoutBuiltChild_.bind(this));
this.children_.forEach(child => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't equivalent. We're listening for new children to be added to the flying carpet, not for already know children to upgrade. "Necessary since not all children will be built by the time the flying-carpet has its #layoutCallback called."

Let's split this into a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Added possibly more complete impl. to #23767

This was referenced Aug 7, 2019
Merged
Merged
@Enriqe Enriqe force-pushed the pre-fcp-perf-improvements branch from 3dabafd to d863b6b Compare August 7, 2019 23:31
@Enriqe Enriqe changed the title [wip] Batch preconnect timer and clean AmpEvents.BUILT Batch timer delay callbacks Aug 7, 2019
jridgewell
jridgewell requested changes Aug 7, 2019
View reviewed changes
src/service/timer-impl.js Outdated
delete this.delayQueue_[delay];

for (let i = 0; i < callbacks.length; i++) {
const cb = callbacks[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be wrapped in a try-catch, since any cb could throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This cb is already wrapped in a try-catch before being pushed to the queue, or am I missing something?

src/service/timer-impl.js Outdated Show resolved Hide resolved
cramforce
cramforce requested changes Aug 8, 2019
View reviewed changes
Copy link
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

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

I don't think this approach is going to work in practice, because it relies on the calls coming in the same milli second and has the additional downside that individual callbacks are no longer scheduled as tasks.

I think this might be as simple as replacing the timer.delay call with startupTask

@Enriqe Enriqe force-pushed the pre-fcp-perf-improvements branch from c51e054 to e53519d Compare August 8, 2019 05:58
@jridgewell
Copy link
Contributor

because it relies on the calls coming in the same milli second

Which is the use case from #22975. But it's not just in the same ms, it's scheduled to run at the same ms. So time = 100, delay = 10 and time = 101, delay = 9 will share a timeout now.

and has the additional downside that individual callbacks are no longer scheduled as tasks.

What's the downside here? It'll require a try-catch, but this should work fine.

@Enriqe Enriqe force-pushed the pre-fcp-perf-improvements branch from 26c084c to bf101f6 Compare August 8, 2019 17:10
@cramforce
Copy link
Member

Downside is that e.g. user actions cannot interrupt execution.

@Enriqe Enriqe force-pushed the pre-fcp-perf-improvements branch 2 times, most recently from 1e8a396 to 56b6385 Compare August 8, 2019 23:15
@Enriqe Enriqe force-pushed the pre-fcp-perf-improvements branch from 56b6385 to 4fd50dc Compare August 9, 2019 21:08
@Enriqe
Copy link
Contributor Author

Enriqe commented Aug 9, 2019

Ready for another review 👍

@Enriqe Enriqe changed the title Batch timer delay callbacks Optimize preconnect callbacks Aug 9, 2019
@Enriqe Enriqe force-pushed the pre-fcp-perf-improvements branch from 4fd50dc to 87a4a95 Compare August 9, 2019 21:29
@Enriqe Enriqe force-pushed the pre-fcp-perf-improvements branch from 87a4a95 to a6463ff Compare August 9, 2019 21:35
@Enriqe Enriqe merged commit 69a1bba into ampproject:master Aug 12, 2019
@Enriqe Enriqe deleted the pre-fcp-perf-improvements branch August 12, 2019 13:50
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
@Enriqe
Optimize preconnect callbacks (ampproject#23557)
2d845c4 
* batch preconnects and clean AmpEvents.BUILT

* fix conflict

* fix call

* types

* change debounce to 1ms, polish comments

* revert commits

* feedback

* don't return timeout id

* fix jsdoc

* replace delay call with startupchunk

* revert type changes

* whitelist startupchunk

* fix conflicts

* fix test

* add fortesting

* fix test: force macrotask before checking if preconnect was called
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jridgewell jridgewell jridgewell approved these changes

@cramforce cramforce cramforce approved these changes

Assignees

@Enriqe Enriqe

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Preconnect timer and BUILT event delays FCP
4 participants
@Enriqe @jridgewell @cramforce @googlebot