Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Additional fix for vector access. #17230

Merged
merged 6 commits into from
Feb 16, 2020
Merged

Additional fix for vector access. #17230

merged 6 commits into from
Feb 16, 2020

Conversation

aws-taylor
Copy link
Contributor

See 9634786 for the original.

Description

Fixes an assertion thrown by operator[] of std::vector when MxNet is compiled with certain STL hardening flags, especially since GCC 8. This pull request is to fix a small block that was missed in the original PR.

pp_A.reserve(batch_count);
pp_B.reserve(batch_count);
pp_C.reserve(batch_count);
std::vector<const double*> pp_A(batch_count, nullptr);
Copy link
Member

@wkcn wkcn Jan 7, 2020

Choose a reason for hiding this comment

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

Thank you for the fix!
It will be faster to use std::vector<const double*> pp_A(batch_count); : )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this because it's precisely the same logic used by the earlier commit.

That being said, I believe the STL guarantees that non-class types will be zero initialized when using the constructor form you propose, so I would expect the performance of the two forms to be identical.

Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@wkcn
Copy link
Member

wkcn commented Feb 16, 2020

Hi @aws-taylor , I could not retrigger the CI.
Could you please retrigger it (push an empty commit)?

Thank you!

--edit:
I have retriggered it.

@wkcn wkcn added the pr-awaiting-review PR is waiting for code review label Feb 16, 2020
Copy link
Member

@TaoLv TaoLv left a comment

Choose a reason for hiding this comment

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

Thank you for the fix! Sorry I missed the double precision part in previous commit.

@wkcn wkcn merged commit 7743fb0 into apache:master Feb 16, 2020
@wkcn
Copy link
Member

wkcn commented Feb 16, 2020

The PR has been merged. Thank you!

zheyuye pushed a commit to zheyuye/incubator-mxnet that referenced this pull request Feb 19, 2020
* Additional fix for vector access. See apache@9634786 for the original.

* CI

* ci

* ci

* retrigger CI

* ci

Co-authored-by: JackieWu <[email protected]>
anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request May 29, 2020
* Additional fix for vector access. See apache@9634786 for the original.

* CI

* ci

* ci

* retrigger CI

* ci

Co-authored-by: JackieWu <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants