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.

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

Introduce "nodom" binary #882

Merged
merged 20 commits into from
Jul 22, 2020
Merged

Introduce "nodom" binary #882

merged 20 commits into from
Jul 22, 2020

Conversation

samouri
Copy link
Member

@samouri samouri commented Jul 15, 2020

summary
Interactive pieces of AMP would like to safely run arbitrary JS without impacting the main thread. While often that may involve dom mutation, there are instances where that isn't necessary. A current example is Protocol Adapters.

For cases like these, it doesn't make sense to ship a binary containing the vdom implementation. Instead we could ship a dom-less "lite" version of worker-dom.

  • For the main-thread code, this means removing all of the unnecessary Executors.
  • For worker-thread code, this means ensuring Element and friends don't enter the build.

The method for removing the unnecessary code was similar in both cases.
Namely, hoisting the relevant to the entrypoints.

results (mjs)

main-thread worker-thread
AMP 11.24 kb 31.04 kb
AMP nodom 7.14 kb 1.45 kb

Note: using the rollup-plugin-analyzer I was able to detect further improvements we could make to main-thread code. worker-thread seemed relatively optimized. I plan on adding this as a dev dependency in a future PR.

samouri added 5 commits July 15, 2020 18:31
Sometimes developers may want to run arbitrary JS on a Worker safely/without access to a DOM.
In those situations, the whole vdom implementation is overkill.

This provides a DocumentLite with none of the implementation.
@samouri samouri self-assigned this Jul 16, 2020
@samouri
Copy link
Member Author

samouri commented Jul 16, 2020

Based on the ease of integration with amp-script, I'm going to split out the main-thread optimization into its own PR. Mainly since I'm unsure how possible it is to take advantage of it (would require lazily loading the code or creating multiple amp-script binaries).

Since the worker-thread code is loaded dynamically, we can continue to do that and integration should be easy.

src/worker-thread/dom/DocumentLite.ts Outdated Show resolved Hide resolved
src/worker-thread/index.lite.amp.ts Outdated Show resolved Hide resolved
src/worker-thread/long-task.ts Outdated Show resolved Hide resolved
@samouri samouri requested a review from dreamofabear July 16, 2020 22:10
Copy link
Collaborator

@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.

Nit: Maybe we can find a name more descriptive than "lite". This feels like "worker-dom-but-no-dom". :)

E.g. "index.nodom.amp.ts" and "DocumentStub". Or "AntiDocument"... "NegaDocument".

src/worker-thread/index.amp.ts Outdated Show resolved Hide resolved
@samouri samouri changed the title Introduce "lite" binary Introduce "nodom" binary Jul 20, 2020
@samouri
Copy link
Member Author

samouri commented Jul 20, 2020

Screen Shot 2020-07-20 at 12 45 09 PM

@kristoferbaxter
Copy link
Contributor

Quick comment while out on leave: Please verify the contents of these new binaries, since the mjs output should always be smaller than the js output.

@samouri
Copy link
Member Author

samouri commented Jul 20, 2020

Quick comment while out on leave: Please verify the contents of these new binaries, since the mjs output should always be smaller than the js output.

Good to know! I wasn't sure if the lack of ...compilePlugin (terser) was intentional for the amp mjs binary or not. I'm assuming not then :). Fixing here: #885

Updated mjs/js comparison for this new binary:
Screen Shot 2020-07-20 at 3 37 40 PM

@samouri
Copy link
Member Author

samouri commented Jul 20, 2020

Quick comment while out on leave: Please verify the contents of these new binaries, since the mjs output should always be smaller than the js output.

@kristoferbaxter: seems like the mjs binaries are actually supposed to be bigger for AMP. The mjs builds are used for dev and the js for prod builds (.max.js vs. .min.js).

This reverts commit f5679a1.
src/worker-thread/dom/DocumentLite.ts Outdated Show resolved Hide resolved
src/worker-thread/WorkerDOMGlobalScope.ts Outdated Show resolved Hide resolved
src/worker-thread/delete-globals.ts Outdated Show resolved Hide resolved
src/worker-thread/delete-globals.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants