Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upUpdate heartbeats-simple dependencies for bug fixes #15458
Conversation
highfive
commented
Feb 8, 2017
|
Heads up! This PR modifies the following files:
|
|
Perhaps somebody can answer a Rust question for me as well. For consistency I wanted to transmute the hashmap pointer like so, as is done in at least one place, rather than dereferencing it each time I used it: let mut hbs: Box<HashMap<ProfilerCategory, Heartbeat>> = mem::transmute(hbs_ptr);However, when I did this an passed the FYI, we do this because there are underlying -sys bindings that don't implement Here's a backtrace for the aforementioned Illegal Instruction - the line numbers won't line up perfectly though: |
I believe the standard way of doing that is using |
|
I suspect that the Box goes out of scope at some point and frees the memory that it points to. |
|
I figured it was something like that but don't understand Rust in enough detail to say for sure. I tried reproducing it with a minimal test program but wasn't able to. I'll check out |
|
I switched to using |
|
Although we've never had any problems that I'm aware of, I am a little concerned about race conditions in accessing the raw HashMap pointer which is dropped in the I could create a separate |
|
I filed #15471 about the preexisting unsynchronized access here. |
|
Thanks @Ms2ger . I can commit a locking strategy as described above to this PR if you want to see it. Unfortunately I don't think I can wrap the |
|
I pushed a new commit which adds synchronization using a spinlock. Please comment. I suspect this would be better done with a function like |
|
You can use something like pub fn foo<F, R>(function: F) -> R
where F: FnOnce() -> R
{
lock;
let result = function();
unlock;
result
} |
|
Thanks @Ms2ger , that's what I'm looking for. I've updated the commit to use that approach. I think this PR is ready for a final review. |
|
|
||
|
|
||
| static mut HBS: Option<*mut HashMap<ProfilerCategory, Heartbeat>> = None; | ||
|
|
||
| // unfortunately can't encompass the actual hashmap in a Mutex (Heartbeat isn't Send/Sync), so we'll use a spinlock | ||
| static mut HBS_SPINLOCK: AtomicBool = ATOMIC_BOOL_INIT; |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
connorimes
Feb 14, 2017
Author
Contributor
Awesome, that also avoids needing unsafe during lock/unlock. Thanks.
| } | ||
| lock_and_work(|| | ||
| unsafe { | ||
| HBS = Some(Box::into_raw(hbs)); |
This comment has been minimized.
This comment has been minimized.
Ms2ger
Feb 14, 2017
Contributor
Let's pass &mut HBS to the callback, then the code that uses it can be... more safe.
This comment has been minimized.
This comment has been minimized.
connorimes
Feb 14, 2017
Author
Contributor
Can we safely touch HBS like that outside of the locking? Also, are you talking about doing this everywhere we use lock_and_work? My Rust is apparently really rusty - would I need to first define the closure outside of the call to lock_and_work in order to pass &mut HBS to it? Thanks.
This comment has been minimized.
This comment has been minimized.
Ms2ger
Feb 14, 2017
Contributor
Something like
fn lock_and_work<F, R>(work: F) -> R
where F: FnOnce(&mut Option<*mut HashMap<ProfilerCategory, Heartbeat>>) -> R
{
work(&mut HBS)
}| // unfortunately can't encompass the actual hashmap in a Mutex (Heartbeat isn't Send/Sync), so we'll use a spinlock | ||
| static mut HBS_SPINLOCK: AtomicBool = ATOMIC_BOOL_INIT; | ||
|
|
||
| fn lock_and_work<F, R>(work: F) -> R where F: FnOnce() -> R { |
This comment has been minimized.
This comment has been minimized.
emilio
Feb 14, 2017
Member
To enforce that HBS is only used in a sound way you can move the declaration inside this function I believe (or in another module, so it's private to everything except this function), and give it as a parameter to work.
This comment has been minimized.
This comment has been minimized.
connorimes
Feb 14, 2017
•
Author
Contributor
Thanks. I'm trying out this approach which makes sense. However, when trying to assign the global HBS by way of the closure parameter, I get
133 | lock_and_work(|&mut hbs_opt|
| ------- first assignment to `hbs_opt`
134 | hbs_opt = Some(Box::into_raw(hbs))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ re-assignment of immutable variableThe only way I have around this is to move the init and cleanup functions to the private module and still assign HBS directly using an unsafe block. So the old approach works for these two functions, but doesn't completely limit access to HBS to the lock_and_work function, which is what I think we were after. For reference within lock_and_work, I do:
work(&mut HBS)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
connorimes
Feb 14, 2017
Author
Contributor
Ah, I was using |&mut hbs_opt| instead of just |hbs_opt| as the closure parameter.
|
Thanks again for the all the feedback (this has turned into way more than the original PR was for). I've pushed a new commit in response to your suggestions. |
| unsafe { | ||
| HBS = Some(Box::into_raw(hbs)); | ||
| lock_and_work(|hbs_opt| | ||
| if (*hbs_opt).is_none() { |
This comment has been minimized.
This comment has been minimized.
emilio
Feb 14, 2017
Member
nit: You shouldn't need (*hbs_opt), hbs_opt should be enough in this case (the compiler auto-dereferences).
| None => None, | ||
| } | ||
| let hbs_opt_box: Option<Box<HashMap<ProfilerCategory, Heartbeat>>> = lock_and_work(|hbs_opt| | ||
| match *hbs_opt { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
connorimes
Feb 14, 2017
Author
Contributor
I see what you're getting at, though that produced an error:
30 | hbs_opt.take().map(Box::from_raw)
| ^^^ the trait `std::ops::FnOnce<(*mut std::collections::HashMap<profile_traits::time::ProfilerCategory, heartbeats_simple::HeartbeatPow>,)>` is not implemented for `unsafe fn(*mut _) -> std::boxed::Box<_> {<std::boxed::Box<T>><_>::from_raw}`
It works if I give it a closure though:
let hbs_opt_box: Option<Box<HashMap<ProfilerCategory, Heartbeat>>> = lock_and_work(|hbs_opt|
hbs_opt.take().map(|hbs_ptr| unsafe {Box::from_raw(hbs_ptr)})
);The take documentation seems to support this approach, but it's not obvious to the the layman (me), so I'll double check - you're certain this will set the hbs_opt (and thus HBS) option to None afterward?
This comment has been minimized.
This comment has been minimized.
| HBS.map_or(false, |hbs_ptr| (*hbs_ptr).contains_key(category)) | ||
| } | ||
| let is_enabled = lock_and_work(|hbs_opt| | ||
| (*hbs_opt).map_or(false, |hbs_ptr| |
This comment has been minimized.
This comment has been minimized.
|
OK, I cleaned up those lines. I'd still like to verify #15458 (comment) though. Cheers. |
|
This looks ok to me, r=me unless @Ms2ger has any more comments? |
|
|
|
FYI, I rebased yesterday but I guess the S-need-rebase tag doesn't get removed automatically. @Ms2ger did you have any further comments on this PR? Thanks. |
|
|
|
@bors-servo r=emilio |
|
|
Update heartbeats-simple dependencies for bug fixes <!-- Please describe your changes on the following line: --> Updates heartbeats-simple dependencies for some bug fixes in native code (primarily for Windows). Now we create heartbeats as needed so we don't have to maintain a hardcoded list which keeps getting out of sync. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15471 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [X] These changes do not require tests because it updates a dependency and performs some code maintenance. <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15458) <!-- Reviewable:end -->
|
|
|
@bors-servo retry
|
Update heartbeats-simple dependencies for bug fixes <!-- Please describe your changes on the following line: --> Updates heartbeats-simple dependencies for some bug fixes in native code (primarily for Windows). Now we create heartbeats as needed so we don't have to maintain a hardcoded list which keeps getting out of sync. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15471 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [X] These changes do not require tests because it updates a dependency and performs some code maintenance. <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15458) <!-- Reviewable:end -->
|
|
connorimes commentedFeb 8, 2017
•
edited
Updates heartbeats-simple dependencies for some bug fixes in native code (primarily for Windows).
Now we create heartbeats as needed so we don't have to maintain a hardcoded list which keeps getting out of sync.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is