Skip to content

MINOR: [C++] Move static declaration to non-static declaration to improve performance#13222

Merged
cyb70289 merged 3 commits intoapache:masterfrom
westonpace:minor/optimize-expression-type
May 31, 2022
Merged

MINOR: [C++] Move static declaration to non-static declaration to improve performance#13222
cyb70289 merged 3 commits intoapache:masterfrom
westonpace:minor/optimize-expression-type

Conversation

@westonpace
Copy link
Member

@westonpace westonpace commented May 24, 2022

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.

@westonpace
Copy link
Member Author

westonpace commented May 24, 2022

@ursabot please benchmark lang=R

@ursabot
Copy link

ursabot commented May 24, 2022 •
edited
Loading

Benchmark runs are scheduled for baseline = 43a604d and contender = 9ab685a2735e8b874aec866075ce9054ab1fe6f2. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Only ['Python'] langs are supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Skipped ⚠️ Only ['C++', 'Java'] langs are supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
[Failed] 9ab685a2 test-mac-arm
[Failed] 9ab685a2 ursa-i9-9960x
[Finished] 43a604de ec2-t3-xlarge-us-east-2
[Failed] 43a604de test-mac-arm
[Failed] 43a604de ursa-i9-9960x
[Finished] 43a604de ursa-thinkcentre-m75q
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

cyb70289
cyb70289 approved these changes May 24, 2022
View reviewed changes
@cyb70289
Copy link
Contributor

cyb70289 commented May 24, 2022

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)

@westonpace
Copy link
Member Author

westonpace commented May 24, 2022

@ursabot please benchmark lang=R

@ursabot
Copy link

ursabot commented May 24, 2022 •
edited
Loading

Benchmark runs are scheduled for baseline = 43a604d and contender = d00b684456cb25d992b08bbbfda23f40b19f7d69. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Only ['Python'] langs are supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Skipped ⚠️ Only ['C++', 'Java'] langs are supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
[Failed] d00b6844 test-mac-arm
[Failed] d00b6844 ursa-i9-9960x
[Finished] 43a604de ec2-t3-xlarge-us-east-2
[Failed] 43a604de test-mac-arm
[Failed] 43a604de ursa-i9-9960x
[Finished] 43a604de ursa-thinkcentre-m75q
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

@pitrou pitrou force-pushed the minor/optimize-expression-type branch from d00b684 to e28ce2a Compare May 30, 2022 16:17
@pitrou
Copy link
Member

pitrou commented May 30, 2022

Rebased and applied suggestion of using a module-private variable.

@pitrou
Copy link
Member

pitrou commented May 30, 2022

@ursabot please benchmark lang=R

@ursabot
Copy link

ursabot commented May 30, 2022 •
edited
Loading

Benchmark runs are scheduled for baseline = 01d8485 and contender = e28ce2a. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Only ['Python'] langs are supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Skipped ⚠️ Only ['C++', 'Java'] langs are supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] e28ce2ac test-mac-arm
[Finished] e28ce2ac ursa-i9-9960x
[Finished] 01d8485d ec2-t3-xlarge-us-east-2
[Failed] 01d8485d test-mac-arm
[Finished] 01d8485d ursa-i9-9960x
[Finished] 01d8485d ursa-thinkcentre-m75q
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

@cyb70289 cyb70289 merged commit 56cdaae into apache:master May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitrou pitrou pitrou approved these changes

@cyb70289 cyb70289 cyb70289 approved these changes

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@westonpace @ursabot @cyb70289 @pitrou