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

DOMPurify: convert to a service #27260

Merged
merged 14 commits into from Mar 23, 2020
Merged

DOMPurify: convert to a service #27260

merged 14 commits into from Mar 23, 2020

Conversation

samouri
Copy link
Member

@samouri samouri commented Mar 17, 2020 •
edited

summary
Addresses #27249.

Prior to this PR, DOMPurify was initialized for each and every <amp-script> and <amp-mustache> element on the page. For mustache templates, this could sometimes be a significant bottleneck. This PR opts to instead create a single instance of DOMPurify per window. Initially I had moved it to a doc-level service, but that proved troublesome seeing as we ban the sync retrieval of doc services (#22414 (comment)), and all usages of the purifier are currently sync (converting to async would have required a lot of plumbing).

In the flamegraph below you can see that a page with ~30 <amp-mustache> elements introduces 200ms of work. After this fix, that is brought down to 26ms.

Note: screenshots were taken with 6x CPU Throttling.

before
image

after
image

@samouri samouri self-assigned this Mar 17, 2020
@samouri samouri changed the base branch from purifier-service to master March 17, 2020 01:19
@samouri samouri marked this pull request as ready for review March 17, 2020 01:19
@lgtm-com
Copy link

lgtm-com bot commented Mar 17, 2020

This pull request introduces 1 alert when merging ead7834 into b3f1936 - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

@kristoferbaxter kristoferbaxter added this to In progress in Performance Improvements via automation Mar 17, 2020
@kristoferbaxter
Copy link
Contributor

This is a great change, can you please add flamecharts from the mid-range Nokia device as well?

@samouri
Copy link
Member Author

samouri commented Mar 17, 2020

On the Nokia device:

before (244ms)
image

after (15.4ms)
image

@samouri
Copy link
Member Author

samouri commented Mar 17, 2020

@choumx: I'm confused by the lint error suggesting I switch to getServicePromiseForDoc. Why wouldn't the service be available before the constructor is called if I'm registering the service within the same extension? Is there a different pattern I could follow that wouldn't force switching the whole codepath to async?

Note that I'm only using the services plumbing to create a Singleton, the service is never used by other extensions.

jridgewell
jridgewell approved these changes Mar 17, 2020
View reviewed changes
extensions/amp-script/0.1/amp-script.js Outdated Show resolved Hide resolved
jridgewell
jridgewell requested changes Mar 17, 2020
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.

aghh github

@dreamofabear
Copy link

#22414 (comment) for context on the sync service raciness issue.

@dreamofabear
Copy link

Re: flame chart screenshots, can we keep the time interval the same across the before/after?

@samouri samouri force-pushed the purifier-service branch 3 times, most recently from 000c3cf to ffcc2ac Compare March 18, 2020 21:51
dreamofabear
dreamofabear approved these changes Mar 20, 2020
View reviewed changes
Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

Looks good, just one question.

src/purifier/purifier.js Outdated
this.doc_ = doc;
constructor(opt_config, opt_attrRewrite) {
/** @private {Document} */
this.doc_ = null;
Copy link

Choose a reason for hiding this comment

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

Why move this out of the constructor and into each function?

Copy link
Member Author

@samouri samouri Mar 20, 2020 •
edited

Choose a reason for hiding this comment

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

How else could it be a window level service that could operate on all the ampdocs of the page unless it took the doc as a function param?

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely the messiest part of this PR, since the purifier hooks are registered in the constructor but the doc it operates on changes each invocation

Copy link

Choose a reason for hiding this comment

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

Hm, does this.doc_ actually change across invocations?

A bit confusing but there can be multiple AmpDoc per window but only one Document. I.e. PWAs have one AmpDoc per shadow AMP but share a single Document. AMP ads have their own window since they're iframed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm so glad for this question then :). I can basically revert all of the changes to purifier.js now. I had assumed they had different documents.

@dreamofabear
Copy link

Let's also add some context in the PR description for posterity: why not a doc service (sync or async)?

@samouri
Copy link
Member Author

samouri commented Mar 20, 2020

New and improved before/after shots with the same time range (3200ms-7000ms) as well as some annotations:

before
before

after
after

@dreamofabear
Copy link

Love the MS Paint vibe scribbles.

jridgewell
jridgewell reviewed Mar 20, 2020
View reviewed changes
src/purifier/purifier.js Outdated Show resolved Hide resolved
src/purifier/purifier.js Outdated Show resolved Hide resolved
@samouri
Copy link
Member Author

samouri commented Mar 23, 2020 •
edited

Once we get this in, I'll create a followup PR to fix test warnings in the tests related to unexpected logging (not introduced by this PR). Writing this here as a reminder

@samouri
Copy link
Member Author

samouri commented Mar 23, 2020

@jridgewell / @choumx: I reverted most of the changes, this is a pretty small PR now. Let me know if it still looks good and if so I'll 🚢

@samouri samouri mentioned this pull request Mar 23, 2020
Merged
@samouri samouri merged commit c72c40a into ampproject:master Mar 23, 2020
Performance Improvements automation moved this from In progress to Done Mar 23, 2020
@samouri samouri deleted the purifier-service branch March 23, 2020 20:41
twintwox pushed a commit to twintwox/amphtml that referenced this pull request Mar 24, 2020
@samouri @twintwox
DOMPurify: convert to a service (ampproject#27260)
879a0b3 
* DOMPurify: convert to a service

* bad parenthesezing

* newed fns must be reg

* lint

* move purifier to window level service

* fix types and add TODO

* lint

* tests + lints

* update .d.ts

* move ivar back to its old place

* presubmit fix

* restore global state

* revert all changes related to mistaking multiple ampdocs with multiple docs

* 'this' strikes again :O
@samouri samouri mentioned this pull request Mar 30, 2020
Closed
@dreamofabear dreamofabear mentioned this pull request Apr 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kristoferbaxter kristoferbaxter kristoferbaxter left review comments

@erwinmombay erwinmombay erwinmombay left review comments

@dreamofabear dreamofabear dreamofabear approved these changes

@jridgewell jridgewell jridgewell approved these changes

Assignees

@samouri samouri

Labels
cla: yes PR use: In Beta / Experimental PR use: In Stable
Projects
Performance Improvements
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@samouri @kristoferbaxter @dreamofabear @jridgewell @erwinmombay @googlebot @ampprojectbot