Fix bugs in C API and refactor tests #3350
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The tests do not do cleanup after creating CAPI pipeline, leaving live pipeline to be cleaned up by process shutdown. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown (as it was using async execution) and the whole test process could crash during shutdown. The read outputs are also not released properly after the last output is accessed. This is a bit awkward part of C API. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
|
!build |
|
CI MESSAGE: [3004657]: BUILD STARTED |
JanuszL
reviewed
Sep 17, 2021
dali/c_api/c_api_test.cc
Outdated
| for (int i = 0; i < prefetch_queue_depth; i++) { | ||
| data.push_back(AllocBuffer<TypeParam>(num_elems * sizeof(uint8_t), false)); | ||
| } | ||
| std::vector<TensorList<TypeParam>> input_wrapper(3); |
There was a problem hiding this comment.
Maybe it is better to use TensorVector unless TensorList is the data structure we are reworking towards?
There was a problem hiding this comment.
Hmm, it should be
Suggested change
| std::vector<TensorList<TypeParam>> input_wrapper(3); | |
| std::vector<TensorList<TypeParam>> input_wrapper(prefetch_queue_depth); |
but other then that I just kept the TensorList. What we will use is up to be decided, but I've heard voices to keep the TensorList to match what we have in Python. But we will see.
|
CI MESSAGE: [3004657]: BUILD PASSED |
JanuszL
approved these changes
Sep 20, 2021
mzient
reviewed
Sep 20, 2021
dali/c_api/c_api.cc
Outdated
| pipe_handle->ws = ws.release(); | ||
| pipe_handle->copy_stream = stream.release(); | ||
| pipe_handle->pipe = pipeline.release(); | ||
| pipe_handle->batch_sizes_map = bs_map.release(); |
There was a problem hiding this comment.
Suggested change
| pipe_handle->batch_sizes_map = bs_map.release(); | |
| pipe_handle->batch_size_map = bs_map.release(); |
mzient
reviewed
Sep 20, 2021
dali/c_api/c_api.cc
Outdated
| pipe_handle->ws = ws.release(); | ||
| pipe_handle->copy_stream = stream.release(); | ||
| pipe_handle->pipe = pipeline.release(); | ||
|
|
||
| auto bs_map = std::make_unique<batch_size_map_t>(); | ||
| pipe_handle->batch_sizes_map = bs_map.release(); |
There was a problem hiding this comment.
Suggested change
| pipe_handle->batch_sizes_map = bs_map.release(); | |
| pipe_handle->batch_size_map = bs_map.release(); |
mzient
approved these changes
Sep 20, 2021
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
|
!build |
|
CI MESSAGE: [3017866]: BUILD STARTED |
|
CI MESSAGE: [3017866]: BUILD PASSED |
jantonguirao
added
the
important-fix
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Oct 17, 2021
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jan 23, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jan 23, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jan 23, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jan 23, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jan 23, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jan 23, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jan 23, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jan 23, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jan 23, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jan 23, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jan 23, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jan 23, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jan 23, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Feb 21, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
May 13, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
cyyever
pushed a commit
to cyyever/DALI
that referenced
this pull request
Jun 7, 2022
Fix `daliDeserializeDefault` - the batch size map was not allocated and when the pipeline handle was correctly cleaned, it tried to deallocate memory under invalid pointer. Add missing `daliDeletePipeline` in all C API tests so the tests no longer leak the created pipeline. Some tests (daliOutputCopySamples) used to fill the prefetch queue and schedule additional runs, whilst accessing only one output iteration. The not-freed pipeline could be actively using the memory when the memory resources were cleared on shutdown and the whole test process could crash during shutdown. Remove the additional unused iteration in such case. Rework tests to not use access to underlying contiguous buffer of TensorList. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
What happened in this PR
Fix
daliDeserializeDefault- the batch sizemap was not allocated and when the pipeline
handle was correctly cleaned, it tried to
deallocate memory under invalid pointer.
Add missing
daliDeletePipelinein all C API testsso the tests no longer leak the created pipeline.
Some tests (daliOutputCopySamples) used to fill the
prefetch queue and schedule additional runs,
whilst accessing only one output iteration.
The not-freed pipeline could be actively using the memory
when the memory resources were cleared on shutdown
and the whole test process could crash during shutdown.
Remove the additional unused iteration in such case.
Rework tests to not use access to underlying
contiguous buffer of TensorList.
Additional information
Affected modules and functionalities:
C API
Key points relevant for the review:
Checklist
Tests
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A