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

Implement Houdini worklets #16814

Merged
merged 1 commit into from May 17, 2017
Merged

Conversation

asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented May 11, 2017

This PR implements the current draft Houdini Worklets specification (https://drafts.css-houdini.org/worklets/).

The implementation is intended to provide a responsive environment for worklet execution, and in particular to ensure that the primary worklet executor does not garbage collect, and does not block loading module code. The implementation does this by providing a thread pool, and performing GC and module loading in a backup thread, not in the primary thread.



This change is Reviewable

@asajeffrey asajeffrey added the A-content/script Related to the script thread label May 11, 2017
@highfive
Copy link

Heads up! This PR modifies the following files:

  • @wafflespeanut: python/tidy/servo_tidy/tidy.py
  • @fitzgen: components/script/dom/webidls/Console.webidl, components/script/dom/testworkletglobalscope.rs, components/script/dom/mod.rs, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl and 20 more
  • @KiChjang: components/script/dom/webidls/Console.webidl, components/script/dom/testworkletglobalscope.rs, components/script/dom/mod.rs, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl and 17 more

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 11, 2017
@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@asajeffrey
Copy link
Member Author

r? @jdm

@highfive highfive assigned jdm and unassigned mbrubeck May 11, 2017
@asajeffrey
Copy link
Member Author

cc @tschneidereit @metajack

@metajack
Copy link
Contributor

cc @bfgeek

@asajeffrey
Copy link
Member Author

Some things that should probably be fixed before this lands:

  • the methods in globalscope which use downcasting are missing some WorkletGlobalScope cases.
  • threads should be named.
  • add time reporting code.
  • think about how to configure SM for worklet threads (in particular how to discourage GC).

The code is ready for review as-is though.

@asajeffrey
Copy link
Member Author

Hi @bfgeek! This is implementation of worklets in Servo, using a thread pool to ensure worklet execution isn't blocked by GC or by module loading. Mostly the spec was pretty straightforward to implement, you'll be pleased to hear!

@asajeffrey
Copy link
Member Author

The spec issues raised by implementing worklets (and starting on paint worklets):

@bfgeek
Copy link

bfgeek commented May 12, 2017

cc/ @nhiroki who works on our implementation.

That's great to see! Thanks for filing spec issues.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #16845) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label May 13, 2017
// https://drafts.css-houdini.org/worklets/#examples
partial interface Window {
[Pref="dom.worklet.testing.enabled", SameObject] readonly attribute Worklet testWorklet;
[Pref="dom.worklet.testing.enabled"] DOMString? testWorkletLookup(DOMString key);
Copy link
Member

Choose a reason for hiding this comment

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

What if we made a separate DOM interface with a constructor so we don't need to include test-only fields in Window?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

pub mem_profiler_chan: mem::ProfilerChan,
/// Chan to the time profiler
pub time_profiler_chan: time::ProfilerChan,
/// Chan to devtools
Copy link
Member

Choose a reason for hiding this comment

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

Let's use channel instead of chan in these comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

use style::thread_state;
use swapper::Swapper;
use swapper::swapper;
use uuid::Uuid;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to collapse some of these import statements from the same modules?

Copy link
Member Author

Choose a reason for hiding this comment

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

Minimizing the chance of annoying merge conflicts.

let global = window.upcast::<GlobalScope>();
unsafe {
WorkletBinding::Wrap(global.get_cx(), global, worklet)
}
Copy link
Member

Choose a reason for hiding this comment

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

We should use reflect_dom_object instead.

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 don't think you can use reflect_dom_object to create globals can you?

Copy link
Member

Choose a reason for hiding this comment

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

WorkletBinding isn't a global.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good point, we're in Worklet not WorkletGlobalScope.

self.worklet_id
}

#[allow(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is dead code, just in for future-proofing. I can get rid of it.

#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, JSTraceable)]
pub struct WorkletId(Uuid);

known_heap_size!(0, WorkletId);
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer using #[ignore_heap_size_of = "Can't measure uuid"] instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

}

