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/02/24 23:14:05 UTC
[GitHub] [incubator-mxnet] connorgoggins opened a new pull request #17677:
[Large Tensor] Fix cumsum op
connorgoggins opened a new pull request #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677
## Description ##
The cumsum op was previously breaking on large tensor (dimension > 2^32) data. With the following input:
```
run_performance_test(nd.cumsum, inputs=[{"a": nd.random_normal(shape=(2**32 + 1, 1))}], run_backward=True, warmup=1, runs=1)
```
the following error was thrown:
```
Segmentation fault (core dumped)
```
To root cause this issue, I ran the previous command in a Python script with GDB, and found that the underlying problem was in the data type of several variables in the forward/backward structs in `np_cumsum-inl.h.h`. These variables used the `int` dtype when they should have been using `index_t` to properly handle long int indices. I switched these variables to `index_t` in the struct header and, after rebuilding, the previous input command displayed the correct output:
```
INFO:root:Begin Benchmark - cumsum
INFO:root:Complete Benchmark - cumsum
[{'cumsum': [{'inputs': {'a': '<NDArray 4294967297x1 @cpu(0)>'}, 'max_storage_mem_alloc_cpu/0': 33285996.0, 'avg_time_forward_cumsum': 4366.7148, 'avg_time_backward_cumsum': 12744.9971}]}]
```
To ensure completeness and to prevent future breaking changes, I also added a nightly test for the cumsum op with large tensor data in `tests/nightly/test_large_array.py`.
## Checklist ##
### Essentials ###
Please feel free to remove inapplicable items for your PR.
- [x] Changes are complete (i.e. I finished coding on this PR)
- [x] All changes have test coverage
- [x] Code is well-documented
- [x] To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change
### Changes ###
- M src/operator/numpy/np_cumsum-inl.h
- M tests/nightly/test_large_array.py
## Comments ##
Tested on r5dn.24xl-ubuntu 16.04 and p2.16xl-ubuntu 16.04 with
1. Individual op run
2. Full OpPerf run
## Results ##
The key difference between CPU and GPU tests was the instance type (r5dn.24xl for CPU, p2.16xl for GPU). All relevant build flags remain the same, and both were tested using CPU context.
[Single operator test - cumsum op (GPU)]() - pending
[Single operator test - cumsum op (CPU)](https://gist.github.com/connorgoggins/7d1a5912512b1dbbacff350e3cd576ee)
[Full OpPerf test (GPU)]() - pending
[Full OpPerf test (CPU)]() - pending
@apeforest @access2rohit @ChaiBapchya
----------------------------------------------------------------
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] connorgoggins commented on a change in pull
request #17677: [Large Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
connorgoggins commented on a change in pull request #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677#discussion_r385289461
##########
File path: src/operator/numpy/np_cumsum-inl.h
##########
@@ -157,10 +157,10 @@ void CumsumBackwardImpl(const OpContext& ctx,
}
}
Stream<xpu> *s = ctx.get_stream<xpu>();
- MSHADOW_TYPE_SWITCH_WITH_BOOL(igrad.type_flag_, IType, {
+ MSHADOW_TYPE_SWITCH_WITH_BOOL(igrad.type_flag_, index_t, {
MSHADOW_TYPE_SWITCH(ograd.type_flag_, OType, {
Kernel<cumsum_backward, xpu>::Launch(
- s, igrad.Size() / middle, igrad.dptr<IType>(),
+ s, igrad.Size() / middle, igrad.dptr<index_t>(),
Review comment:
Reverted, thanks!
----------------------------------------------------------------
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] access2rohit commented on a change in pull
request #17677: [Large Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
access2rohit commented on a change in pull request #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677#discussion_r384233496
##########
File path: src/operator/numpy/np_cumsum-inl.h
##########
@@ -60,17 +60,17 @@ struct CumsumParam : public dmlc::Parameter<CumsumParam> {
struct cumsum_forward {
template<typename IType, typename OType>
- MSHADOW_XINLINE static void Map(int i,
+ MSHADOW_XINLINE static void Map(index_t i,
OType *out,
const IType *in,
- const int middle,
- const int trailing) {
- int left = i / trailing, right = i % trailing;
- int offset = left * middle * trailing + right;
+ const index_t middle,
+ const index_t trailing) {
+ index_t left = i / trailing, right = i % trailing;
+ index_t offset = left * middle * trailing + right;
const IType *lane_in = in + offset;
Review comment:
possible values for IType ? Since this can also be an iterator
----------------------------------------------------------------
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] connorgoggins commented on issue #17677: [Large
Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
connorgoggins commented on issue #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677#issuecomment-590597021
@mxnet-label-bot add [pr-awaiting-review]
----------------------------------------------------------------
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] apeforest commented on a change in pull request
#17677: [Large Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
apeforest commented on a change in pull request #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677#discussion_r385284895
##########
File path: src/operator/numpy/np_cumsum-inl.h
##########
@@ -98,11 +98,11 @@ void CumsumForwardImpl(const OpContext& ctx,
}
Stream<xpu> *s = ctx.get_stream<xpu>();
- MSHADOW_TYPE_SWITCH_WITH_BOOL(in.type_flag_, IType, {
+ MSHADOW_TYPE_SWITCH_WITH_BOOL(in.type_flag_, index_t, {
Review comment:
I think you will not need this switch macro, right?
----------------------------------------------------------------
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] apeforest commented on a change in pull request
#17677: [Large Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
apeforest commented on a change in pull request #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677#discussion_r385286229
##########
File path: src/operator/numpy/np_cumsum-inl.h
##########
@@ -157,10 +157,10 @@ void CumsumBackwardImpl(const OpContext& ctx,
}
}
Stream<xpu> *s = ctx.get_stream<xpu>();
- MSHADOW_TYPE_SWITCH_WITH_BOOL(igrad.type_flag_, IType, {
+ MSHADOW_TYPE_SWITCH_WITH_BOOL(igrad.type_flag_, index_t, {
Review comment:
You don't need a macro if the type is predefined.
----------------------------------------------------------------
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] sandeep-krishnamurthy commented on issue #17677:
[Large Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
sandeep-krishnamurthy commented on issue #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677#issuecomment-591547428
1. Are there any performance regression associated with this index type change? (Performance on Before and after the change)
2. Thanks for fix this issue, is there a way to know if there are any other broken operators with similar issue? (Not intended to fix with this PR, but, wanted to see the blast radius and possibly create a tracking issue while we are on it here)
----------------------------------------------------------------
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] access2rohit commented on a change in pull
request #17677: [Large Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
access2rohit commented on a change in pull request #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677#discussion_r385285544
##########
File path: src/operator/numpy/np_cumsum-inl.h
##########
@@ -157,10 +157,10 @@ void CumsumBackwardImpl(const OpContext& ctx,
}
}
Stream<xpu> *s = ctx.get_stream<xpu>();
- MSHADOW_TYPE_SWITCH_WITH_BOOL(igrad.type_flag_, IType, {
+ MSHADOW_TYPE_SWITCH_WITH_BOOL(igrad.type_flag_, index_t, {
Review comment:
Can you revert this change as well ? rest LGTM !
----------------------------------------------------------------
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] apeforest merged pull request #17677: [Large
Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
apeforest merged pull request #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677
----------------------------------------------------------------
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] connorgoggins commented on a change in pull
request #17677: [Large Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
connorgoggins commented on a change in pull request #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677#discussion_r385289205
##########
File path: src/operator/numpy/np_cumsum-inl.h
##########
@@ -98,11 +98,11 @@ void CumsumForwardImpl(const OpContext& ctx,
}
Stream<xpu> *s = ctx.get_stream<xpu>();
- MSHADOW_TYPE_SWITCH_WITH_BOOL(in.type_flag_, IType, {
+ MSHADOW_TYPE_SWITCH_WITH_BOOL(in.type_flag_, index_t, {
Review comment:
Great point, reverted to IType.
----------------------------------------------------------------
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] connorgoggins commented on a change in pull
request #17677: [Large Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
connorgoggins commented on a change in pull request #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677#discussion_r384235914
##########
File path: src/operator/numpy/np_cumsum-inl.h
##########
@@ -60,17 +60,17 @@ struct CumsumParam : public dmlc::Parameter<CumsumParam> {
struct cumsum_forward {
template<typename IType, typename OType>
- MSHADOW_XINLINE static void Map(int i,
+ MSHADOW_XINLINE static void Map(index_t i,
OType *out,
const IType *in,
- const int middle,
- const int trailing) {
- int left = i / trailing, right = i % trailing;
- int offset = left * middle * trailing + right;
+ const index_t middle,
+ const index_t trailing) {
+ index_t left = i / trailing, right = i % trailing;
+ index_t offset = left * middle * trailing + right;
const IType *lane_in = in + offset;
Review comment:
[Here](https://www.cct.lsu.edu/~rguidry/eclipse-doc36/org/eclipse/cdt/core/dom/ast/IType.html) it's defined as an interface for all C and C++ types.
----------------------------------------------------------------
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] access2rohit commented on a change in pull
request #17677: [Large Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
access2rohit commented on a change in pull request #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677#discussion_r385285641
##########
File path: src/operator/numpy/np_cumsum-inl.h
##########
@@ -157,10 +157,10 @@ void CumsumBackwardImpl(const OpContext& ctx,
}
}
Stream<xpu> *s = ctx.get_stream<xpu>();
- MSHADOW_TYPE_SWITCH_WITH_BOOL(igrad.type_flag_, IType, {
+ MSHADOW_TYPE_SWITCH_WITH_BOOL(igrad.type_flag_, index_t, {
MSHADOW_TYPE_SWITCH(ograd.type_flag_, OType, {
Kernel<cumsum_backward, xpu>::Launch(
- s, igrad.Size() / middle, igrad.dptr<IType>(),
+ s, igrad.Size() / middle, igrad.dptr<index_t>(),
Review comment:
Ditto
----------------------------------------------------------------
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] connorgoggins commented on a change in pull
request #17677: [Large Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
connorgoggins commented on a change in pull request #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677#discussion_r385289319
##########
File path: src/operator/numpy/np_cumsum-inl.h
##########
@@ -157,10 +157,10 @@ void CumsumBackwardImpl(const OpContext& ctx,
}
}
Stream<xpu> *s = ctx.get_stream<xpu>();
- MSHADOW_TYPE_SWITCH_WITH_BOOL(igrad.type_flag_, IType, {
+ MSHADOW_TYPE_SWITCH_WITH_BOOL(igrad.type_flag_, index_t, {
Review comment:
Great point, reverted to IType.
----------------------------------------------------------------
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] connorgoggins commented on issue #17677: [Large
Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
connorgoggins commented on issue #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677#issuecomment-591604673
@sandeep-krishnamurthy thanks for your questions!
1. If you look at performance of the `cumsum` op [before](https://gist.github.com/connorgoggins/3fc3c7a40dae8b5cea7f82d9ca4b1a72) vs. [after](https://gist.github.com/connorgoggins/4538df906c3fbe36f989bb9067b8feb8) the changes in this PR, you can see that there is no regression on the time required for a forward pass.
2. My current assignment has been to systematically fix all ops with similar issues (i.e. throwing errors when handling large tensor data). As of now, I have created PRs with fixes for every problematic op, and once all my remaining op fix PRs (3 remaining PRs, including this one) are merged, all similar issues will be resolved.
----------------------------------------------------------------
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] access2rohit commented on a change in pull
request #17677: [Large Tensor] Fix cumsum op
Posted by GitBox <gi...@apache.org>.
access2rohit commented on a change in pull request #17677: [Large Tensor] Fix cumsum op
URL: https://github.com/apache/incubator-mxnet/pull/17677#discussion_r384233536
##########
File path: src/operator/numpy/np_cumsum-inl.h
##########
@@ -125,17 +125,17 @@ void CumsumForward(const nnvm::NodeAttrs& attrs,
struct cumsum_backward {
template<typename IType, typename OType>
- MSHADOW_XINLINE static void Map(int i,
+ MSHADOW_XINLINE static void Map(index_t i,
IType *igrad,
const OType *ograd,
- const int middle,
- const int trailing) {
- int left = i / trailing, right = i % trailing;
- int offset = left * middle * trailing + right;
+ const index_t middle,
+ const index_t trailing) {
+ index_t left = i / trailing, right = i % trailing;
+ index_t offset = left * middle * trailing + right;
const OType *lane_ograd = ograd + offset;
IType *lane_igrad = igrad + offset;
Review comment:
same as above
----------------------------------------------------------------
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