Skip to content

[VecOps] Fix #10297. Teach Rvec's to swap small and adopting vectors#10325

Merged
ikabadzhov merged 5 commits intoroot-project:masterfrom
ikabadzhov:VecOps_swap_fix
Apr 6, 2022
Merged

[VecOps] Fix #10297. Teach Rvec's to swap small and adopting vectors#10325
ikabadzhov merged 5 commits intoroot-project:masterfrom
ikabadzhov:VecOps_swap_fix

Conversation

@ikabadzhov
Copy link
Copy Markdown
Contributor

@ikabadzhov ikabadzhov commented Apr 5, 2022

Changes or fixes:

  • Rvec can now swap small and adopting vectors.
  • Introduce ROOT::Detail::VecOps::IsSmall(v) and ROOT::Detail::VecOps::IsAdopting(v) to check whether an RVec is small and owning its memory respectively
  • Added a corresponding test suite

Checklist:

  • tested changes locally

This PR fixes #10297

@ikabadzhov ikabadzhov self-assigned this Apr 5, 2022
@ikabadzhov ikabadzhov requested a review from eguiraud as a code owner April 5, 2022 08:29
@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 5, 2022

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 5, 2022

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@ikabadzhov ikabadzhov added this to the 6.26/02 milestone Apr 5, 2022
@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 5, 2022

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 5, 2022

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

eguiraud
eguiraud approved these changes Apr 5, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@eguiraud eguiraud left a comment

Choose a reason for hiding this comment

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

The new tests are amazing, thank you very much! The fix looks good to me as well.

Not necessarily for this PR, but it would be good to parameterize the new unit tests so they are all executed once with std::swap and once with swap. It could be done e.g. by writing the test cases value-parameterized tests and implementing, in the fixture, a method or a pointer to function data-member that points to swap or std::swap depending on the true/false value of the parameter.

Some more minor comments below that would be nice to address before merging.

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 5, 2022

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 5, 2022

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 5, 2022

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 5, 2022

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 5, 2022

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 5, 2022

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

  • [2022-04-05T15:56:04.149Z] C:\build\workspace\root-pullrequests-build\root\math\vecops\inc\ROOT/RVec.hxx(1509,1): error C2955: 'ROOT::VecOps::RVec': use of class template requires template argument list [C:\build\workspace\root-pullrequests-build\build\math\vecops\G__ROOTVecOps.vcxproj]
  • [2022-04-05T15:56:04.149Z] C:\build\workspace\root-pullrequests-build\root\math\vecops\inc\ROOT/RVec.hxx(1509,1): error C2063: 'ROOT::Detail::VecOps::IsSmall': not a function [C:\build\workspace\root-pullrequests-build\build\math\vecops\G__ROOTVecOps.vcxproj]
  • [2022-04-05T15:56:04.149Z] C:\build\workspace\root-pullrequests-build\root\math\vecops\inc\ROOT/RVec.hxx(1511,1): error C2063: 'ROOT::Detail::VecOps::IsAdopting': not a function [C:\build\workspace\root-pullrequests-build\build\math\vecops\G__ROOTVecOps.vcxproj]

@ikabadzhov
[VecOps] Handle swap of small and adopting vectors
bdc2603 
There was one combination of vectors that was braking the Rvec's `swap`
previously - small and adopting vectors. This case is now fixed.
@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 6, 2022

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 6, 2022

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 6, 2022

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

  • [2022-04-06T07:58:33.212Z] C:\build\workspace\root-pullrequests-build\root\math\vecops\inc\ROOT/RVec.hxx(1509,1): error C2955: 'ROOT::VecOps::RVec': use of class template requires template argument list [C:\build\workspace\root-pullrequests-build\build\math\vecops\G__ROOTVecOps.vcxproj]
  • [2022-04-06T07:58:33.212Z] C:\build\workspace\root-pullrequests-build\root\math\vecops\inc\ROOT/RVec.hxx(1509,1): error C2063: 'ROOT::Detail::VecOps::IsSmall': not a function [C:\build\workspace\root-pullrequests-build\build\math\vecops\G__ROOTVecOps.vcxproj]
  • [2022-04-06T07:58:33.212Z] C:\build\workspace\root-pullrequests-build\root\math\vecops\inc\ROOT/RVec.hxx(1511,1): error C2063: 'ROOT::Detail::VecOps::IsAdopting': not a function [C:\build\workspace\root-pullrequests-build\build\math\vecops\G__ROOTVecOps.vcxproj]

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 6, 2022

Build failed on mac11/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

bellenot
bellenot reviewed Apr 6, 2022
View reviewed changes
ikabadzhov and others added 3 commits April 6, 2022 11:18
@ikabadzhov
[VecOps] Add public IsSmall and IsAdopting methods
6eca3da 
Introduce ROOT::Detail::VecOps::IsSmall(v) and ROOT::Detail::VecOps::IsAdopting(v) to check whether an RVec is small and owning its memory respectively.
@ikabadzhov
[VecOps] Add a test suite for possible vector swaps
0b65f3c 
Possible combinations of vectors to swap:
1. small <-> small
2. regular <-> regular (not small, not adopting)
3. adopting <-> adopting
4. small <-> regular (and vice versa)
5. small <-> adopting (and vice versa)
6. regular <-> adopting (and vice versa)
@ikabadzhov @bernhardmgruber
[VecOps] Introduce value parameterized tests for VecOpsSwap
d8628e3 
Added unit test fixture to test both `ROOT::VecOps::swap` and `std::swap`.

Co-authored-by: Bernhard Manfred Gruber <[email protected]>
@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 6, 2022

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Apr 6, 2022

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@eguiraud
Copy link
Copy Markdown
Contributor

eguiraud commented Apr 6, 2022

Note that every time you make a push the CI build is restarted, I'm not sure if the previous windows build keeps running or gets interrupted.

@bellenot
Copy link
Copy Markdown
Member

bellenot commented Apr 6, 2022 •
edited
Loading

Note that every time you make a push the CI build is restarted, I'm not sure if the previous windows build keeps running or gets interrupted.

It gets interrupted and re-started. And note that it is now successful 🙂

@ikabadzhov ikabadzhov merged commit 2605710 into root-project:master Apr 6, 2022
@ikabadzhov ikabadzhov deleted the VecOps_swap_fix branch April 6, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bellenot bellenot bellenot left review comments

+1 more reviewer

@eguiraud eguiraud eguiraud approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ikabadzhov ikabadzhov

Labels

None yet

Projects

None yet

Milestone

6.26/02

Development

Successfully merging this pull request may close these issues.

RVec's swap is broken when RVec is adopting memory

4 participants

@ikabadzhov @phsft-bot @eguiraud @bellenot