fn decrement_counter_by(&self, offset: isize) -> isize {
self.0.fetch_sub(offset, Ordering::AcqRel)
Copy link
Member

Choose a reason for hiding this comment

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

Why AcqRel instead of SeqCst?

Copy link
Member Author

Choose a reason for hiding this comment

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

Using the weakest ordering that does the job. In this case it doesn't matter a huge amount, this isn't hot code.

debug!("Finished adding script.");
let old_counter = pending_tasks_struct.decrement_counter_by(1);
if old_counter == 1 {
// TODO: trigger a reflow?
Copy link
Member

Choose a reason for hiding this comment

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

This happens automatically when events run in the script thread. Did you mean something more specific?

Copy link
Member Author

Choose a reason for hiding this comment

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

At this point, we have loaded a worklet, but not actually run it (e.g. to get a paint worklet to do its painting).

}

/// Fetch and invoke a worklet script.
///g https://drafts.css-houdini.org/worklets/#fetch-and-invoke-a-worklet-script
Copy link
Member

Choose a reason for hiding this comment

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

nit: g

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops.

@jdm
Copy link
Member

jdm commented May 16, 2017

As usual, I found these changes to be a pleasant and straightforward read. Good job!

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 16, 2017
type_: RequestType::Script,
destination: Destination::Script,
credentials_mode: credentials.into(),
.. RequestInit::default()
Copy link
Member

Choose a reason for hiding this comment

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

We should be using CORS mode here, per https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-single-module-script. We'll need to include a real origin, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

CORS mode isn't a problem, but I'm not sure which origin to use here. One snag is that this is being done in a different thread, so we don't have access to the MutableOrigin of the window.

Copy link
Member

Choose a reason for hiding this comment

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

Send an ImmutableOrigin from the window as part of the initiation?

Copy link
Member Author

@asajeffrey asajeffrey left a comment

Choose a reason for hiding this comment

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

Thanks for the review!

// https://drafts.css-houdini.org/worklets/#examples
partial interface Window {
[Pref="dom.worklet.testing.enabled", SameObject] readonly attribute Worklet testWorklet;
[Pref="dom.worklet.testing.enabled"] DOMString? testWorkletLookup(DOMString key);
Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

pub mem_profiler_chan: mem::ProfilerChan,
/// Chan to the time profiler
pub time_profiler_chan: time::ProfilerChan,
/// Chan to devtools
Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

use style::thread_state;
use swapper::Swapper;
use swapper::swapper;
use uuid::Uuid;
Copy link
Member Author

Choose a reason for hiding this comment

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

Minimizing the chance of annoying merge conflicts.

let global = window.upcast::<GlobalScope>();
unsafe {
WorkletBinding::Wrap(global.get_cx(), global, worklet)
}
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 don't think you can use reflect_dom_object to create globals can you?

self.worklet_id
}

#[allow(dead_code)]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is dead code, just in for future-proofing. I can get rid of it.

#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, JSTraceable)]
pub struct WorkletId(Uuid);

known_heap_size!(0, WorkletId);
Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

}

fn decrement_counter_by(&self, offset: isize) -> isize {
self.0.fetch_sub(offset, Ordering::AcqRel)
Copy link
Member Author

Choose a reason for hiding this comment

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

Using the weakest ordering that does the job. In this case it doesn't matter a huge amount, this isn't hot code.

}

/// Fetch and invoke a worklet script.
///g https://drafts.css-houdini.org/worklets/#fetch-and-invoke-a-worklet-script
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops.

type_: RequestType::Script,
destination: Destination::Script,
credentials_mode: credentials.into(),
.. RequestInit::default()
Copy link
Member Author

Choose a reason for hiding this comment

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

CORS mode isn't a problem, but I'm not sure which origin to use here. One snag is that this is being done in a different thread, so we don't have access to the MutableOrigin of the window.

debug!("Finished adding script.");
let old_counter = pending_tasks_struct.decrement_counter_by(1);
if old_counter == 1 {
// TODO: trigger a reflow?
Copy link
Member Author

Choose a reason for hiding this comment

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

At this point, we have loaded a worklet, but not actually run it (e.g. to get a paint worklet to do its painting).

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels May 16, 2017
@jdm jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels May 16, 2017
@jdm
Copy link
Member

jdm commented May 16, 2017

Squash and r=me.

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-needs-squash Some (or all) of the commits in the PR should be combined. labels May 16, 2017
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels May 16, 2017
@asajeffrey
Copy link
Member Author

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

📌 Commit de96995 has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 16, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit de96995 with merge d8f4b59...

bors-servo pushed a commit that referenced this pull request May 17, 2017
Implement Houdini worklets

<!-- Please describe your changes on the following line: -->

This PR implements the current draft Houdini Worklets specification (https://drafts.css-houdini.org/worklets/).

The implementation is intended to provide a responsive environment for worklet execution, and in particular to ensure that the primary worklet executor does not garbage collect, and does not block loading module code. The implementation does this by providing a thread pool, and performing GC and module loading in a backup thread, not in the primary thread.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #16206
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16814)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels May 17, 2017
@jdm
Copy link
Member

jdm commented May 17, 2017

  ▶ Unexpected subtest result in /_mozilla/mozilla/interfaces.html:
  │ FAIL [expected PASS] Interfaces exposed on the window
  │   → assert_true: If this is failing: DANGER, are you sure you want to expose the new interface Worklet to all webpages as a property on the global? Do not make a change to this file without review from jdm or Ms2ger for that specific change! expected true got false
  │ 
  │ test_interfaces/<@http://web-platform.test:8000/_mozilla/mozilla/interfaces.js:73:7
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:497:9
  │ test_interfaces@http://web-platform.test:8000/_mozilla/mozilla/interfaces.js:2:3
  └ @http://web-platform.test:8000/_mozilla/mozilla/interfaces.html:13:1

We can either hide the Worklet interface behind an off-by-default pref or update the test to include the interface in the expected list.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels May 17, 2017
@asajeffrey
Copy link
Member Author

I added Worklet to the interfaces test. @bors-servo r=jdm

@bors-servo
Copy link
Contributor

📌 Commit af8436c has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 17, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit af8436c with merge ac99a48...

bors-servo pushed a commit that referenced this pull request May 17, 2017
Implement Houdini worklets

<!-- Please describe your changes on the following line: -->

This PR implements the current draft Houdini Worklets specification (https://drafts.css-houdini.org/worklets/).

The implementation is intended to provide a responsive environment for worklet execution, and in particular to ensure that the primary worklet executor does not garbage collect, and does not block loading module code. The implementation does this by providing a thread pool, and performing GC and module loading in a backup thread, not in the primary thread.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #16206
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16814)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: jdm
Pushing ac99a48 to master...

@bors-servo bors-servo merged commit af8436c into servo:master May 17, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 17, 2017
@bors-servo bors-servo mentioned this pull request May 17, 2017
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/script Related to the script thread
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement CSS Houdini Worklets
7 participants