Using OnceCell<T> from Mitochondria #18059

sendilkumarn · 2017-08-13T06:32:45Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13402 (github issue number if applicable).

This change is

highfive · 2017-08-13T06:32:50Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/Cargo.toml, components/script/lib.rs, components/script/dom/bindings/js.rs
@KiChjang: components/script/Cargo.toml, components/script/lib.rs, components/script/dom/bindings/js.rs

highfive · 2017-08-13T06:32:52Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

nox · 2017-08-13T10:00:09Z

Checking files for tidiness...
./components/script/dom/bindings/js.rs:395: trailing whitespace
./components/script/dom/bindings/js.rs:442: trailing whitespace
./components/script/dom/bindings/js.rs:462: trailing whitespace
./components/script/dom/bindings/js.rs:496: trailing whitespace
./components/script/dom/bindings/js.rs:521: trailing whitespace
./components/script/dom/bindings/js.rs:759: no newline at EOF
./components/script/dom/bindings/js.rs:462: extra space after (

nox · 2017-08-13T10:06:10Z

components/script/dom/bindings/js.rs

+/// on `JS<T>`.
+#[must_root]
+pub struct OnceCellJS<T: DomObject> {
+    ptr: OnceCell<Option<JS<T>>>,


There should be no Option<T> here.

nox · 2017-08-13T10:06:10Z

components/script/dom/bindings/js.rs

+
+    /// Retrieve a copy of the current inner value. If it is `None`, it is
+    /// initialized with the result of `cb` first.
+    pub fn or_init<F>(&self, cb: F) -> Root<T>


This should call OnceCell::init_once.

nox · 2017-08-13T10:06:10Z

components/script/dom/bindings/js.rs

+        debug_assert!(thread_state::get().is_layout());
+        match self.ptr.as_ref() {
+            Some(ptr_ref) => {
+                ptr::read(ptr_ref).map(|js| js.to_layout())


Why ptr::read?

nox · 2017-08-13T10:06:10Z

components/script/dom/bindings/js.rs

+        unsafe {
+            match self.ptr.as_ref() {
+                Some(ptr_ref) => {
+                    ptr::read(ptr_ref).map(|o| Root::from_ref(&*o))


Why ptr::read?

nox · 2017-08-13T10:06:10Z

components/script/dom/bindings/js.rs

+    pub fn get(&self) -> Option<Root<T>> {
+        debug_assert!(thread_state::get().is_script());
+        unsafe {
+            match self.ptr.as_ref() {


Use Option::map instead of a match expression.

nox · 2017-08-13T10:06:10Z

components/script/dom/bindings/js.rs

+    }
+
+    /// Set this `OnceCellJS` to the given value. If it is not already set
+    pub fn set(&self, val: Option<&T>) {


This method shouldn't exist at all.

using init_once straight away in or_init method causes the following error

expected type `core::option::Option<T>` found type `core::option::Option<&dom::bindings::js::JS<T>>`

and if I pass it to this method it kinda works.

This method shouldn't exist anyway.

nox · 2017-08-13T10:06:10Z

components/script/dom/bindings/js.rs

+    }
+
+    /// Gets the current value out of this object and sets it to `None`.
+    pub fn take(&self) -> Option<Root<T>> {


This method shouldn't exist at all.

nox · 2017-08-13T10:06:10Z

components/script/dom/bindings/js.rs

+    }
+}
+
+impl<T: DomObject> PartialEq for OnceCellJS<T> {


This should be derived.

nox · 2017-08-13T10:06:10Z

components/script/dom/bindings/js.rs

+        #[cfg(not(debug_assertions))]
+        let trace_info = "for DOM object on heap";
+
+        match self.ptr.as_ref().clone() {


Just call trace on the result of self.ptr.as_ref().

nox · 2017-08-13T10:06:10Z

components/script/dom/bindings/js.rs

@@ -603,4 +756,4 @@ unsafe impl<T: DomObject> JSTraceable for Root<T> {
    unsafe fn trace(&self, _: *mut JSTracer) {
        // Already traced.
    }
-}
+}


Nit: missing new line.

KiChjang · 2017-08-14T21:20:10Z

components/script/dom/bindings/js.rs

+
+    /// Get a rooted value out of this object
+    #[allow(unrooted_must_root)]
+    pub fn get(&self) -> Option<Root<T>> {


I don't think this is the right return type. The whole idea behind the issue is that whenever I call the get method, I shouldn't need to worry about whether the cell is empty or not, and that it will always get the appropriate value for me.

That makes sense. Since MutNullableJS and MutJS returns Root<T> , I decided to return something similar. Rather here I will return Option<JS<T>>

No, Option<JS<T>> shouldn't be returned, but Option<&T>.

KiChjang · 2017-08-15T09:22:32Z

components/script/dom/bindings/js.rs

+    /// Retrieve a copy of the current inner value. If it is `None`, it is
+    /// initialized with the result of `cb` first.
+    #[allow(unrooted_must_root)]
+    pub unsafe fn or_init<F>(&self, cb: F) -> &JS<T>


I'm still not convinced this is an improvement over MutNullableJS<T>. In fact as it stands currently, it is an exact copy of it. Let me reiterate the goals of the issue:

Consider the Elements method in HTMLFormElement. It tries to see if the elements field is a Some; if so, it would return that inner value, and if not, it does a whole chunk of initialization for it, and finally setting that inner value into Some.

What I personally would like to see is to have that method body be reduced into simply self.elements.get(), and perhaps in the new method of the OnceCellJS that houses the list of elements, accept a closure/function that gets called whenever the get method encounters a None for its inner value.

This should return &T. get should either not exist or return Option<&T>.

@KiChjang thanks

@nox yeah it should return &T. We can remove the get too use or_init straightaway

nox · 2017-08-16T12:22:43Z

My comments should be addressed, and then the commits should be squashed together.

Also, we can't land this as long as you don't also migrate at least one MutNullableJS to that new type, finally making use of it.

nox · 2017-08-16T12:22:54Z

components/script/dom/bindings/js.rs

+#[allow(unrooted_must_root)]
+unsafe impl<T: DomObject> JSTraceable for OnceCellJS<T> {
+    unsafe fn trace(&self, trc: *mut JSTracer) {
+        match self.ptr.as_ref() {


if let Some(ptr) = self.ptr.as_ref() { ptr.trace(trc); }

nox · 2017-08-16T12:22:54Z

components/script/dom/bindings/js.rs

+    /// Retrieve a copy of the current inner value. If it is `None`, it is
+    /// initialized with the result of `cb` first.
+    #[allow(unrooted_must_root)]
+    pub unsafe fn or_init<F>(&self, cb: F) -> &T


This is still uncorrect.

It should not be unsafe.

F should be FnOnce() -> Root<T>.

The method should only call init_once with a wrapper of cb converting the Root<T> to JS<T> and dereferencing the returned JS<T> to &T.

I'm still confused as to why this function exists. How is this any different than MutNullableJS?

You cannot retrieve a &T from a MutNullableJS<T>. IMO the method should be named init_once just like in Mitochondria.

nox · 2017-08-16T12:22:54Z

components/script/dom/bindings/js.rs

+
+impl<T: DomObject> OnceCellJS<T> {
+    /// Create a new `OnceCellJS` with value
+    pub fn new(initial: Option<&T>) -> OnceCellJS<T> {


I'm pretty sure there is no need for an initial value at all here.

which makes this function similar to Default::default() Instead, we can change the signature to new_with_value similar to mitochondria

I don't really seem to see that use case at all... Why would you need to provide an initial value when the purpose is to either get it from the OnceCell or run a closure to produce a value?

nox · 2017-08-21T09:12:32Z

components/script/dom/bindings/js.rs

+
+impl<T: DomObject> OnceCellJS<T> {
+    /// Create a new `OnceCellJS` with value
+    pub fn new() -> OnceCellJS<T> {


Make that the impl of Default for OnceCellJS<T> and this is good to go!

Thanks for keeping up with my comments!

You mean change this method into default or implement default separately.

Implement the Default trait on OnceCellJS, and move the method body below to be inside of fn default.

nox · 2017-09-07T11:58:28Z

Am stupid, the PR should be rebased to land. ><

bors-servo · 2017-09-08T01:35:31Z

🔒 Merge conflict

KiChjang · 2017-09-08T19:38:38Z

components/script/dom/htmlformelement.rs

+            let filter = box ElementsFilter { form: Root::from_ref(self) };
+            let window = window_from_node(self);
+            let elements = HTMLFormControlsCollection::new(&window, self.upcast(), filter);
+            elements


Can the elements variable be removed here?


        moving to oncecell

adding oncecell for JS references removing option<JS<T>> to <JS<T>> changing return types removing get method and refactoring the function changing getElements method lint fixes moving to default simplifying return ordering Removing elements linting

sendilkumarn · 2017-09-16T17:37:38Z

@nox is it pending for any other thing?

KiChjang · 2017-09-16T17:49:17Z

@bors-servo r=nox

bors-servo · 2017-09-16T17:49:17Z

📌 Commit 8b33e0f has been approved by nox

bors-servo · 2017-09-16T17:49:18Z

🌲 The tree is currently closed for pull requests below priority 9000, this pull request will be tested once the tree is reopened

bors-servo · 2017-09-17T03:33:54Z

⌛ Testing commit 8b33e0f with merge b42a2b2918e79820c4b228bfa88cd8cf37eedfe4...

bors-servo · 2017-09-17T04:17:49Z

💔 Test failed - linux-rel-wpt

bors-servo · 2017-09-21T18:20:31Z

⌛ Testing commit 8b33e0f with merge 5c797d1...


        Auto merge of #18059 - sendilkumarn:mito, r=nox

Using OnceCell<T> from Mitochondria  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13402 (github issue number if applicable).    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18059)

bors-servo · 2017-09-21T19:55:19Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: nox
Pushing 5c797d1 to master...

highfive assigned wafflespeanut Aug 13, 2017

highfive added the S-awaiting-review label Aug 13, 2017

sendilkumarn changed the title ~~Mito~~ using oncecell in Mitochondria Aug 13, 2017

nox changed the title ~~using oncecell in Mitochondria~~ using OnceCell<T> in Mitochondria Aug 13, 2017

nox changed the title ~~using OnceCell<T> in Mitochondria~~ Using OnceCell<T> in Mitochondria Aug 13, 2017

nox added the S-fails-tidy label Aug 13, 2017

nox requested changes Aug 13, 2017

View changes

nox assigned nox and unassigned wafflespeanut Aug 13, 2017

nox added S-needs-code-changes and removed S-awaiting-review labels Aug 13, 2017

KiChjang reviewed Aug 14, 2017

View changes

sendilkumarn changed the title ~~Using OnceCell<T> in Mitochondria~~ Using OnceCell<T> from Mitochondria Aug 15, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 15, 2017

KiChjang reviewed Aug 15, 2017

View changes

nox added S-needs-squash and removed S-fails-tidy labels Aug 16, 2017

nox requested changes Aug 16, 2017

View changes

nox added S-needs-code-changes and removed S-awaiting-review labels Aug 16, 2017

sendilkumarn force-pushed the sendilkumarn:mito branch from 8eb6069 to 511d833 Aug 18, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 18, 2017

nox reviewed Aug 21, 2017

View changes

nox added the S-needs-code-changes label Aug 21, 2017

nox added S-needs-rebase and removed S-awaiting-merge labels Sep 7, 2017

sendilkumarn force-pushed the sendilkumarn:mito branch from fe7a5fb to 35bbe5e Sep 8, 2017

highfive added the S-awaiting-review label Sep 8, 2017

sendilkumarn force-pushed the sendilkumarn:mito branch from 35bbe5e to b16a209 Sep 8, 2017

KiChjang reviewed Sep 8, 2017

View changes

sendilkumarn force-pushed the sendilkumarn:mito branch from b16a209 to 8b33e0f Sep 9, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Sep 16, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 17, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 21, 2017

bors-servo merged commit 8b33e0f into servo:master Sep 21, 2017
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 21, 2017

bors-servo mentioned this pull request Sep 21, 2017

Make dictionaries and unions containing GC values safer #17056

Merged

3 of 4 tasks complete

servo / servo

Join GitHub today

Using OnceCell<T> from Mitochondria #18059

Using OnceCell<T> from Mitochondria #18059

Conversation

sendilkumarn commented Aug 13, 2017 • edited

highfive commented Aug 13, 2017

highfive commented Aug 13, 2017

nox commented Aug 13, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented Aug 16, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented Sep 7, 2017

bors-servo commented Sep 8, 2017

This comment has been minimized.

sendilkumarn commented Sep 16, 2017

KiChjang commented Sep 16, 2017

bors-servo commented Sep 16, 2017

bors-servo commented Sep 16, 2017

bors-servo commented Sep 17, 2017

bors-servo commented Sep 17, 2017

bors-servo commented Sep 21, 2017

bors-servo commented Sep 21, 2017

sendilkumarn commented Aug 13, 2017 •

edited