[jira] [Commented] (ARROW-2400) [C++] Status destructor is expensive
[ https://issues.apache.org/jira/browse/ARROW-2400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16522344#comment-16522344 ] Antoine Pitrou commented on ARROW-2400: --- > Apache Kudu has a modified version of Status with various interesting > tidbits, if interested you may have a look to tinker > [https://github.com/apache/kudu/blob/master/src/kudu/util/status.h] Interesting. It doesn't seem conceptually different, except that they have micro-optimized the state encoding, even though it's only used on error. I don't think it's worth the effort. > [C++] Status destructor is expensive > > > Key: ARROW-2400 > URL: https://issues.apache.org/jira/browse/ARROW-2400 > Project: Apache Arrow > Issue Type: Improvement >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Assignee: Antoine Pitrou >Priority: Major > Labels: pull-request-available > Fix For: 0.10.0 > > Time Spent: 1h 20m > Remaining Estimate: 0h > > Let's take the following micro-benchmark (in Python): > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 784 usec per loop > {code} > If I replace the Status destructor with a no-op: > {code:c++} > ~Status() { } > {code} > then the benchmark result becomes: > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 561 usec per loop > {code} > This is almost a 30% win. I get similar results on the conversion benchmarks > in the benchmark suite. > I'm unsure about the explanation. In the common case, {{delete _state}} > should be extremely fast, since the state is NULL. Yet, it seems it adds > significant overhead. Perhaps because of exception handling? -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2400) [C++] Status destructor is expensive
[ https://issues.apache.org/jira/browse/ARROW-2400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16520991#comment-16520991 ] Wes McKinney commented on ARROW-2400: - Apache Kudu has a modified version of Status with various interesting tidbits, if interested you may have a look to tinker https://github.com/apache/kudu/blob/master/src/kudu/util/status.h > [C++] Status destructor is expensive > > > Key: ARROW-2400 > URL: https://issues.apache.org/jira/browse/ARROW-2400 > Project: Apache Arrow > Issue Type: Improvement >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Priority: Major > Labels: pull-request-available > Time Spent: 1h 10m > Remaining Estimate: 0h > > Let's take the following micro-benchmark (in Python): > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 784 usec per loop > {code} > If I replace the Status destructor with a no-op: > {code:c++} > ~Status() { } > {code} > then the benchmark result becomes: > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 561 usec per loop > {code} > This is almost a 30% win. I get similar results on the conversion benchmarks > in the benchmark suite. > I'm unsure about the explanation. In the common case, {{delete _state}} > should be extremely fast, since the state is NULL. Yet, it seems it adds > significant overhead. Perhaps because of exception handling? -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2400) [C++] Status destructor is expensive
[ https://issues.apache.org/jira/browse/ARROW-2400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16520272#comment-16520272 ] Antoine Pitrou commented on ARROW-2400: --- Nothing yet :) I think we should also experiment with {{noexcept}} here (apparently it can have non-trivial performance implications). > [C++] Status destructor is expensive > > > Key: ARROW-2400 > URL: https://issues.apache.org/jira/browse/ARROW-2400 > Project: Apache Arrow > Issue Type: Improvement >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Priority: Major > > Let's take the following micro-benchmark (in Python): > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 784 usec per loop > {code} > If I replace the Status destructor with a no-op: > {code:c++} > ~Status() { } > {code} > then the benchmark result becomes: > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 561 usec per loop > {code} > This is almost a 30% win. I get similar results on the conversion benchmarks > in the benchmark suite. > I'm unsure about the explanation. In the common case, {{delete _state}} > should be extremely fast, since the state is NULL. Yet, it seems it adds > significant overhead. Perhaps because of exception handling? -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2400) [C++] Status destructor is expensive
[ https://issues.apache.org/jira/browse/ARROW-2400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16520268#comment-16520268 ] Dimitri Vorona commented on ARROW-2400: --- What did actually happen of it? 200ns per command saved by a 3 lines change would seem like a win to me. > [C++] Status destructor is expensive > > > Key: ARROW-2400 > URL: https://issues.apache.org/jira/browse/ARROW-2400 > Project: Apache Arrow > Issue Type: Improvement >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Priority: Major > > Let's take the following micro-benchmark (in Python): > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 784 usec per loop > {code} > If I replace the Status destructor with a no-op: > {code:c++} > ~Status() { } > {code} > then the benchmark result becomes: > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 561 usec per loop > {code} > This is almost a 30% win. I get similar results on the conversion benchmarks > in the benchmark suite. > I'm unsure about the explanation. In the common case, {{delete _state}} > should be extremely fast, since the state is NULL. Yet, it seems it adds > significant overhead. Perhaps because of exception handling? -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2400) [C++] Status destructor is expensive
[ https://issues.apache.org/jira/browse/ARROW-2400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16430414#comment-16430414 ] Antoine Pitrou commented on ARROW-2400: --- The change doesn't seem to make any difference with MSVC 19.13 (VS 2017 Community Edition). > [C++] Status destructor is expensive > > > Key: ARROW-2400 > URL: https://issues.apache.org/jira/browse/ARROW-2400 > Project: Apache Arrow > Issue Type: Improvement >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Priority: Major > > Let's take the following micro-benchmark (in Python): > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 784 usec per loop > {code} > If I replace the Status destructor with a no-op: > {code:c++} > ~Status() { } > {code} > then the benchmark result becomes: > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 561 usec per loop > {code} > This is almost a 30% win. I get similar results on the conversion benchmarks > in the benchmark suite. > I'm unsure about the explanation. In the common case, {{delete _state}} > should be extremely fast, since the state is NULL. Yet, it seems it adds > significant overhead. Perhaps because of exception handling? -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2400) [C++] Status destructor is expensive
[ https://issues.apache.org/jira/browse/ARROW-2400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16426908#comment-16426908 ] Dimitri Vorona commented on ARROW-2400: --- With the predict patch (https://gist.github.com/alendit/73ec62887d00fb44ef32e8f8eec41af3) {noformat} Run on (4 X 3200 MHz CPU s) 2018-04-05 15:16:29 Benchmark Time CPU Iterations BM_ArrowStatus 657 ns 654 ns 1037970 BM_OwnCopyStatus 657 ns 655 ns 1080042 BM_NoOpStatus330 ns 329 ns 2131999 BM_PredictStatus 448 ns 446 ns 1642423{noformat} > [C++] Status destructor is expensive > > > Key: ARROW-2400 > URL: https://issues.apache.org/jira/browse/ARROW-2400 > Project: Apache Arrow > Issue Type: Improvement >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Priority: Major > > Let's take the following micro-benchmark (in Python): > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 784 usec per loop > {code} > If I replace the Status destructor with a no-op: > {code:c++} > ~Status() { } > {code} > then the benchmark result becomes: > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 561 usec per loop > {code} > This is almost a 30% win. I get similar results on the conversion benchmarks > in the benchmark suite. > I'm unsure about the explanation. In the common case, {{delete _state}} > should be extremely fast, since the state is NULL. Yet, it seems it adds > significant overhead. Perhaps because of exception handling? -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2400) [C++] Status destructor is expensive
[ https://issues.apache.org/jira/browse/ARROW-2400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16426848#comment-16426848 ] Antoine Pitrou commented on ARROW-2400: --- This patch seems to improve things by ~15% here on gcc 4.9: {code:c++} diff --git a/cpp/src/arrow/status.h b/cpp/src/arrow/status.h index 84f55e4..a2cf438 100644 --- a/cpp/src/arrow/status.h +++ b/cpp/src/arrow/status.h @@ -107,7 +107,11 @@ class ARROW_EXPORT Status { public: // Create a success status. Status() : state_(NULL) {} - ~Status() { delete state_; } + ~Status() { +if (ARROW_PREDICT_FALSE(state_ != NULL)) { + DeleteState(); +} + } Status(StatusCode code, const std::string& msg); @@ -207,6 +211,10 @@ class ARROW_EXPORT Status { // a `State` structure containing the error code and message(s) State* state_; + void DeleteState() { +delete state_; +state_ = NULL; + } void CopyFrom(const State* s); }; {code} > [C++] Status destructor is expensive > > > Key: ARROW-2400 > URL: https://issues.apache.org/jira/browse/ARROW-2400 > Project: Apache Arrow > Issue Type: Improvement >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Priority: Major > > Let's take the following micro-benchmark (in Python): > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 784 usec per loop > {code} > If I replace the Status destructor with a no-op: > {code:c++} > ~Status() { } > {code} > then the benchmark result becomes: > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 561 usec per loop > {code} > This is almost a 30% win. I get similar results on the conversion benchmarks > in the benchmark suite. > I'm unsure about the explanation. In the common case, {{delete _state}} > should be extremely fast, since the state is NULL. Yet, it seems it adds > significant overhead. Perhaps because of exception handling? -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2400) [C++] Status destructor is expensive
[ https://issues.apache.org/jira/browse/ARROW-2400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16426845#comment-16426845 ] Dimitri Vorona commented on ARROW-2400: --- CompilerExplorer for clang: [https://godbolt.org/g/kx1pnp] and GCC: https://godbolt.org/g/cCnaWc > [C++] Status destructor is expensive > > > Key: ARROW-2400 > URL: https://issues.apache.org/jira/browse/ARROW-2400 > Project: Apache Arrow > Issue Type: Improvement >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Priority: Major > > Let's take the following micro-benchmark (in Python): > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 784 usec per loop > {code} > If I replace the Status destructor with a no-op: > {code:c++} > ~Status() { } > {code} > then the benchmark result becomes: > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 561 usec per loop > {code} > This is almost a 30% win. I get similar results on the conversion benchmarks > in the benchmark suite. > I'm unsure about the explanation. In the common case, {{delete _state}} > should be extremely fast, since the state is NULL. Yet, it seems it adds > significant overhead. Perhaps because of exception handling? -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2400) [C++] Status destructor is expensive
[ https://issues.apache.org/jira/browse/ARROW-2400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16426774#comment-16426774 ] Dimitri Vorona commented on ARROW-2400: --- Hi Antoine, this is something I've noticed before, but didn't find any time to investigate. To localize the problem I've created a gist: [https://gist.github.com/alendit/73ec62887d00fb44ef32e8f8eec41af3] I get similar numbers (both with -O3): clang++ version 6.0.0 {{Run on (4 X 3200 MHz CPU s)}} {{2018-04-05 13:29:13}} {{}} {{Benchmark Time CPU Iterations}} {{}} {{BM_ArrowStatus 885 ns 878 ns 735896}} {{BM_OwnCopyStatus 897 ns 893 ns 780257}} {{BM_NoOpStatus 884 ns 879 ns 785719}} g++ 7.3.1 {{Run on (4 X 3200 MHz CPU s)}} {{2018-04-05 13:29:28}} {{}} {{Benchmark Time CPU Iterations}} {{}} {{BM_ArrowStatus 818 ns 813 ns 799285}} {{BM_OwnCopyStatus 865 ns 855 ns 785573}} {{BM_NoOpStatus 423 ns 421 ns 1734452}} So apparently there is an optimization which GCC can perform if the destructor is strictly noop. I'd have to test further to localize the problem. A look into the generated code might be worth it. Cheers! > [C++] Status destructor is expensive > > > Key: ARROW-2400 > URL: https://issues.apache.org/jira/browse/ARROW-2400 > Project: Apache Arrow > Issue Type: Improvement >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Priority: Major > > Let's take the following micro-benchmark (in Python): > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 784 usec per loop > {code} > If I replace the Status destructor with a no-op: > {code:c++} > ~Status() { } > {code} > then the benchmark result becomes: > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 561 usec per loop > {code} > This is almost a 30% win. I get similar results on the conversion benchmarks > in the benchmark suite. > I'm unsure about the explanation. In the common case, {{delete _state}} > should be extremely fast, since the state is NULL. Yet, it seems it adds > significant overhead. Perhaps because of exception handling? -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2400) [C++] Status destructor is expensive
[ https://issues.apache.org/jira/browse/ARROW-2400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16426749#comment-16426749 ] Antoine Pitrou commented on ARROW-2400: --- The figures above were with gcc 4.9. If I switch to clang 5.0 the slowdown is much smaller, but still exists: * with the normal destructor: {code:shell} $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in range(1)]" "pa.array(data, type=pa.binary())" 1000 loops, best of 3: 546 usec per loop {code} * with an empty destructor: {code:shell} $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in range(1)]" "pa.array(data, type=pa.binary())" 1000 loops, best of 3: 520 usec per loop {code} > [C++] Status destructor is expensive > > > Key: ARROW-2400 > URL: https://issues.apache.org/jira/browse/ARROW-2400 > Project: Apache Arrow > Issue Type: Improvement >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Priority: Major > > Let's take the following micro-benchmark (in Python): > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 784 usec per loop > {code} > If I replace the Status destructor with a no-op: > {code:c++} > ~Status() { } > {code} > then the benchmark result becomes: > {code:bash} > $ python -m timeit -s "import pyarrow as pa; data = [b'xx' for i in > range(1)]" "pa.array(data, type=pa.binary())" > 1000 loops, best of 3: 561 usec per loop > {code} > This is almost a 30% win. I get similar results on the conversion benchmarks > in the benchmark suite. > I'm unsure about the explanation. In the common case, {{delete _state}} > should be extremely fast, since the state is NULL. Yet, it seems it adds > significant overhead. Perhaps because of exception handling? -- This message was sent by Atlassian JIRA (v7.6.3#76005)