search

servo / rust-mozjs

DEPRECATED - moved to servo/mozjs instead.
Mozilla Public License 2.0
293 stars 122 forks source link

Reimplement CustomAutoRooter hierarchy #382

Closed Xanewok closed 6 years ago

Xanewok commented 6 years ago

This is an attempt at https://github.com/servo/servo/issues/16678. It doesn't yet contain conversion code that's described here: https://github.com/servo/servo/issues/16678#issuecomment-345774906.

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 Reviewable

Xanewok commented 6 years ago

Addressed feedback and pushed related changes.

Xanewok commented 6 years ago

@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 commented 6 years ago

@bors-servo: r+ Thanks!

bors-servo commented 6 years ago

:pushpin: Commit 03b62d3 has been approved by jdm

bors-servo commented 6 years ago

:hourglass: Testing commit 03b62d3ad2c9f398259e520d0db708c77c9cae73 with merge bc498516d5a1fc4be54028077581d5e73265380f...

bors-servo commented 6 years ago

:broken_heart: Test failed - status-appveyor

jdm commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

Xanewok commented 6 years ago

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 commented 6 years ago

@bors-servo: r+

bors-servo commented 6 years ago

:pushpin: Commit d88d93a has been approved by jdm

bors-servo commented 6 years ago

:hourglass: Testing commit d88d93aa9f1acd744bb27a9efcc1d89ae3f52538 with merge 0551c14afbf241a1ce567802ebf03b7781046417...

bors-servo commented 6 years ago

:broken_heart: Test failed - status-travis

Xanewok commented 6 years ago

Same as https://github.com/servo/rust-mozjs/pull/381#issuecomment-348116907, 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 commented 6 years ago

:sunny: Test successful - status-appveyor, status-travis Approved by: jdm Pushing 0551c14afbf241a1ce567802ebf03b7781046417 to master...