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 Investigate a Lazy<T> DOM member type instead of MutNullableHeap<T> #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:00:49Z

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:01:50Z

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:02:56Z

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:03:16Z

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:03:36Z

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:04:22Z

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:04:30Z

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:04:43Z

components/script/dom/bindings/js.rs

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


This should be derived.

nox · 2017-08-13T10:05:54Z

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:05Z

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:25Z

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:19:31Z

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:21:41Z

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:00Z

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:27Z

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?

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

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 There is new code that needs to be reviewed. 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 `./mach test-tidy` reported errors. label Aug 13, 2017

nox suggested changes Aug 13, 2017

View reviewed changes

nox assigned nox and unassigned wafflespeanut Aug 13, 2017

nox 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 Aug 13, 2017

KiChjang reviewed Aug 14, 2017

View reviewed changes

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

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 Aug 15, 2017

KiChjang previously requested changes Aug 15, 2017

View reviewed changes

nox added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-fails-tidy `./mach test-tidy` reported errors. labels Aug 16, 2017

nox suggested changes Aug 16, 2017

View reviewed changes

nox 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 Aug 16, 2017

sendilkumarn force-pushed the mito branch from 8eb6069 to 511d833 Compare August 18, 2017 11:06

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 Aug 18, 2017

nox reviewed Aug 21, 2017

View reviewed changes

nox added the S-needs-code-changes Changes have not yet been made that were requested by a reviewer. label Aug 21, 2017

nox added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Sep 7, 2017

sendilkumarn force-pushed the mito branch from fe7a5fb to 35bbe5e Compare September 8, 2017 04:50

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 8, 2017

sendilkumarn force-pushed the mito branch from 35bbe5e to b16a209 Compare September 8, 2017 07:31

KiChjang reviewed Sep 8, 2017

View reviewed changes

moving to oncecell

8b33e0f

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 force-pushed the mito branch from b16a209 to 8b33e0f Compare September 9, 2017 00:27

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. S-needs-rebase There are merge conflict errors. labels Sep 16, 2017

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 Sep 17, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Sep 21, 2017

bors-servo merged commit 8b33e0f into servo:master Sep 21, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Sep 21, 2017

bors-servo mentioned this pull request Sep 21, 2017

Make dictionaries and unions containing GC values safer #17056

Merged

4 tasks

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nox commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nox commented Sep 7, 2017

bors-servo commented Sep 8, 2017

Choose a reason for hiding this comment

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