Don't use unsafe Heap::new constructor

[Xanewok]

Pulls servo/rust-mozjs#398 and aims to close servo/rust-mozjs#343.

We can't convert from JSVal to Heap<JSVal> safely (due to GC barriers we can't move Heap value after changing its underlying value to something meaningful, e.g. non-null or non-undefined), so I decided to also wrap the Heap values in a Box (and in dictionaries in RootedTraceableBox, see #20265 (comment) and the issue for more details) in dictionaries.

Since we allocate more to be safe, I think it'd be good to also do some sort of a JS perf run, if there is any to see if there's any significant overhead.

r? @jdm

---

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

• There are tests for these changes OR
• These changes do not require tests because checking for not moving Heap after setting a value would require encoding a lot more info in type system (Heap) and I'm not sure how to do that and end up with an ergonomic and consistent API

---

This change is 

[highfive]

Heads up! This PR modifies the following files:

• @asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/Cargo.toml, components/script/dom/bindings/conversions.rs, components/script/dom/bindings/iterable.rs
• @fitzgen: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/Cargo.toml, components/script/dom/bindings/conversions.rs, components/script/dom/bindings/iterable.rs
• @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/Cargo.toml, components/script/dom/bindings/conversions.rs, components/script/dom/bindings/iterable.rs

[highfive]

Warning

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

[jdm]

When we've tried to evaluate JS perf changes in the past, we've run ./mach test-dromaeo --release and focused on the DOM Core tests.

[jdm]

The travisCI failure is #14774 .

[Xanewok]

I did 3 runs on master and patch, averaged the results and compare those.
This probably needs more samples, but I couldn't automate the process, since ./mach test-dromaeo --release dom intermittently failed for me with

ERROR:script::dom::bindings::error: Error at http://localhost:8192/dromaeo/web/tests/sunspider-3d-morph.html:8:29 startTest is not defined

for different tests (always with startTest is not defined).

Also probably ./mach compare_dromaeo needs updating or I'm not sure how to explicitly save the output of the test run - it couldn't find [dromaeo] Saving... (here), so I had to remove that and modify the test results a bit for it to work.

 Test							master		patch		diff
 dom-query/getElementsByTagName(a)                	46,283.87	46,669.87	0.83%
 dom-modify                                       	120.05		119.84		-0.17%
 dom-query/getElementsByTagName (not in document) 	64,524.67	65,717.93	1.85%
 dom-traverse/nextSibling                         	466.22		451.03		-3.26%
 dom-attr/getAttribute                            	84,469.87	83,224.33	-1.47%
 dom-query/getElementsByName (not in document)    	0.28		0.27		-2.06%
 dom-query/getElementById                         	368.93		353.16		-4.27%
 dom-query/getElementsByTagName(p)                	47,167.67	44,975.73	-4.65%
 dom-traverse/lastChild                           	167.43		166.08		-0.81%
 dom-query                                        	1,218.14	1,199.87	-1.50%
 dom-query/getElementsByTagName(div)              	47,485.93	47,089.93	-0.83%
 dom-modify/cloneNode                             	47.58		47.97		0.83%
 dom-modify/appendChild                           	311.11		316.84		1.84%
 dom-attr                                         	1,605.80	1,763.87	9.84%
 dom-attr/element_expando = value                 	1,024.20	1,159.23	13.18%
 dom-traverse/previousSibling                     	471.18		452.30		-4.01%
 dom-traverse/childNodes                          	230.01		210.22		-8.60%
 dom-modify/insertBefore                          	285.23		274.56		-3.74%
 dom-attr/element_property                        	41,884.27	45,377.93	8.34%
 dom-traverse                                     	288.65		278.06		-3.67%
 dom-attr/element_property = value                	69.84		77.37		10.79%
 dom-modify/createElement                         	188.14		190.27		1.13%
 dom-traverse/firstChild                          	237.61		235.70		-0.80%
 dom                                              	575.26		581.72		1.12%
 dom-query/getElementsByName                      	0.34		0.33		-0.58%
 dom-attr/element_expando                         	1,272.24	1,351.94	6.26%
 dom-modify/innerHTML                             	26.32		26.20		-0.46%
 dom-attr/setAttribute                            	54.74		65.81		20.24%
 dom-modify/createTextNode                        	143.22		142.75		-0.33%
 dom-query/getElementsByTagName(*)                	47,868.80	46,498.73	-2.86%
 dom-query/getElementById (not in document)       	540.54		536.89		-0.68%

Besides couple of tests, like dom-attr/element_expando = value and dom-attr/element_property, is this something that can be considered a noise? I have to say that variance was pretty considerate when doing these runs and I'm not sure if we will need more samples to get any information out of these runs.

Re travis: is this something that needs to be resolved before this can land?

[jdm]

I wonder if those tests even execute these code paths, since I don't think there are dictionaries involved in those APIs. It's probably worth measuring one particular API that makes use of a dictionary that contains an object or any value.

[Xanewok]

I think it makes sense to base the benchmark on key/value iterable like in https://github.com/servo/servo/blob/master/tests/wpt/mozilla/tests/mozilla/iterable.html.

How should I measure the tests? Should I create a dummy HTML with the script, run the browser exe against it (like in run_dromaeo?) and measure the time it takes to finish?

Is there a benchmark/performance test suite/mode for WPT tests?

[jdm]

Yes, creating an HTML file like https://github.com/servo/servo/blob/master/tests/html/mut_observer_perf.html is the best way to go about it. There's no existing benchmark suite; just one-off pages like that.

[Xanewok]

So I tried creating a HTML benchmark, but I'm still not sure if I'm doing it right.
I tried running it with

./mach run -r --pref=dom.testbinding.enabled tests/html/pair_iterable_perf.html

and for both my local master (b93f153) and the branch I had average times 0.0296~0.0304s on a i5-2520m CPU and the times distribution seemed the same for both.
Whenever I tried to up either ENTRY_COUNT or RUN_COUNT, I encoutered a panic on both branches (did it allocate too much?):

ERROR:servo: DomRefCell<T> already mutably borrowed: BorrowError
fatal runtime error: failed to initiate panic, error 5
Redirecting call to abort() to mozalloc_abort

Stack trace for thread "ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(NonZero(NonZero(1))) }"
...
  14:     0x5567e65a3195 - core::result::unwrap_failed::hedcf71620cb1ff56
  15:     0x5567e5f72a0e - <script::dom::bindings::cell::DomRefCell<T> as script::dom::bindings::trace::JSTraceable>::trace::h73462e53b7d9746f
  16:     0x5567e6894aaa - <std::thread::local::LocalKey<T>>::with::hec1f001669ec6135
  17:     0x5567e62de28e - script::script_runtime::trace_rust_roots::h69bce205ed4d664d
  18:     0x5567e80cd54a - _ZN2js2gc9GCRuntime11markRuntimeEP8JSTracerNS1_18TraceOrMarkRuntimeERNS_26AutoLockForExclusiveAccessE
                        at /home/xanewok/.cargo/registry/src/github.com-1ecc6299db9ec823/mozjs_sys-0.50.1/mozjs/js/src/gc/RootMarking.cpp:343
...

I believe this should test the code path now, since despite the fact that iterable is generic over non-any values

servo/components/script/dom/webidls/TestBindingPairIterable.webidl

Line 11 in 234951e

iterable<DOMString, unsigned long>;

the generated code uses IterableIterator under the hood (e.g. for entries()):

unsafe extern fn entries(cx: *mut JSContext, _obj: HandleObject, this: *const TestBindingPairIterable, args: *const JSJitMethodCallArgs) -> bool {
    return wrap_panic(panic::AssertUnwindSafe(|| {
        let this = &*this;
        let args = &*args;
        let argc = args._base.argc_;
        let result = IterableIterator::new(&*this,
                                       IteratorType::Entries,
                                       super::TestBindingPairIterableIteratorBinding::Wrap);


        (result).to_jsval(cx, args.rval());
        return true;
    }), false);
}

which in turns uses the modified API in fn Next():

servo/components/script/dom/bindings/iterable.rs

Line 102 in c1ed4bb

key_and_value_return(cx, rval.handle_mut(), key.handle(), value.handle())

[jdm]

The panic is a result of a GC occurring and #19872. That looks like a sensible benchmark to me :)

[Xanewok]

Cool, thanks for taking a look! So does it need more work or is it just blocked on servo/rust-mozjs#398?

[jdm]

@bors-servo r+

[bors-servo]

📌 Commit 0ea6d8a has been approved by jdm

[bors-servo]

⌛ Testing commit 0ea6d8a with merge 23b6f56...

[Xanewok]

@jdm this still uses mozjs 0.3 from my branch, shouldn't we switch it to use the now-published 0.3 from crates.io before merging?

[bors-servo]

☀️ 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: jdm
Pushing 23b6f56 to master...