Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nonzeroing Move Hints (take3 branch) #26173

Merged
merged 11 commits into from
Jul 28, 2015

Conversation

pnkfelix
Copy link
Member

Add dropflag hints (stack-local booleans) for unfragmented paths in trans. Part of #5016.

Added code to maintain these hints at runtime, and to conditionalize drop-filling and calls to destructors.

In this early stage of my incremental implementation strategy, we are using hints, so we are always free to leave out a flag for a path -- then we just pass None as the dropflag hint in the corresponding schedule cleanup call. But, once a path has a hint, we must at least maintain it: i.e. if the hint exists, we must ensure it is never set to "moved" if the data in question might actually have been initialized. It remains sound to conservatively set the hint to "initialized" as long as the true drop-flag embedded in the value itself is up-to-date.

I hope the commit series has been broken up to be readable; most of the commits in the series should build (though I did not always check this).


Oh, I think this technically qualifies as a:
[breaking-change]
because it removes drop-filling in some cases where one could previously observe it. That should only affect unsafe code; no safe code should be able to inspect whether the drop-fill was present or not. For an example of code that needed to change to account for this, see commit a81c24ae0216ab47df59acd724f8a33124fb6d97 (a unit test of the move_val_init intrinsic). I have not encountered actual code that needed to be updated to account for the change, since this should only be skipping the drop-filling on moved values, not on dropped one, and so even types that use unsafe_no_drop_flag should be unchanged by this particular PR. (Their time will come later.)

@rust-highfive
Copy link
Collaborator

r? @Aatch

(rust_highfive has picked a reviewer for you, use r? to override)

@pnkfelix
Copy link
Member Author

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned Aatch Jun 10, 2015
@pnkfelix
Copy link
Member Author

cc @pcwalton @eefriedman @eddyb

@pnkfelix
Copy link
Member Author

Note: I am still putting this branch through its paces locally.

My most recent make check-stage1 attempt, in a --enable-debug --enable-optimize build, bombs out
during stage1/test/stdtest-x86_64-apple-darwin:

...
test sys_common::wtf8::tests::wtf8buf_show ... ok
test sys_common::wtf8::tests::wtf8buf_truncate_fail_code_point_boundary ... /bin/sh: line 1: 61239 Trace/BPT trap: 5       DYLD_LIBRARY_PATH=/Users/fklock/Dev/Mozilla/rust-nzdrop/objdir-dbgopt/x86_64-apple-darwin/stage1/lib/rustlib/x86_64-apple-darwin/lib:$DYLD_LIBRARY_PATH x86_64-apple-darwin/stage1/test/stdtest-x86_64-apple-darwin --logfile tmp/check-stage1-T-x86_64-apple-darwin-H-x86_64-apple-darwin-std.log
make: *** [tmp/check-stage1-T-x86_64-apple-darwin-H-x86_64-apple-darwin-std.ok] Error 133

Not sure yet what is up with that; it could be a latent bug in my PR, or some other bad interaction.

In any case, I wanted to put this up for some preliminary review; I suspect that even if we cannot land the whole PR yet, some of the earlier commits that refactor the code may be worth landing on their own.

@nikomatsakis
Copy link
Contributor

@pnkfelix
Copy link
Member Author

