Reimplement CustomAutoRooter hierarchy #382

Xanewok · 2017-11-27T01:48:56Z

This is an attempt at servo/servo#16678. It doesn't yet contain conversion code that's described here: servo/servo#16678 (comment).

I wanted to see if the design is somewhat sound and ergonomic. Tried to make CustomAutoRooter a convenient generic type that could support tracing arbitrary types, just like C++ equivalent does, albeit with generics, since we can't just use C++ inheritance.

Here SequenceRooter<T> is implemented as a simple alias for CustomAutoRooter<Vec<T>>.

cc @jdm

This change is

jdm

Nice work!

jdm · 2017-11-27T19:55:31Z

src/rust.rs

@@ -463,6 +464,185 @@ macro_rules! rooted {
    }
 }

+use jsapi::AutoGCRooter_jspubtd_h_unnamed_1 as AutoGCRooterTag;


Let's move this up with the other use statements.

jdm · 2017-11-27T19:55:53Z

src/rust.rs

+        }
+    }
+
+    pub unsafe fn add_to_root_stack(&mut self, cx: *mut JSContext) {


This doesn't need to be public.

jdm · 2017-11-27T19:56:20Z

src/rust.rs

+    }
+
+    pub unsafe fn add_to_root_stack(&mut self, cx: *mut JSContext) {
+        let autoGCRooters: &mut _ = {


Is the &mut _ here necessary?

Yes, because we need a pointer to the autoGCRooters_ stack list itself (which is also represented as a pointer).
The stackTop member is initialized with it in C++: https://dxr.mozilla.org/mozilla-central/source/js/public/RootingAPI.h#839.

jdm · 2017-11-27T19:56:38Z

src/rust.rs

+        *self.stackTop = self;
+    }
+
+    pub unsafe fn remove_from_root_stack(&mut self) {


This doesn't need to be public.

jdm · 2017-11-27T19:57:31Z

src/rust.rs

+}
+
+impl<T> CustomAutoRooter<T> {
+    pub unsafe fn add_to_root_stack(&mut self, cx: *mut JSContext) {


These don't need to be public.

jdm · 2017-11-27T20:12:38Z

src/rust.rs

+pub type SequenceRooter<T> = CustomAutoRooter<Vec<T>>;
+
+/// TODO: Document me (similar to Rooted)
+pub struct SequenceRooterGuard<'a, T: 'a + RootKind + GCMethods> {


For some of the work that the rest of your group is doing, we will want to make this more general. I'm fine if you want to make this follow-up work.

Since there is nothing special about sequences here, let's make this more general: RooterGuard<T> where T: StackRoot (which can be a new type that provides add_to_root_stack and remove_from_stack, as well as a way to access the contained data).

jdm · 2017-11-27T20:13:18Z

tests/sequence_rooter.rs

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#![cfg(feature = "debugmozjs")]


Let's remove this so the test is always run.

Ah, JS_SetGCZeal needed that and I thought I stumbled upon this also with JS_GC, but that's not the case and it's not needed here. Will do, thanks!

jdm · 2017-11-27T20:14:02Z

tests/sequence_rooter.rs

+
+    unsafe impl CustomTrace for TestStruct {
+        fn trace(&self, _: *mut JSTracer) {
+            unsafe { TRACE_FN_WAS_CALLED = true; }


We could store a Cell<bool> inside CustomTrace instead of using static mut for this.

As we talked, #[cfg(test)] code is not compiled in a library target that's used by a Cargo integration test, so we can't modify the CustomTrace trait here.

I can extract the boilerplate code to a macro that declares the static variable and the impl block, if that works.

Whoops, I meant to says TestStruct instead of CustomTrace.

jdm · 2017-11-27T20:14:39Z

tests/sequence_rooter.rs

+
+    unsafe impl CustomTrace for TestStruct {
+        fn trace(&self, _: *mut JSTracer) {
+            unsafe { TRACE_FN_WAS_CALLED = true; }


Same as previous comment.

jdm · 2017-11-27T20:16:27Z

tests/sequence_rooter.rs

+
+        rooted.add_to_root_stack(rt.cx());
+        JS_GC(rt.rt());
+        rooted.remove_from_root_stack();


This test would benefit from the generalization I suggested earlier.

Xanewok · 2017-11-28T02:30:46Z

Addressed feedback and pushed related changes.

Xanewok · 2017-11-29T00:26:59Z

@jdm pushed a commit that uses Cell instead of static mut variable. Could you take another look at it and tell me if this needs more work still?

jdm

This looks great!

jdm · 2017-11-29T13:12:43Z

src/rust.rs

+        let $name = __root.root($cx);
+    };
+    (in($cx:expr) let mut $name:ident = $init:expr) => {
+        let mut __root = $crate::rust::SequenceRooter::new($init);


One of these is CustomAutoRooter, and one is SequenceRooter. That's surprising!

Oops! Fixed that now, renamed the macro to a more general auto_root name, to be used with CustomAutoRooter itself.

jdm · 2017-11-29T18:10:00Z

@bors-servo: r+
Thanks!

bors-servo · 2017-11-29T18:10:01Z

📌 Commit 03b62d3 has been approved by jdm

bors-servo · 2017-11-29T18:10:04Z

⌛ Testing commit 03b62d3 with merge bc49851...

@jdm

Reimplement CustomAutoRooter hierarchy This is an attempt at servo/servo#16678. It doesn't yet contain conversion code that's described here: servo/servo#16678 (comment). I wanted to see if the design is somewhat sound and ergonomic. Tried to make CustomAutoRooter a convenient generic type that could support tracing arbitrary types, just like C++ equivalent does, albeit with generics, since we can't just use C++ inheritance. Here `SequenceRooter<T>` is implemented as a simple [alias](https://github.com/Xanewok/rust-mozjs/blob/custom-auto-rooter/src/rust.rs#L592) for `CustomAutoRooter<Vec<T>>`. cc @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/382)

bors-servo · 2017-11-29T18:34:31Z

💔 Test failed - status-appveyor

jdm · 2017-11-29T18:35:49Z

Ah, having both new tests in the same file is hitting the usual error.

thread 'root_macro' panicked at 'called `Result::unwrap()` on an `Err` value: ()', src\libcore\result.rs:906:4

Xanewok · 2017-11-29T19:26:50Z

Darn it, I tried to reproduce it and ran cargo test ~2000 times on Ubuntu 16.10 on i7-3770K, but couldn't reproduce it in the end.
@jdm do you think we can proceed with this and work in the meantime on bulletproofing the testing infra? Maybe we can do a test run with cargo test -- --test-threads=1 on CI to see if that helps?

jdm · 2017-11-29T19:51:01Z

If you move the second new test into a separate file, it should avoid the problem entirely.

Xanewok · 2017-11-29T20:04:25Z

Done. However I'm not sure if that's a good idea moving forward, since linking and producing separate binary for each test takes considerable amount of time, so if each test doesn't need a completely separate process, it'd probably be faster and easier to maintain to run tests using only a single thread instead in fewer test binaries.

jdm · 2017-11-29T20:08:32Z

@bors-servo: r+

bors-servo · 2017-11-29T20:08:33Z

📌 Commit d88d93a has been approved by jdm

bors-servo · 2017-11-29T20:08:35Z

⌛ Testing commit d88d93a with merge 0551c14...

@jdm

Reimplement CustomAutoRooter hierarchy This is an attempt at servo/servo#16678. It doesn't yet contain conversion code that's described here: servo/servo#16678 (comment). I wanted to see if the design is somewhat sound and ergonomic. Tried to make CustomAutoRooter a convenient generic type that could support tracing arbitrary types, just like C++ equivalent does, albeit with generics, since we can't just use C++ inheritance. Here `SequenceRooter<T>` is implemented as a simple [alias](https://github.com/Xanewok/rust-mozjs/blob/custom-auto-rooter/src/rust.rs#L592) for `CustomAutoRooter<Vec<T>>`. cc @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/382)

bors-servo · 2017-11-30T01:42:05Z

💔 Test failed - status-travis

Xanewok · 2017-11-30T12:42:39Z

Same as #381 (comment), Travis couldn't start macOS debugmozjs job for a long time and when it did, it couldn't set up the environment:

error: toolchain 'nightly' is not installed
info: caused by: not a directory: '/Users/travis/.rustup/toolchains/nightly-x86_64-apple-darwin'

https://travis-ci.org/servo/rust-mozjs/jobs/309158923

bors-servo · 2017-12-01T06:37:17Z

☀️ Test successful - status-appveyor, status-travis
Approved by: jdm
Pushing 0551c14 to master...

Root sequence<any> params using CustomAutoRooter  Attempt at #16678. At the moment this pulls an external [remove-handle-conv](https://github.com/Xanewok/rust-mozjs/tree/remove-handle-conv) branch for rust-mozjs (removing `FromJSValConvertible` implementation for `HandleValue` and adding one for `*mut JSObject`) In short, this roots the `sequence<any>` and `sequence<object>` function arguments using `CustomAutoRooter` (introduced in servo/rust-mozjs#382). As per #16678 (comment) the underlying vector contains raw gc thing pointers instead of handles. To do that, this introduces new possible `isMember = "AutoRoot"` value in CodegenRust.py. The rooted argument is passed to a function wrapped in a `CustomAutoRooterGuard` [RAII root guard](https://github.com/servo/rust-mozjs/blob/ed5e37a288b5738d9b571b8100b4a22a2c00f075/src/rust.rs#L586-L588) (e.g. as `CustomAutoRooterGuard<Vec<JSVal>>`) I felt quite out of my depth when trying to adapt CodegenRust.py code, so I'd appreciate any help with how can be done better/in a more clean manner. Also the name `CustomAutoRooterGuard` is quite lengthy, so I'm also open to changing it (`AutoRoot`? `AutoRootGuard`?) ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #16678 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19644)

…r (from Xanewok:root-seq-any); r=jdm  Attempt at servo/servo#16678. At the moment this pulls an external [remove-handle-conv](https://github.com/Xanewok/rust-mozjs/tree/remove-handle-conv) branch for rust-mozjs (removing `FromJSValConvertible` implementation for `HandleValue` and adding one for `*mut JSObject`) In short, this roots the `sequence<any>` and `sequence<object>` function arguments using `CustomAutoRooter` (introduced in servo/rust-mozjs#382). As per servo/servo#16678 (comment) the underlying vector contains raw gc thing pointers instead of handles. To do that, this introduces new possible `isMember = "AutoRoot"` value in CodegenRust.py. The rooted argument is passed to a function wrapped in a `CustomAutoRooterGuard` [RAII root guard](https://github.com/servo/rust-mozjs/blob/ed5e37a288b5738d9b571b8100b4a22a2c00f075/src/rust.rs#L586-L588) (e.g. as `CustomAutoRooterGuard<Vec<JSVal>>`) I felt quite out of my depth when trying to adapt CodegenRust.py code, so I'd appreciate any help with how can be done better/in a more clean manner. Also the name `CustomAutoRooterGuard` is quite lengthy, so I'm also open to changing it (`AutoRoot`? `AutoRootGuard`?) ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #16678 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   Source-Repo: https://github.com/servo/servo Source-Revision: 989d2fd53267d063212ef3b7b7f13f2402943c4a --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : 1d2081536bfa13c32a50b4862abbe3a03f305da3

…r (from Xanewok:root-seq-any); r=jdm  Attempt at servo/servo#16678. At the moment this pulls an external [remove-handle-conv](https://github.com/Xanewok/rust-mozjs/tree/remove-handle-conv) branch for rust-mozjs (removing `FromJSValConvertible` implementation for `HandleValue` and adding one for `*mut JSObject`) In short, this roots the `sequence<any>` and `sequence<object>` function arguments using `CustomAutoRooter` (introduced in servo/rust-mozjs#382). As per servo/servo#16678 (comment) the underlying vector contains raw gc thing pointers instead of handles. To do that, this introduces new possible `isMember = "AutoRoot"` value in CodegenRust.py. The rooted argument is passed to a function wrapped in a `CustomAutoRooterGuard` [RAII root guard](https://github.com/servo/rust-mozjs/blob/ed5e37a288b5738d9b571b8100b4a22a2c00f075/src/rust.rs#L586-L588) (e.g. as `CustomAutoRooterGuard<Vec<JSVal>>`) I felt quite out of my depth when trying to adapt CodegenRust.py code, so I'd appreciate any help with how can be done better/in a more clean manner. Also the name `CustomAutoRooterGuard` is quite lengthy, so I'm also open to changing it (`AutoRoot`? `AutoRootGuard`?) ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #16678 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   Source-Repo: https://github.com/servo/servo Source-Revision: 989d2fd53267d063212ef3b7b7f13f2402943c4a

…r (from Xanewok:root-seq-any); r=jdm  Attempt at servo/servo#16678. At the moment this pulls an external [remove-handle-conv](https://github.com/Xanewok/rust-mozjs/tree/remove-handle-conv) branch for rust-mozjs (removing `FromJSValConvertible` implementation for `HandleValue` and adding one for `*mut JSObject`) In short, this roots the `sequence<any>` and `sequence<object>` function arguments using `CustomAutoRooter` (introduced in servo/rust-mozjs#382). As per servo/servo#16678 (comment) the underlying vector contains raw gc thing pointers instead of handles. To do that, this introduces new possible `isMember = "AutoRoot"` value in CodegenRust.py. The rooted argument is passed to a function wrapped in a `CustomAutoRooterGuard` [RAII root guard](https://github.com/servo/rust-mozjs/blob/ed5e37a288b5738d9b571b8100b4a22a2c00f075/src/rust.rs#L586-L588) (e.g. as `CustomAutoRooterGuard<Vec<JSVal>>`) I felt quite out of my depth when trying to adapt CodegenRust.py code, so I'd appreciate any help with how can be done better/in a more clean manner. Also the name `CustomAutoRooterGuard` is quite lengthy, so I'm also open to changing it (`AutoRoot`? `AutoRootGuard`?) ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #16678 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   Source-Repo: https://github.com/servo/servo Source-Revision: 989d2fd53267d063212ef3b7b7f13f2402943c4a UltraBlame original commit: cdc2693ba485669c0fbd4e63de6a394b6cfca8a0

Xanewok added 6 commits November 27, 2017 19:48

Implement CustomAutoRooter (incl. SequenceRooter)

869cf12

WIP: SequenceRooter tests

7a88fb9

Simplify AutoGCRooter pointer casts

e0769ce

Fix bugs

516a980

Lift Default trait bound from CustomAutoRooter<T>

204383e

Add virtual dispatch tests

bc11ffb

Xanewok force-pushed the custom-auto-rooter branch from 5cd0d31 to bc11ffb Compare November 27, 2017 18:50

taki-jaro mentioned this pull request Nov 27, 2017

Add codegen support for object types in unions servo/servo#9759

Closed

jdm suggested changes Nov 27, 2017

View reviewed changes

Address feedback, make CustomAutoRooter more general

9d1c4b8

Xanewok force-pushed the custom-auto-rooter branch from 727d3f8 to 9d1c4b8 Compare November 28, 2017 02:28

Use Cell in custom_auto_rooter tests

90e7b5d

jdm suggested changes Nov 29, 2017

View reviewed changes

Fix CustomAutoRooter macro

03b62d3

Split tests into separate files

d88d93a

bors-servo merged commit d88d93a into servo:master Dec 1, 2017

This was referenced Dec 1, 2017

Infer integration tests under tests/ #386

Open

Safe, straightforward API for capturing JS stacks #381

Merged

Xanewok deleted the custom-auto-rooter branch December 1, 2017 07:00

Xanewok mentioned this pull request Dec 25, 2017

Root sequence<any> params using CustomAutoRooter servo/servo#19644

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement CustomAutoRooter hierarchy #382

Reimplement CustomAutoRooter hierarchy #382

Xanewok commented Nov 27, 2017 •

edited by larsbergstrom

jdm left a comment

jdm Nov 27, 2017

jdm Nov 27, 2017

jdm Nov 27, 2017

Xanewok Nov 27, 2017

jdm Nov 27, 2017

jdm Nov 27, 2017

jdm Nov 27, 2017

jdm Nov 27, 2017

Xanewok Nov 27, 2017

jdm Nov 27, 2017

Xanewok Nov 28, 2017

jdm Nov 28, 2017

jdm Nov 27, 2017

jdm Nov 27, 2017

Xanewok commented Nov 28, 2017

Xanewok commented Nov 29, 2017

jdm left a comment

jdm Nov 29, 2017

Xanewok Nov 29, 2017

jdm commented Nov 29, 2017

bors-servo commented Nov 29, 2017

bors-servo commented Nov 29, 2017

bors-servo commented Nov 29, 2017

jdm commented Nov 29, 2017

Xanewok commented Nov 29, 2017

jdm commented Nov 29, 2017

Xanewok commented Nov 29, 2017

jdm commented Nov 29, 2017

bors-servo commented Nov 29, 2017

bors-servo commented Nov 29, 2017

bors-servo commented Nov 30, 2017

Xanewok commented Nov 30, 2017

bors-servo commented Dec 1, 2017

Reimplement CustomAutoRooter hierarchy #382

Reimplement CustomAutoRooter hierarchy #382

Conversation

Xanewok commented Nov 27, 2017 • edited by larsbergstrom

jdm left a 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

Xanewok commented Nov 28, 2017

Xanewok commented Nov 29, 2017

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Nov 29, 2017

bors-servo commented Nov 29, 2017

bors-servo commented Nov 29, 2017

bors-servo commented Nov 29, 2017

jdm commented Nov 29, 2017

Xanewok commented Nov 29, 2017

jdm commented Nov 29, 2017

Xanewok commented Nov 29, 2017

jdm commented Nov 29, 2017

bors-servo commented Nov 29, 2017

bors-servo commented Nov 29, 2017

bors-servo commented Nov 30, 2017

Xanewok commented Nov 30, 2017

bors-servo commented Dec 1, 2017

Xanewok commented Nov 27, 2017 •

edited by larsbergstrom