You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "findepi (via GitHub)" <gi...@apache.org> on 2024/09/02 11:27:45 UTC
[PR] Remove unsafe Send impl from PriorityMap [datafusion]
findepi opened a new pull request, #12289:
URL: https://github.com/apache/datafusion/pull/12289
It's not necessary to use unsafe Send impl. It's enough to require the referenced trait objects as Send.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove unsafe Send impl from PriorityMap [datafusion]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on code in PR #12289:
URL: https://github.com/apache/datafusion/pull/12289#discussion_r1741143078
##########
datafusion/physical-plan/src/aggregates/topk/priority_map.rs:
##########
@@ -25,17 +25,12 @@ use datafusion_common::Result;
/// A `Map<K, V>` / `PriorityQueue` combo that evicts the worst values after reaching `capacity`
pub struct PriorityMap {
- map: Box<dyn ArrowHashTable>,
- heap: Box<dyn ArrowHeap>,
+ map: Box<dyn ArrowHashTable + Send>,
+ heap: Box<dyn ArrowHeap + Send>,
capacity: usize,
mapper: Vec<(usize, usize)>,
}
-// JUSTIFICATION
Review Comment:
I believe I ran the benchmark, and it was slower than without the optimization. So I made this change and it got however much faster is listed in the comment.
I sounds from @findepi though like the question is now moot?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove unsafe Send impl from PriorityMap [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #12289:
URL: https://github.com/apache/datafusion/pull/12289#discussion_r1740932826
##########
datafusion/physical-plan/src/aggregates/topk/priority_map.rs:
##########
@@ -25,17 +25,12 @@ use datafusion_common::Result;
/// A `Map<K, V>` / `PriorityQueue` combo that evicts the worst values after reaching `capacity`
pub struct PriorityMap {
- map: Box<dyn ArrowHashTable>,
- heap: Box<dyn ArrowHeap>,
+ map: Box<dyn ArrowHashTable + Send>,
+ heap: Box<dyn ArrowHeap + Send>,
capacity: usize,
mapper: Vec<(usize, usize)>,
}
-// JUSTIFICATION
Review Comment:
i didn't verify benchmarks, but per static code anaysis, PriorityMap is required to be Send. if you just remove trait implementation (marker), code won't compile.
With this PR, PriorityMap is **still** Send. The only difference is that this is now inferred by the compiler, so safer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove unsafe Send impl from PriorityMap [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #12289:
URL: https://github.com/apache/datafusion/pull/12289#discussion_r1741180122
##########
datafusion/physical-plan/src/aggregates/topk/priority_map.rs:
##########
@@ -25,17 +25,12 @@ use datafusion_common::Result;
/// A `Map<K, V>` / `PriorityQueue` combo that evicts the worst values after reaching `capacity`
pub struct PriorityMap {
- map: Box<dyn ArrowHashTable>,
- heap: Box<dyn ArrowHeap>,
+ map: Box<dyn ArrowHashTable + Send>,
+ heap: Box<dyn ArrowHeap + Send>,
capacity: usize,
mapper: Vec<(usize, usize)>,
}
-// JUSTIFICATION
Review Comment:
removing `unsafe impl Send for PriorityMap {}` line alone gives compilation error, because rustc does not infer `PriorityMap` to be `Send`
```
main *$ cargo build
Compiling datafusion-physical-plan v41.0.0 (/Users/findepi/repos/datafusion/datafusion/physical-plan)
error[E0277]: `(dyn ArrowHashTable + 'static)` cannot be sent between threads safely
--> datafusion/physical-plan/src/aggregates/mod.rs:249:9
|
249 | / match stream {
250 | | StreamType::AggregateStream(stream) => Box::pin(stream),
251 | | StreamType::GroupedHash(stream) => Box::pin(stream),
252 | | StreamType::GroupedPriorityQueue(stream) => Box::pin(stream),
253 | | }
| |_________^ `(dyn ArrowHashTable + 'static)` cannot be sent between threads safely
|
= help: the trait `std::marker::Send` is not implemented for `(dyn ArrowHashTable + 'static)`, which is required by `GroupedTopKAggregateStream: std::marker::Send`
= note: required for `std::ptr::Unique<(dyn ArrowHashTable + 'static)>` to implement `std::marker::Send`
note: required because it appears within the type `Box<(dyn ArrowHashTable + 'static)>`
--> /Users/findepi/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/boxed.rs:237:12
|
237 | pub struct Box<
| ^^^
note: required because it appears within the type `PriorityMap`
--> datafusion/physical-plan/src/aggregates/topk/priority_map.rs:27:12
|
27 | pub struct PriorityMap {
| ^^^^^^^^^^^
note: required because it appears within the type `GroupedTopKAggregateStream`
--> datafusion/physical-plan/src/aggregates/topk_stream.rs:39:12
|
39 | pub struct GroupedTopKAggregateStream {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: required for the cast from `Pin<Box<GroupedTopKAggregateStream>>` to `Pin<Box<dyn RecordBatchStream + std::marker::Send>>`
error[E0277]: `(dyn ArrowHeap + 'static)` cannot be sent between threads safely
--> datafusion/physical-plan/src/aggregates/mod.rs:249:9
|
249 | / match stream {
250 | | StreamType::AggregateStream(stream) => Box::pin(stream),
251 | | StreamType::GroupedHash(stream) => Box::pin(stream),
252 | | StreamType::GroupedPriorityQueue(stream) => Box::pin(stream),
253 | | }
| |_________^ `(dyn ArrowHeap + 'static)` cannot be sent between threads safely
|
= help: the trait `std::marker::Send` is not implemented for `(dyn ArrowHeap + 'static)`, which is required by `GroupedTopKAggregateStream: std::marker::Send`
= note: required for `std::ptr::Unique<(dyn ArrowHeap + 'static)>` to implement `std::marker::Send`
note: required because it appears within the type `Box<(dyn ArrowHeap + 'static)>`
--> /Users/findepi/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/boxed.rs:237:12
|
237 | pub struct Box<
| ^^^
note: required because it appears within the type `PriorityMap`
--> datafusion/physical-plan/src/aggregates/topk/priority_map.rs:27:12
|
27 | pub struct PriorityMap {
| ^^^^^^^^^^^
note: required because it appears within the type `GroupedTopKAggregateStream`
--> datafusion/physical-plan/src/aggregates/topk_stream.rs:39:12
|
39 | pub struct GroupedTopKAggregateStream {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: required for the cast from `Pin<Box<GroupedTopKAggregateStream>>` to `Pin<Box<dyn RecordBatchStream + std::marker::Send>>`
For more information about this error, try `rustc --explain E0277`.
error: could not compile `datafusion-physical-plan` (lib) due to 2 previous errors
```
however, removing `unsafe impl Send for PriorityMap {}` **plus** other changes in this PR keeps `PriorityMap` as `Send`, so the code works exactly as it does on current main
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove unsafe Send impl from PriorityMap [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #12289:
URL: https://github.com/apache/datafusion/pull/12289#issuecomment-2325773029
yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove unsafe Send impl from PriorityMap [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12289:
URL: https://github.com/apache/datafusion/pull/12289#discussion_r1740812783
##########
datafusion/physical-plan/src/aggregates/topk/priority_map.rs:
##########
@@ -25,17 +25,12 @@ use datafusion_common::Result;
/// A `Map<K, V>` / `PriorityQueue` combo that evicts the worst values after reaching `capacity`
pub struct PriorityMap {
- map: Box<dyn ArrowHashTable>,
- heap: Box<dyn ArrowHeap>,
+ map: Box<dyn ArrowHashTable + Send>,
+ heap: Box<dyn ArrowHeap + Send>,
capacity: usize,
mapper: Vec<(usize, usize)>,
}
-// JUSTIFICATION
Review Comment:
@avantgardnerio -- can you please remind me how we tested this / how we can double check that this doesn't cause a performance regression?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove unsafe Send impl from PriorityMap [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #12289:
URL: https://github.com/apache/datafusion/pull/12289
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org