(i'm sure people are curious about performance results too. I can work on posting some numbers once I regather them, but the expectation based on my earlier measurements is that this yields somewhere between a 2% and 5% improvement in compilation time. It is not 100% clear where the improvement is coming from; that is, I had thought we would be able to clearly associate the improvement with one factor like "LLVM is spending less time trying to optimize code")

@pnkfelix
Copy link
Member Author

(I do want to read over @eefriedman 's alternative approach to removing drop flags, at eefriedman:dynamic-drop ; but I wanted to get this up first since as I said it has some changes that may want to land regardless of what overall strategy we adopt.)

@pnkfelix
Copy link
Member Author

cc @carllerche

@pnkfelix
Copy link
Member Author

The stage1/test/stdtest-x86_64-apple-darwin bug appears to be somewhat non-deterministic, but running it under a debugger also seems to improve the probability that I can observe it.

There is not a 100% clear smoking gun from the debugger backtraces, however. Most of the backtraces look like this:

(lldb) bt
* thread #1: tid = 0xeee85d, 0x00007fff9144e136 libsystem_kernel.dylib`__psynch_cvwait + 10, queue = 'com.apple.main-thread', stop reason = signal SIGPIPE
  * frame #0: 0x00007fff9144e136 libsystem_kernel.dylib`__psynch_cvwait + 10
    frame #1: 0x00007fff8984ae0c libsystem_pthread.dylib`_pthread_cond_wait + 693
    frame #2: 0x00000001005f7770 libstd-d8ace771.dylib`std::thread::park + 8 at condvar.rs:47
    frame #3: 0x00000001005f7768 libstd-d8ace771.dylib`std::thread::park [inlined] std::sys_common::condvar::Condvar::wait at condvar.rs:44
    frame #4: 0x00000001005f7768 libstd-d8ace771.dylib`std::thread::park [inlined] std::sync::condvar::StaticCondvar::wait<bool>(self=<unavailable>) + 17 at condvar.rs:255
    frame #5: 0x00000001005f7757 libstd-d8ace771.dylib`std::thread::park [inlined] std::sync::condvar::Condvar::wait<bool> + 32 at condvar.rs:133
    frame #6: 0x00000001005f7737 libstd-d8ace771.dylib`std::thread::park + 295 at mod.rs:517
    frame #7: 0x0000000100635bb2 libstd-d8ace771.dylib`std::sync::mpsc::blocking::WaitToken::wait(self=<unavailable>) + 82 at blocking.rs:83
    frame #8: 0x000000010042502e libtest-d8ace771.dylib`test::sync::mpsc::Receiver<T>::recv [inlined] test::sync::mpsc::shared::Packet<T>::recv(self=<unavailable>) + 65 at shared.rs:231
    frame #9: 0x0000000100424fed libtest-d8ace771.dylib`test::sync::mpsc::Receiver<T>::recv(self=<unavailable>) + 4365 at mod.rs:792
    frame #10: 0x000000010040e782 libtest-d8ace771.dylib`test::run_tests_console + 201 at lib.rs:825
    frame #11: 0x000000010040e6b9 libtest-d8ace771.dylib`test::run_tests_console(opts=0x00007fff5fbff0b0, tests=<unavailable>) + 7657 at lib.rs:698

but there were two that looked like this:

(lldb) bt
* thread #1: tid = 0xeed4a1, 0x00007fff914495c2 libsystem_kernel.dylib`swtch_pri + 10, queue = 'com.apple.main-thread', stop reason = signal SIGPIPE
  * frame #0: 0x00007fff914495c2 libsystem_kernel.dylib`swtch_pri + 10
    frame #1: 0x00007fff8984b37a libsystem_pthread.dylib`_pthread_find_thread + 81
    frame #2: 0x00007fff8984b978 libsystem_pthread.dylib`_pthread_lookup_thread + 53
    frame #3: 0x00007fff8984b8d4 libsystem_pthread.dylib`pthread_detach + 22
    frame #4: 0x000000010062e715 libstd-d8ace771.dylib`std::sys::thread::Thread.Drop::drop(self=<unavailable>) + 53 at thread.rs:157
    frame #5: 0x000000010042f408 libtest-d8ace771.dylib`std..thread..JoinInner$LT$$LP$$RP$$GT$::drop.9367::ha226051b3a20aed3 + 72
    frame #6: 0x000000010042f1c0 libtest-d8ace771.dylib`test::run_test::run_test_inner(desc=<unavailable>, monitor_ch=<unavailable>, nocapture=<unavailable>, testfn=<unavailable>) + 1456 at lib.rs:944
    frame #7: 0x000000010041f460 libtest-d8ace771.dylib`test::run_test(opts=<unavailable>, force_ignore=<unavailable>, test=<unavailable>, monitor_ch=<unavailable>) + 672 at lib.rs:989
    frame #8: 0x000000010040e6fe libtest-d8ace771.dylib`test::run_tests_console + 69 at lib.rs:821
    frame #9: 0x000000010040e6b9 libtest-d8ace771.dylib`test::run_tests_console(opts=0x00007fff5fbff0b0, tests=<unavailable>) + 7657 at lib.rs:698

and then there was another that looked like this:

(lldb) bt
* thread #1: tid = 0xeeeaff, 0x00007fff9144d7da libsystem_kernel.dylib`__bsdthread_create + 10, queue = 'com.apple.main-thread', stop reason = signal SIGPIPE
  * frame #0: 0x00007fff9144d7da libsystem_kernel.dylib`__bsdthread_create + 10
    frame #1: 0x00007fff8984a01d libsystem_pthread.dylib`pthread_create + 230
    frame #2: 0x000000010062fcdb libstd-d8ace771.dylib`std::sys::thread::Thread::new(stack=<unavailable>, p=<unavailable>) + 475 at thread.rs:64
    frame #3: 0x000000010042f7be libtest-d8ace771.dylib`test::thread::Builder::spawn_inner<()>(self=<unavailable>, f=<unavailable>) + 622 at mod.rs:353
    frame #4: 0x000000010042efd7 libtest-d8ace771.dylib`test::run_test::run_test_inner [inlined] test::thread::Builder::spawn<closure,()>(f=<unavailable>) + 26 at mod.rs:278
    frame #5: 0x000000010042efbd libtest-d8ace771.dylib`test::run_test::run_test_inner [inlined] test::thread::spawn<closure,()>(f=<unavailable>) + 442 at mod.rs:382
    frame #6: 0x000000010042ee03 libtest-d8ace771.dylib`test::run_test::run_test_inner(desc=<unavailable>, monitor_ch=<unavailable>, nocapture=<unavailable>, testfn=<unavailable>) + 499 at lib.rs:944
    frame #7: 0x000000010041f460 libtest-d8ace771.dylib`test::run_test(opts=<unavailable>, force_ignore=<unavailable>, test=<unavailable>, monitor_ch=<unavailable>) + 672 at lib.rs:989
    frame #8: 0x000000010040e6fe libtest-d8ace771.dylib`test::run_tests_console + 69 at lib.rs:821
    frame #9: 0x000000010040e6b9 libtest-d8ace771.dylib`test::run_tests_console(opts=0x00007fff5fbff0b0, tests=<unavailable>) + 7657 at lib.rs:698
    frame #10: 0x000000010040b573 libtest-d8ace771.dylib`test::test_main(args=<unavailable>, tests=<unavailable>) + 307 at lib.rs:253
    frame #11: 0x0000000100412d5e libtest-d8ace771.dylib`test::test_main_static(args=<unavailable>, tests=<unavailable>) + 2750 at lib.rs:276
    frame #12: 0x000000010022d1dc stdtest-x86_64-apple-darwin`__test::main::hd5f7e0a7fd4698957lw + 92
    frame #13: 0x00000001006e3939 libstd-d8ace771.dylib`rust_try_inner + 9
    frame #14: 0x00000001006e3926 libstd-d8ace771.dylib`rust_try + 6
    frame #15: 0x000000010064ae56 libstd-d8ace771.dylib`std::rt::lang_start [inlined] std::rt::unwind::try::inner_try(f=<unavailable>) + 76 at mod.rs:147
    frame #16: 0x000000010064ae0a libstd-d8ace771.dylib`std::rt::lang_start [inlined] std::rt::unwind::try<closure> at mod.rs:130
    frame #17: 0x000000010064ae0a libstd-d8ace771.dylib`std::rt::lang_start(main=<unavailable>, argc=<unavailable>, argv=<unavailable>) + 666 at mod.rs:128
    frame #18: 0x00007fff91f485c9 libdyld.dylib`start + 1
(lldb) 

(Its certainly possible that one of the cases that is marked as DontZeroJustUse needs to instead be ZeroAndMaintain in order to keep up some internal runtime invariant...)

@nikomatsakis
Copy link
Contributor

@pnkfelix did you observe an improvement in execution time as well? I cannot recall :)

// FIXME (#5016): pnkfelix is not 100% sure how we are
// going handle this long-term. Maybe easiest to just
// not allow arguments that are Drop to be passed
// indirectly.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the caller is trying to drop the alloca used for an indirect argument, that's a bug. They shouldn't be touched at all after a call because the callee is guaranteed to drop them. (At least, that's how I understand the current model.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is correct. The callee owns the value, they can move it, drop it, etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll see if I can properly encode this and avoid the ZeroAndMaintain here in that case. (But first I want to fix the bugs i am observing with this branch as it stands...)

@bors
Copy link
Contributor

bors commented Jun 11, 2015

☔ The latest upstream changes (presumably #26190) made this pull request unmergeable. Please resolve the merge conflicts.

@pcwalton
Copy link
Contributor

This looks good to me.

@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 12, 2015
@pnkfelix
Copy link
Member Author

(note that there is still that bug I need to resolve, noted above)

@pnkfelix
Copy link
Member Author

(note that there is still that bug I need to resolve, noted above)

Two weeks later: After some more investigation (that was interrupted several time for long stretches), I am pretty confident that the only bugs left to resolve are poor interactions with the drop-flag sanity checking that I put in.

In other words, if I cannot resolve those interactions quickly, then I will be happy to simply remove the sanity checking, and get this landed.

@nikomatsakis
Copy link
Contributor

r+ modulo comments and nits.

…hInput` rather than `ValueRef`.

(already thumbs-upped pre-rebase by nikomatsakis)

The refactoring here is trivial because `trans::datum::Lvalue`
currently carries no payload. However, future commits will start
adding a payload to `Lvalue`, and thus will force us either

 1. to thread the payload through the `_match` code (a long term goal), or

 2. to ensure the payload has some reasonable default value.
(already thumbs-upped pre-rebase by nikomatsakis)
…hint.

Instrumented calls sites that construct Lvalues to ease tracking down
cases that we might need to change whether or not they carry a hint.

Note that this commit does not do anything to actually *construct*
the `lldropflag_hints` map, nor does it change anything about codegen
itself. Those parts are in follow-on commits.
…rans.

Added code to maintain these hints at runtime, and to conditionalize
drop-filling and calls to destructors.

In this early stage, we are using hints, so we are always free to
leave out a flag for a path -- then we just pass `None` as the
dropflag hint in the corresponding schedule cleanup call. But, once a
path has a hint, we must at least maintain it: i.e. if the hint
exists, we must ensure it is never set to "moved" if the data in
question might actually have been initialized. It remains sound to
conservatively set the hint to "initialized" as long as the true
drop-flag embedded in the value itself is up-to-date.

----

Here are some high-level details I want to point out:

 * We maintain the hint in Lvalue::post_store, marking the lvalue as
   moved. (But also continue drop-filling if necessary.)

 * We update the hint on ExprAssign.

 * We pass along the hint in once closures that capture-by-move.

 * You only call `drop_ty` for state that does not have an associated hint.
   If you have a hint, you must call `drop_ty_core` instead.
   (Originally I passed the hint into `drop_ty` as well, to make the
   connection to a hint more apparent, but the vast majority of
   current calls to `drop_ty` are in contexts where no hint is
   available, so it just seemed like noise in the resulting diff.)
Testing indicates bug would have been caught, albeit later than one
might hope, during `sync::mpsc::tests::smoke_shared_port_gone2` test.
Updated all call sites that used the other contructors to uniformly
call `Lvalue::new_with_hint`, passing along the appropriate kind
of hint for each context.

Placated tidy in a few other places in datum.rs.
@pnkfelix
Copy link
Member Author

@bors r=nikomatsakis

@pnkfelix
Copy link
Member Author

@bors r=nikomatsakis b4dd765

@bors
Copy link
Contributor

bors commented Jul 28, 2015

📌 Commit b4dd765 has been approved by nikomatsakis

bors added a commit that referenced this pull request Jul 28, 2015
Add dropflag hints (stack-local booleans) for unfragmented paths in trans.  Part of #5016.

Added code to maintain these hints at runtime, and to conditionalize drop-filling and calls to destructors.

In this early stage of my incremental implementation strategy, we are using hints, so we are always free to leave out a flag for a path -- then we just pass `None` as the dropflag hint in the corresponding schedule cleanup call. But, once a path has a hint, we must at least maintain it: i.e. if the hint exists, we must ensure it is never set to "moved" if the data in question might actually have been initialized. It remains sound to conservatively set the hint to "initialized" as long as the true drop-flag embedded in the value itself is up-to-date.

I hope the commit series has been broken up to be readable; most of the commits in the series should build (though I did not always check this).

----

Oh, I think this technically qualifies as a:
[breaking-change]
because it removes drop-filling in some cases where one could previously observe it. That should only affect `unsafe` code; no safe code should be able to inspect whether the drop-fill was present or not. For an example of code that needed to change to account for this, see commit a81c24ae0216ab47df59acd724f8a33124fb6d97 (a unit test of the `move_val_init` intrinsic).  I have not encountered actual code that needed to be updated to account for the change, since this should only be skipping the drop-filling on *moved* values, not on dropped one, and so even types that use `unsafe_no_drop_flag` should be unchanged by this particular PR. (Their time will come later.)
@bors
Copy link
Contributor

bors commented Jul 28, 2015

⌛ Testing commit b4dd765 with merge 661a5ad...

@Gankra
Copy link
Contributor

Gankra commented Jul 28, 2015

🎊 💖 ✨

@bors bors merged commit b4dd765 into rust-lang:master Jul 28, 2015
pnkfelix added a commit to pnkfelix/rust that referenced this pull request Aug 7, 2015
This is a temporary workaround for the bugs that have been found in
the implementation of PR rust-lang#26173.

 * pnkfelix is unavailable in the short-term (i.e. for the next week) to fix them.

 * When the bugs are fixed, we will turn this back on by default.

(If you want to play with the known-to-be-buggy optimization in the
meantime, you can opt-back in via the debugging option that this
commit is toggling.)
bors added a commit that referenced this pull request Aug 7, 2015
…lt, r=nikomatsakis

Turn nonzeroing move hints back off by default.

Works around bugs injected by PR #26173.

 * (@pnkfelix is unavailable in the short-term (i.e. for the next week) to fix them.)

 * When the bugs are fixed, we will turn nonzeroing move hints back on by default.

Fix #27401
brson pushed a commit to brson/rust that referenced this pull request Aug 10, 2015
This is a temporary workaround for the bugs that have been found in
the implementation of PR rust-lang#26173.

 * pnkfelix is unavailable in the short-term (i.e. for the next week) to fix them.

 * When the bugs are fixed, we will turn this back on by default.

(If you want to play with the known-to-be-buggy optimization in the
meantime, you can opt-back in via the debugging option that this
commit is toggling.)
sfackler pushed a commit to sfackler/rust that referenced this pull request Aug 11, 2015
This is a temporary workaround for the bugs that have been found in
the implementation of PR rust-lang#26173.

 * pnkfelix is unavailable in the short-term (i.e. for the next week) to fix them.

 * When the bugs are fixed, we will turn this back on by default.

(If you want to play with the known-to-be-buggy optimization in the
meantime, you can opt-back in via the debugging option that this
commit is toggling.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants