search

servo / unicode-bidi

Implementation of the Unicode Bidirection Algorithm in Rust
Other
74 stars 34 forks source link

Instrumentation for perf analysis using Flame #40

Closed behnam closed 7 years ago

behnam commented 7 years ago

Test:

cargo run --example flame_udhr --features flame_it

produces flame-udhr-graph.html.

This change is Reviewable

behnam commented 7 years ago

Typical flame-udhr-graph.html:

screen shot 2017-06-14 at 4 19 37 am
emilio commented 7 years ago

This looks quite cool, but Travis seems unhappy:

error[E0554]: #[feature] may not be used on the beta release channel
 --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/flamer-0.1.4/src/lib.rs:1:1
  |
1 | #![feature(plugin_registrar, quote, rustc_private, custom_attribute)]
emilio commented 7 years ago

(It's just a matter of adjusting the travis script to only run the flame_it tests on nightly)

behnam commented 7 years ago

Thanks, @emilio. Yeah, I forgot about limiting it in nightly. Posted an updated that does so.

Then, something strange happened on travis: https://travis-ci.org/behnam/rust-unicode-bidi/jobs/242960645

running 23 tests
test char_data::tests::test_bidi_class ... ok
test level::tests::test_new_explicit ... ok
test level::tests::test_raise ... ok
test level::tests::test_raise_explicit ... ok
test level::tests::test_str_eq ... ok
test level::tests::test_string_eq ... ok
test level::tests::test_vec ... ok
test prepare::tests::test_level_runs ... ok
test prepare::tests::test_not_removed_by_x9 ... ok
test prepare::tests::test_removed_by_x9 ... ok
test level::tests::test_is_ltr ... ok
test level::tests::test_is_rtl ... ok
test level::tests::test_into ... ok
test level::tests::test_new ... ok
test tests::test_initial_text_info ... ok
test level::tests::test_has_rtl ... ok
thread panicked while processing panic. aborting.
error: process didn't exit successfully: `/home/travis/build/behnam/rust-unicode-bidi/target/debug/deps/unicode_bidi-72a6ca68f0858202` (signal: 4, SIGILL: illegal instruction)

The command "travis-cargo --only nightly test -- --features flame_it" exited with 101.

It doesn't repro on my machine (macOS). Also, the next command, travis-cargo --only nightly run -- --features flame_it --example flame_udhr, works fine. I'm guessing could be something about flame on nightly over linux.

Now, I've enforced another travis run for the patch to see if it repros.

behnam commented 7 years ago

Previous link was for travis for my own fork. The same happened on the servo PR travis: https://travis-ci.org/servo/unicode-bidi/jobs/242960658, and on the new run: https://travis-ci.org/servo/unicode-bidi/jobs/242966158

running 23 tests
test char_data::tests::test_bidi_class ... ok
test level::tests::test_is_rtl ... ok
test level::tests::test_has_rtl ... ok
test level::tests::test_is_ltr ... ok
test level::tests::test_into ... ok
test level::tests::test_str_eq ... ok
test level::tests::test_string_eq ... ok
thread panicked while processing panic. aborting.
error: process didn't exit successfully: `/home/travis/build/servo/unicode-bidi/target/debug/deps/unicode_bidi-6c9a148930ea74f6` (signal: 4, SIGILL: illegal instruction)

The command "travis-cargo --only nightly test -- --features flame_it" exited with 101.

I'm not sure how to investigate it, since don't have a linux dev server around at the moment.

@TyOverby, is it a known problem with flame?

TyOverby commented 7 years ago

@behnam Nope, that's pretty unexpected. Interestingly, Flame doesn't have any unsafe and the only libraries that it uses are lazy_static and thread_id which are pretty heavily battle tested.

jdm commented 7 years ago

I recommend adding RUST_BACKTRACE=1 to all testing commands.

behnam commented 7 years ago

So, RUST_BACKTRACE=1 didn't help at all and I get the same thread panicked while processing panic. aborting. without any more info.

Then I added RUST_TEST_THREADS=1, which made it clear which lib test fn is failing, and it's prepare::tests::test_isolating_run_sequences_sos_and_eos().

Now, the thing is that it's been passing without any problems without flame/flamer code, and it's failing on linux+nightly on travis consistently. So, either a rustc nightly issue, flame/flamer issue, or unicode-bidi issue surfaced by the flame interruptions.

So, I'm finding a linux env to try it on before we move forward with this PR. Thanks everyone for the tips!

behnam commented 7 years ago

Hum... the error doesn't repro on ide.c9.io either, having the same versions of rustc-nightly and crates. :|

Now setting up ubuntu VMs to keep it closer to travis....

behnam commented 7 years ago

Okay, I've added dist: trusty to .travis.yml and now all tests pass. This is now good to get a review.

I'm installing a local Ubuntu 12.04 LTS to investigate the threat panic issue we had on nightly. Looks like a rustc issue to me. I'll file a new issue when I find more.

mbrubeck commented 7 years ago

@bors-servo r+

bors-servo commented 7 years ago

:pushpin: Commit bf4a987 has been approved by mbrubeck

bors-servo commented 7 years ago

:hourglass: Testing commit bf4a9870b6a346dcac8b66f3ae77e37f899cff82 with merge 7224dcf17160dfca37733fdc475aaa1282a8ac27...

jdm commented 7 years ago

@bors-servo: r=mbrubeck

bors-servo commented 7 years ago

:pushpin: Commit 325b6be has been approved by mbrubeck

bors-servo commented 7 years ago

:hourglass: Testing commit 325b6be383106e7431e4609c4bc5ebbb979b1c49 with merge b8a3364cba9e9ca6e255d3ecb96db8820ba8ed8a...

bors-servo commented 7 years ago

:sunny: Test successful - status-travis Approved by: mbrubeck Pushing b8a3364cba9e9ca6e255d3ecb96db8820ba8ed8a to master...

behnam commented 7 years ago

The nightly tests failed on this branch after an update to flame yesterday. I've filed an issue for flame here: https://github.com/TyOverby/flame/issues/22

This error is introduced in flame:=0.1.12 and there's another bug on `flame:<0.1.12, so we should consider the flame_it feature partially broken until https://github.com/TyOverby/flame/issues/22 is resolved.