Skip to content

Element-level signals for lifecycle events#7205

Merged
dvoytenko merged 2 commits intoampproject:masterfrom
dvoytenko:fie14-signals
Jan 26, 2017
Merged

Element-level signals for lifecycle events#7205
dvoytenko merged 2 commits intoampproject:masterfrom
dvoytenko:fie14-signals

Conversation

@dvoytenko
Copy link
Copy Markdown
Contributor

@dvoytenko dvoytenko commented Jan 25, 2017

We have more and more use cases that need major lifecycle signals produced by standard AMP element and extensions. We can implement each separately, but there are some issues:

  1. Most of signals will not be used. Thus we want minimal CPU/memory until they are requested.
  2. Most of life-cycle events can be split into two signals: start and end.
  3. Individual code is a mess: it needs time, error, promise, resolver and rejector for all needs.

This PR:

  1. Creates a minimally filled struct per signal and adds three methods to support it: whenSignal, signal and rejectSignal.
  2. The initial set of signals demonstrated here is small, but extensions can supply their own. That's why these methods are public.
  3. signal is the only "always use" method and thus it fills it only minimal parts of the struct and does minimal work. The rest is done lazily.
  4. whenBuilt is an example of API-level usage. Some lifecycle signals can be easily exposed via special API. This is currently in work for another PR. (/cc @mkhatib Wait for amp-selector to build before submitting #7190)
  5. signalDelta is an example of how many metrics can be calculated between two signals. This will be (already is) very common in many analytics needs.

Copy link
Copy Markdown
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.

Could some parts of the performance service be rewritten in terms of this?

I am pretty worried about the extra memory usage of this. It might be worth limiting to a single array per object where each signal gets 6 (would need to add the name) slots in the array. This would avoid any object allocation per signal that never get a promise created.

*/
signal(name, opt_time) {
let signal = this.signals_[name];
if (!signal) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Assign all fields to undefined to avoid change to object shape.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed to separate map.

@dvoytenko dvoytenko changed the title WORK IN PROGRESS: Element-level signals for lifecycle events Element-level signals for lifecycle events Jan 25, 2017
@dvoytenko
Copy link
Copy Markdown
Contributor Author

@cramforce Reimplemented via two maps: one for result and one for promise. PTAL.

@dvoytenko
Copy link
Copy Markdown
Contributor Author

@cramforce RE:performance. Not yet, but some aspects in Resources will eventually be made simpler that performance relies on. However, other elements, such as amp-sticky-ad could now be simplified via signals.

Copy link
Copy Markdown
Contributor

@mkhatib mkhatib left a comment

Choose a reason for hiding this comment

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

LGTM!

@dvoytenko dvoytenko merged commit 8be35f4 into ampproject:master Jan 26, 2017
@dvoytenko dvoytenko deleted the fie14-signals branch January 26, 2017 18:00
jridgewell pushed a commit to jridgewell/amphtml that referenced this pull request Jan 31, 2017
* Element-level signals for lifecycle events

* fixing tests
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
* Element-level signals for lifecycle events

* fixing tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants