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 ? |
| @@ -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.
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.
fwyzard
force-pushed
the
patatrack_update_BeamSpotCUDA_data_format
branch
from
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.
fwyzard
force-pushed
the
patatrack_update_BeamSpotCUDA_data_format
branch
from
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=53
logs 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
BeamSpotCUDAclass movable and explicitly non-copiable (as was already the case due to thedevice::unique_ptrdata member).Remove the
cudaMemcpyAsyncfrom theBeamSpotCUDAdata format, and make the copy explicitly in theBeamSpotToCUDAproducer.