-
Notifications
You must be signed in to change notification settings - Fork 5
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
Update the BeamSpotCUDA class #530
Update the BeamSpotCUDA class #530
Conversation
Requires (and includes) #529 . |
@AdrianoDee could you launch the validation of this PR ? |
@makortel, assuming the tests run fine, do you think this is a reasonable approach ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is certainly a reasonable approach. I can think some other ways, but I don't have a strong feeling between them. Assuming a long-term goal of a generic SoA that would internally manage its memory, the approach of this PR would be closest to that.
@@ -21,12 +20,22 @@ class BeamSpotCUDA { | |||
}; | |||
|
|||
BeamSpotCUDA() = default; | |||
BeamSpotCUDA(Data const* data_h, cudaStream_t stream); | |||
BeamSpotCUDA(cudaStream_t stream) { data_d_ = cms::cuda::make_device_unique<Data>(stream); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking out loud, an alternative would be to also allocate the data in the EDProducer and move in the cms::cuda::device::unique_ptr
. Then the ptr()
interface would not be needed.
I'm not convinced if that would be better though. E.g. I'd think that for a generic SoA the EDProducer should not have to deal with the memory allocation.
I suppose the main reason why allocation-in-EDProducer would look feasible here is the fact that BeamSpotCUDA
is essentially a wrapper for cms::cuda::device::unique_ptr
. Which leads me to another alternative of storing directly something along cms::cuda::device::unique_ptr<BeamSpotPOD>
(assuming BeamSpotCUDA::Data
would be renamed to, e.g., BeamSpotPOD
).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
5950cb3
to
15bd6df
Compare
For whatever reason is was working fine in 11.1.x, re-running in 11.2.x gave the same errors. The reason is that I was using the default constructor of I've committed two fixes:
Local test was fine on top of 11.2.x, I'll wait to re-test it once #531 has been merged. |
Could you elaborate why this would be needed? I'd imagine any use of the default-constructed |
That's fine by me... in fact, do we need to have a default constructor at all ? |
Default constructor is needed because of e.g. how |
Written that, if we'd really really want, we could work around this requirement (e.g. with |
Does it only need to exist, or is it actually called ? |
The default constructor is actually called. |
OK, then what is your preference ?
? |
The default constructor leading to "invalid object" is clearly what we do elsewhere. But the more I think about this, the more I start to like of changing
to std::optional<T> , which would allow storing objects without a default constructor. I think we should not even need any checks for the optional having a value. (or we could "fake" the optional with std::unique_ptr , although that would lead to additional indirection).
(maybe some day also |
OK, I'll make the change here as well. Using Looking at |
The comment is actually imprecise, the |
Make the BeamSpotCUDA movable and explicitly non-copiable (as was already the case due to the device::unique_ptr data member). Remove the cudaMemcpyAsync from the BeamSpotCUDA data format, and move it to the BeamSpotToCUDA producer.
15bd6df
to
b1cac7f
Compare
Validation summaryReference release CMSSW_11_2_0_pre2 at 92997c6 Validation plots/RelValTTbar_14TeV/CMSSW_11_1_0_pre8-PU_111X_mcRun3_2021_realistic_v4-v1/GEN-SIM-DIGI-RAW
/RelValZMM_14/CMSSW_11_1_0_pre8-111X_mcRun3_2021_realistic_v4-v1/GEN-SIM-DIGI-RAW
/RelValZEE_14/CMSSW_11_1_0_pre8-111X_mcRun3_2021_realistic_v4-v1/GEN-SIM-DIGI-RAW
Throughput plots/EphemeralHLTPhysics1/Run2018D-v1/RAW run=323775 lumi=53logs and
|
On a further look it could be the comment could actually more accurate than I indicated above. I don't think we (CMS) really call the default constructor of a data product ( |
I'm not sure why "testing" is always faster than "development", even when there are no relevant changes in the code... |
No impact on physics (as expected), probably no impact on timing (I don't trust the flat 10% improvement, since it applies also to the ECAL and HCAL workflows...) |
Make the BeamSpotCUDA movable and explicitly non-copiable (as was already the case due to the device::unique_ptr data member). Remove the cudaMemcpyAsync from the BeamSpotCUDA data format, and move it to the BeamSpotToCUDA producer.
Make the
BeamSpotCUDA
class movable and explicitly non-copiable (as was already the case due to thedevice::unique_ptr
data member).Remove the
cudaMemcpyAsync
from theBeamSpotCUDA
data format, and make the copy explicitly in theBeamSpotToCUDA
producer.