You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/05/24 02:04:35 UTC
[GitHub] [arrow] westonpace opened a new pull request, #13222: MINOR: [C++] Move static declaration to non-static declaration to improve performance
westonpace opened a new pull request, #13222:
URL: https://github.com/apache/arrow/pull/13222
According to conbench there was a slight regression on #12957 . Poking around a bit it seems that a static local variable is implemented using some kind of global lock (__cxa_guard_acquire / __cxa_guard_release). On the other hand, constructing an empty shared_ptr is cheap (two pointers are set to 0). So if we care about performance here we probably don't want `static`. This may be what is causing the conbench issue.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #13222: MINOR: [C++] Move static declaration to non-static declaration to improve performance
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #13222:
URL: https://github.com/apache/arrow/pull/13222#issuecomment-1135319448
@ursabot please benchmark lang=R
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 merged pull request #13222: MINOR: [C++] Move static declaration to non-static declaration to improve performance
Posted by GitBox <gi...@apache.org>.
cyb70289 merged PR #13222:
URL: https://github.com/apache/arrow/pull/13222
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #13222: MINOR: [C++] Move static declaration to non-static declaration to improve performance
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #13222:
URL: https://github.com/apache/arrow/pull/13222#issuecomment-1136184481
@ursabot please benchmark lang=R
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13222: MINOR: [C++] Move static declaration to non-static declaration to improve performance
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13222:
URL: https://github.com/apache/arrow/pull/13222#issuecomment-1141322620
Rebased and applied suggestion of using a module-private variable.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on pull request #13222: MINOR: [C++] Move static declaration to non-static declaration to improve performance
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on PR #13222:
URL: https://github.com/apache/arrow/pull/13222#issuecomment-1135425796
appveyor test failure may deserve deeper investigation, though unlikely related to this pr:
https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/43639301/job/n3qanqaqw8xdrcij#L2236
```
23/94 Test #23: arrow-threading-utility-test ..............***Failed 27.32 sec
Running main() from D:\bld\gtest_1647154904782\work\googletest\src\gtest_main.cc
[==========] Running 140 tests from 24 test suites.
[----------] Global test environment set-up.
[----------] 7 tests from CancelTest
[ RUN ] CancelTest.StopBasics
[ OK ] CancelTest.StopBasics (0 ms)
[ RUN ] CancelTest.StopTokenCopy
[ OK ] CancelTest.StopTokenCopy (0 ms)
[ RUN ] CancelTest.RequestStopTwice
[ OK ] CancelTest.RequestStopTwice (0 ms)
[ RUN ] CancelTest.Unstoppable
[ OK ] CancelTest.Unstoppable (0 ms)
[ RUN ] CancelTest.SourceVanishes
[ OK ] CancelTest.SourceVanishes (0 ms)
[ RUN ] CancelTest.ThreadedPollSuccess
[ OK ] CancelTest.ThreadedPollSuccess (8332 ms)
[ RUN ] CancelTest.ThreadedPollCancel
[ OK ] CancelTest.ThreadedPollCancel (7768 ms)
[----------] 7 tests from CancelTest (16100 ms total)
[----------] 2 tests from SignalCancelTest
[ RUN ] SignalCancelTest.Register
[ OK ] SignalCancelTest.Register (0 ms)
[ RUN ] SignalCancelTest.RegisterUnregister
C:/projects/arrow/cpp/src/arrow/util/cancel_test.cc(191): error: Value of: stop_token_->IsStopRequested()
Actual: true
Expected: false
C:/projects/arrow/cpp/src/arrow/util/cancel_test.cc(191): error: Value of: stop_token_->IsStopRequested()
Actual: true
Expected: false
[ FAILED ] SignalCancelTest.RegisterUnregister (22 ms)
[----------] 2 tests from SignalCancelTest (22 ms total)
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #13222: MINOR: [C++] Move static declaration to non-static declaration to improve performance
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #13222:
URL: https://github.com/apache/arrow/pull/13222#issuecomment-1135319489
Benchmark runs are scheduled for baseline = 43a604de6b44394f99900a3ae9e869d3c11670eb and contender = 9ab685a2735e8b874aec866075ce9054ab1fe6f2. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Only ['Python'] langs are supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/a0d6ade37ca24700aa6a94fa7e877861...4df92b0934ab4c80a33b007025aeb6f1/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/a4ac04d8c6124a409192eadedc067e78...b32e1c4c75ed419aa929775092cee4a1/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/dcc25a3f7e3d4967849a34852f9771c6...9ed070c064f54dc9856d110d23f74f76/)
[Skipped :warning: Only ['C++', 'Java'] langs are supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a5d67b43dd0c4042adc53da4faec8ca8...9ebab5a6809f4902b1b90017b5c9b8d6/)
Buildkite builds:
[Scheduled] [`9ab685a2` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/810)
[Scheduled] [`9ab685a2` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/800)
[Finished] [`43a604de` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/812)
[Scheduled] [`43a604de` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/809)
[Scheduled] [`43a604de` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/799)
[Scheduled] [`43a604de` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/815)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #13222: MINOR: [C++] Move static declaration to non-static declaration to improve performance
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #13222:
URL: https://github.com/apache/arrow/pull/13222#issuecomment-1141322793
Benchmark runs are scheduled for baseline = 01d8485d17adacbe75dac2a9b97c34dc28ca31f5 and contender = e28ce2acda74e2e06eb1d5dd4c10f5205b20500a. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Only ['Python'] langs are supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/2319c8a037064c1b8940d6ac7883d2e7...6982f805ab904939ad9279d8c595f5d8/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/ac195ab5c53a4232a600bbf3f4e24d33...e78feef57e8c442d824169e45a6b2b7e/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/bffab965eb3e4658acbfcee9cf821839...e7c43de5d23647bc842c1148b0e985f4/)
[Skipped :warning: Only ['C++', 'Java'] langs are supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/9df7c51f54174addb662ecc46344342d...5ba1cf31c4c040489524de61c5065544/)
Buildkite builds:
[Scheduled] [`e28ce2ac` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/847)
[Scheduled] [`e28ce2ac` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/836)
[Finished] [`01d8485d` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/847)
[Scheduled] [`01d8485d` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/845)
[Scheduled] [`01d8485d` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/834)
[Scheduled] [`01d8485d` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/848)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13222: MINOR: [C++] Move static declaration to non-static declaration to improve performance
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13222:
URL: https://github.com/apache/arrow/pull/13222#issuecomment-1141322695
@ursabot please benchmark lang=R
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a diff in pull request #13222: MINOR: [C++] Move static declaration to non-static declaration to improve performance
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on code in PR #13222:
URL: https://github.com/apache/arrow/pull/13222#discussion_r880034314
##########
cpp/src/arrow/compute/exec/expression.cc:
##########
@@ -107,9 +107,10 @@ ValueDescr Expression::descr() const {
return CallNotNull(*this)->descr;
}
+const std::shared_ptr<DataType> kNoType;
Review Comment:
Nit: wrap it inside an anonymous namespace, or as static global?
Did a quick comparison of `local static` and `global`. Looks for normal case (not the first time access which is complex due to thread safety), `local static` has the penalty of additional load-compare-cond_jump. Guess it may cause observable effect on microbenchmark.
https://godbolt.org/z/cxWPhccnx
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #13222: MINOR: [C++] Move static declaration to non-static declaration to improve performance
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #13222:
URL: https://github.com/apache/arrow/pull/13222#issuecomment-1136184623
Benchmark runs are scheduled for baseline = 43a604de6b44394f99900a3ae9e869d3c11670eb and contender = d00b684456cb25d992b08bbbfda23f40b19f7d69. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Only ['Python'] langs are supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/a0d6ade37ca24700aa6a94fa7e877861...bbd9a539a87e41b2b6694af1b423c80f/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/a4ac04d8c6124a409192eadedc067e78...13c4c1e881fc4d64b0260e022d2a3b85/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/dcc25a3f7e3d4967849a34852f9771c6...bdd478da7a3d42c3ac405203a3eff826/)
[Skipped :warning: Only ['C++', 'Java'] langs are supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a5d67b43dd0c4042adc53da4faec8ca8...dcb89debd3ac45fb85880cd2083df0a7/)
Buildkite builds:
[Scheduled] [`d00b6844` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/814)
[Scheduled] [`d00b6844` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/804)
[Finished] [`43a604de` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/812)
[Failed] [`43a604de` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/809)
[Failed] [`43a604de` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/799)
[Finished] [`43a604de` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/815)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org