You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/01/06 23:42:50 UTC
[GitHub] [incubator-mxnet] aws-taylor opened a new pull request #17230:
Additional fix for vector access.
aws-taylor opened a new pull request #17230: Additional fix for vector access.
URL: https://github.com/apache/incubator-mxnet/pull/17230
See https://github.com/apache/incubator-mxnet/commit/9634786f96388004f68c223d72e120ad425c2f12 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] wkcn commented on a change in pull request
#17230: Additional fix for vector access.
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #17230: Additional fix for vector access.
URL: https://github.com/apache/incubator-mxnet/pull/17230#discussion_r363559546
##########
File path: 3rdparty/mshadow/mshadow/dot_engine-inl.h
##########
@@ -421,12 +421,9 @@ struct BLASEngine<cpu, double> {
CBLAS_TRANSPOSE p_transa[GROUP_SIZE] = {cblas_a_trans};
CBLAS_TRANSPOSE p_transb[GROUP_SIZE] = {cblas_b_trans};
- std::vector<const double*> pp_A;
- std::vector<const double*> pp_B;
- std::vector<double*> pp_C;
- pp_A.reserve(batch_count);
- pp_B.reserve(batch_count);
- pp_C.reserve(batch_count);
+ std::vector<const double*> pp_A(batch_count, nullptr);
Review comment:
Thank you for the fix!
It will be faster to use `std::vector<const double*> pp_A(batch_count);` : )
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] wkcn commented on issue #17230: Additional fix
for vector access.
Posted by GitBox <gi...@apache.org>.
wkcn commented on issue #17230: Additional fix for vector access.
URL: https://github.com/apache/incubator-mxnet/pull/17230#issuecomment-586722380
The PR has been merged. Thank you!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] wkcn commented on a change in pull request
#17230: Additional fix for vector access.
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #17230: Additional fix for vector access.
URL: https://github.com/apache/incubator-mxnet/pull/17230#discussion_r363559546
##########
File path: 3rdparty/mshadow/mshadow/dot_engine-inl.h
##########
@@ -421,12 +421,9 @@ struct BLASEngine<cpu, double> {
CBLAS_TRANSPOSE p_transa[GROUP_SIZE] = {cblas_a_trans};
CBLAS_TRANSPOSE p_transb[GROUP_SIZE] = {cblas_b_trans};
- std::vector<const double*> pp_A;
- std::vector<const double*> pp_B;
- std::vector<double*> pp_C;
- pp_A.reserve(batch_count);
- pp_B.reserve(batch_count);
- pp_C.reserve(batch_count);
+ std::vector<const double*> pp_A(batch_count, nullptr);
Review comment:
Thank you for the fix!
~~It will be better to use `std::array`, since `std::vector` will allocate the memory, which is greater than `batch_count`.~~
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] wkcn commented on issue #17230: Additional fix
for vector access.
Posted by GitBox <gi...@apache.org>.
wkcn commented on issue #17230: Additional fix for vector access.
URL: https://github.com/apache/incubator-mxnet/pull/17230#issuecomment-586662544
Hi @aws-taylor , I could not retrigger the CI.
Could you please retrigger it (push an empty commit)? Thank you!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] wkcn merged pull request #17230: Additional fix
for vector access.
Posted by GitBox <gi...@apache.org>.
wkcn merged pull request #17230: Additional fix for vector access.
URL: https://github.com/apache/incubator-mxnet/pull/17230
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] aws-taylor commented on a change in pull request
#17230: Additional fix for vector access.
Posted by GitBox <gi...@apache.org>.
aws-taylor commented on a change in pull request #17230: Additional fix for vector access.
URL: https://github.com/apache/incubator-mxnet/pull/17230#discussion_r363880914
##########
File path: 3rdparty/mshadow/mshadow/dot_engine-inl.h
##########
@@ -421,12 +421,9 @@ struct BLASEngine<cpu, double> {
CBLAS_TRANSPOSE p_transa[GROUP_SIZE] = {cblas_a_trans};
CBLAS_TRANSPOSE p_transb[GROUP_SIZE] = {cblas_b_trans};
- std::vector<const double*> pp_A;
- std::vector<const double*> pp_B;
- std::vector<double*> pp_C;
- pp_A.reserve(batch_count);
- pp_B.reserve(batch_count);
- pp_C.reserve(batch_count);
+ std::vector<const double*> pp_A(batch_count, nullptr);
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] wkcn commented on a change in pull request
#17230: Additional fix for vector access.
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #17230: Additional fix for vector access.
URL: https://github.com/apache/incubator-mxnet/pull/17230#discussion_r363559546
##########
File path: 3rdparty/mshadow/mshadow/dot_engine-inl.h
##########
@@ -421,12 +421,9 @@ struct BLASEngine<cpu, double> {
CBLAS_TRANSPOSE p_transa[GROUP_SIZE] = {cblas_a_trans};
CBLAS_TRANSPOSE p_transb[GROUP_SIZE] = {cblas_b_trans};
- std::vector<const double*> pp_A;
- std::vector<const double*> pp_B;
- std::vector<double*> pp_C;
- pp_A.reserve(batch_count);
- pp_B.reserve(batch_count);
- pp_C.reserve(batch_count);
+ std::vector<const double*> pp_A(batch_count, nullptr);
Review comment:
Thank you for the fix!
It will be better to use `std::array`, since `std::vector` will allocate the memory, which is greater than `batch_count`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] wkcn edited a comment on issue #17230: Additional
fix for vector access.
Posted by GitBox <gi...@apache.org>.
wkcn edited a comment on issue #17230: Additional fix for vector access.
URL: https://github.com/apache/incubator-mxnet/pull/17230#issuecomment-586662544
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services