Fix storage of non-POD random distributions.

[mzient]

Category:

Bug fix (non-breaking change which fixes an issue)

Description:

Storing dists_cpu_ in a byte buffer results in a resource leak and other potential problems in case of non-trivially-copyable and non-trivially-destructible distributions.

Additional information:

Affected modules and functionalities:

Random number generators.

Key points relevant for the review:

Tests:

test_random_choice
test_uniform
test_noise_gaussian

The fix was validated by running a selected test form test_random_choice with valgrind.
CI validation happens in a sanitizer build.

• Existing tests apply
• New tests added
  • Python tests
  • GTests
  • Benchmark
  • Other
• N/A

Checklist

Documentation

• Existing documentation applies
• Documentation updated
  • Docstring
  • Doxygen
  • RST
  • Jupyter
  • Other
• N/A

DALI team only

Requirements

• Implements new requirements
• Affects existing requirements
• N/A

REQ IDs: N/A

JIRA TASK: N/A

[dali-automaton]

CI MESSAGE: [13803979]: BUILD STARTED

[dali-automaton]

CI MESSAGE: [13803979]: BUILD PASSED

[klecki]

CI validation happens in a sanitizer build.

Did you trigger the sanitizer build?

[mzient]

CI validation happens in a sanitizer build.

Did you trigger the sanitizer build?

There's no need, strictly speaking. Valgrind confirmed the former leak, which disappeared after the fix. We'll get a confirmation in a nightly.

[klecki]

CI validation happens in a sanitizer build.

Did you trigger the sanitizer build?

There's no need, strictly speaking. Valgrind confirmed the former leak, which disappeared after the fix. We'll get a confirmation in a nightly.

I would say that if you are posting a fix, you should confirm that it works in CI before merging. If the problem was in L1 test, you would trigger the job rather than wait for the nightly to see if it worked, right?

[mzient]

CI validation happens in a sanitizer build.

Did you trigger the sanitizer build?

There's no need, strictly speaking. Valgrind confirmed the former leak, which disappeared after the fix. We'll get a confirmation in a nightly.

I would say that if you are posting a fix, you should confirm that it works in CI before merging. If the problem was in L1 test, you would trigger the job rather than wait for the nightly to see if it worked, right?

That's one way of looking at it. The way I see it, I'm not fixing CI. I'm fixing a memory leak / undefined behavior / ... I'm confident that the tools I used to verify the fix are sufficient. If you feel insecure about this code, feel free to trigger the build. It's not a mandatory part of the team-approved merging process.

[klecki]

CI validation happens in a sanitizer build.

Did you trigger the sanitizer build?

There's no need, strictly speaking. Valgrind confirmed the former leak, which disappeared after the fix. We'll get a confirmation in a nightly.

I would say that if you are posting a fix, you should confirm that it works in CI before merging. If the problem was in L1 test, you would trigger the job rather than wait for the nightly to see if it worked, right?

That's one way of looking at it. The way I see it, I'm not fixing CI. I'm fixing a memory leak / undefined behavior / ... I'm confident that the tools I used to verify the fix are sufficient. If you feel insecure about this code, feel free to trigger the build.

If you are fixing a specific test, it is a normal practice to run the CI for that test, so all reviewers can see that it works. And I would consider that author responsibility. We have tools for that rather than promising that it works.

It's not a mandatory part of the team-approved merging process.

"A full CI build has been run for the latest commit submitted and all the tests are passing."

[dali-automaton]

CI MESSAGE: [13827654]: BUILD STARTED

[klecki]

I triggered the sanitizer build.

[mzient]

I triggered the sanitizer build.

The relevant job https://gitlab-master.nvidia.com/dl/dali/dali-gh-mirror/-/jobs/87208338 has passed; killing the rest.