ARROW-15678: [C++][CI] a crossbow job with MinRelSize enabled #12928

jonkeane · 2022-04-19T21:22:10Z

In addition to the CI job that tests this, it includes for the wrong SIMD instructions on some platforms

github-actions · 2022-04-19T21:22:30Z

https://issues.apache.org/jira/browse/ARROW-15678

github-actions · 2022-04-19T21:22:31Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

jonkeane · 2022-04-19T21:23:19Z

@github-actions crossbow submit test-r-minsizerel

This should fail, it is before the fix is added

github-actions · 2022-04-19T21:24:26Z

Revision: 44bfc59

Submitted crossbow builds: ursacomputing/crossbow @ actions-1876

Task	Status
test-r-minsizerel

jonkeane · 2022-04-19T21:27:06Z

@github-actions crossbow submit test-r-minsizerel

After @westonpace 's patch

github-actions · 2022-04-19T21:28:34Z

Revision: eb91ce0

Submitted crossbow builds: ursacomputing/crossbow @ actions-1877

Task	Status
test-r-minsizerel

jonkeane · 2022-04-20T15:48:48Z

cpp/src/arrow/util/bitmap_writer.h

@@ -78,96 +79,24 @@ class BitmapWriter {
  int64_t byte_offset_;
 };

-class FirstTimeBitmapWriter {
+class ARROW_EXPORT FirstTimeBitmapWriter {


This resolved the windows CI failures, though not sure if I should include ARROW_EXPORT for other classes as well.

Since the other classes are fully defined in the header you should not have to. Technically it wouldn't hurt either but best to be minimal with changes probably.

pitrou · 2022-04-20T15:56:26Z

@jonkeane Can you point to the Windows CI failures?

jonkeane · 2022-04-20T15:59:03Z

Here is one: https://github.com/apache/arrow/runs/6085559326?check_suite_focus=true but there are a handful of others on eb91ce0

pitrou · 2022-04-20T16:02:11Z

Well, un-inlining the FirstTimeBitmapWriter method definitions could very well decrease performance, so we shouldn't do that. I'm also sure there's a way to avoid the linking failures, perhaps @kou can help?

pitrou · 2022-04-20T16:02:21Z

@ursabot please benchmark lang=C++

ursabot · 2022-04-20T16:02:26Z

Benchmark runs are scheduled for baseline = 4544f95 and contender = 24a1764. 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
[Finished ⬇️2.85% ⬆️0.25%] test-mac-arm
[Skipped ⚠️ Only ['JavaScript', 'Python', 'R'] langs are supported on ursa-i9-9960x] ursa-i9-9960x
[Finished ⬇️2.13% ⬆️0.3%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/537| 24a17640 test-mac-arm>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/546| 24a17640 ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/544| 4544f953 ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/531| 4544f953 test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/529| 4544f953 ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/541| 4544f953 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

jonkeane · 2022-04-20T17:58:43Z

There definitely are regressions in throughput on those C++ benchmarks.

Interestingly, it looks like there was a pretty big regression (larger than here, even!) in some of these already around #12634 which is odd since that's a java + documentation only ticket. Though it does look like our compiler version on the benchmarking machine went from 9.4.0 to 10.3.0 coincidentally at that commit, which might be the real source of that regression.

pitrou · 2022-04-20T18:43:58Z

Yes, you can see there are regressions in kernels that write an output bitmap (such as is_in or substring matching), which definitely relates to FirstTimeBitmapWriter.

kou · 2022-04-21T01:42:18Z

Can we use ARROW_FORCE_INLINE instead of hiding implementation to .cc here?

We can use __attribute__((always_inline)) with g++. There are many force inline macros in our code base such as FORCEINLINE, FORCE_INLINE and GDV_FORCE_INLINE.

…ng implemented in level_conversion_bmi2.cc.o which is always compiled with avx2

pitrou · 2022-04-21T17:01:43Z

I started a Crossbow job here: https://github.com/ursacomputing/crossbow/actions/runs/2203242917

westonpace · 2022-04-22T03:05:04Z

I played around with this a bit more. I can reproduce it locally by building with SSE4_2:

cmake .. -DARROW_PARQUET=ON -DARROW_SIMD_LEVEL=SSE4_2 -DARROW_RUNTIME_SIMD_LEVEL=MAX -DARROW_BUILD_TESTS=ON

From there it's easiest to verify by just manually checking to see which version ends up in libparquet.so:

objdump --disassemble=_ZN5arrow8internal21FirstTimeBitmapWriter10AppendWordEml -S ./minsizerel/libparquet.so.800.0.0

If the output contains shlx then you've reproduced the bug. If it only contains shl then it picked the correct default symbol. If the method is entirely inlined you get no output.

The symbol is inlined with -DCMAKE_BUILD_TYPE=Release
The symbol is not inlined with -DCMAKE_BUILD_TYPE=MinSizeRel
- However, on my system, in all cases, the libparquet.so file chooses the correct version unless...
- I can get an invalid .so file if I switch the order the object files are passed to the linker: /usr/bin/clang++-13 ... level_conversion_bmi2.cc.o ... level_conversion.cc.o ...
The symbol is inlined, even with MinSizeRel is I try @kou's fix (__attribute__((always_inline))).
- This seems like the easiest "spot fix" if we wanted to include something as part of 8.0.0

If you really want to reproduce the issue, I found a tool sde64 which will work if you have an Intel processor. It allows you to simulate older Intel processors and so you can pretend to have an Ivy Bridge processor (which does not have AVX2/BMI2 support):

sde64 -ivb -- ./minsizerel/parquet-arrow-test --gtest_filter=TestParquetIO/0.SingleNullableListNullableColumnReadWrite
Running main() from ../googletest/src/gtest_main.cc
Note: Google Test filter = TestParquetIO/0.SingleNullableListNullableColumnReadWrite
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from TestParquetIO/0, where TypeParam = arrow::BooleanType
[ RUN      ] TestParquetIO/0.SingleNullableListNullableColumnReadWrite
TID 0 SDE-ERROR: Executed instruction not valid for specified chip (IVYBRIDGE): 0x7f80c95b96b3: shlx rax, rbx, rax
Image: /home/pace/dev/arrow/cpp/sse4.2-min-build/minsizerel/libparquet.so.800+0x1666b3
Function: _ZN5arrow8internal21FirstTimeBitmapWriter10AppendWordEml
Instruction bytes are: c4 e2 f9 f7 c3

@pitrou has also posted a suggestion on the Jira using pragmas. I had to include the arch specifier so it generated __attribute__((target("arch=haswell,avx2"))) and this appears to do what is expected. The function is compiled with avx2 but the nested call is not.

kszucs · 2022-04-24T18:08:17Z

Thanks @westonpace for digging deeper! Since the release is approaching we should choose one solution to unblock the release.

pitrou · 2022-04-25T08:25:18Z

IMHO the temporary solution should simply be to disallow dynamic dispatch on Homebrew builds. This should be doable by passing -DARROW_RUNTIME_SIMD_LEVEL=NONE to CMake.

jonkeane · 2022-04-25T13:04:17Z

If we're going to leave this as is, we should also document somewhere that compiling with MinSizeRel also needs to disable SIMD in the same way.

pitrou · 2022-04-25T14:07:25Z

This is not due to MinSizeRel specifically, so it shouldn't be worded like that. Also, this is really a bug that needs fixing, it's just that it's not reasonably possible to fix it properly for the release.

westonpace · 2022-04-25T17:19:00Z

I agree with @pitrou that it doesn't make sense to rush this while we investigate a longer term fix for dynamic dispatch. The forced inline is more of a band-aid and there is no real guarantee it would work in all situations.

Agreed this isn't related to MinSizeRel. That configuration triggers this bug on this particular system. However, it is entirely possible some other build on some other system runs into this exact same bug without using MinSizeRel. As far as the compiler's are concerned we are in the domain of "undefined behavior" so any change to compilation flags or compiler versions could change what results we get.

jonkeane · 2022-04-25T17:47:24Z

Ok, if we want to test this in our CI before we upstream the change to homebrew, I would remove this line

arrow/dev/tasks/homebrew-formulae/apache-arrow.rb

Lines 66 to 68 in d85e902

    
           # https://github.com/Homebrew/homebrew-core/issues/94724 
        
           # https://issues.apache.org/jira/browse/ARROW-15664 
        
           ENV["HOMEBREW_OPTIMIZATION_LEVEL"] = "O2"

, add the compiler flag below, and then run the crossbow job homebrew-r-brew.

kszucs · 2022-07-25T15:37:55Z

@jonkeane @nealrichardson what's the status of it? Do we need this for the next homebrew release?

jonkeane · 2022-07-25T22:48:26Z

What's here only adds the CI that will replicate the failure long-discussed in ARROW-15678. I'm inclined to close it until we have a champion who can resolve the issue — the code is here if they need it (or we could merge it and have a crossbow job that always fails until we have someone who can champion the fix...).

github-actions bot added Component: C++ Component: R labels Apr 19, 2022

jonkeane requested review from pitrou and westonpace April 20, 2022 15:48

jonkeane commented Apr 20, 2022

View reviewed changes

jonkeane and others added 11 commits April 21, 2022 18:16

This should fail cause MinRelSize is not yet fixed

2c9ee58

add MinSizeRel to setupCxxFlags.cmake

22060c7

oops

32fc2c4

add ARROW_R_DEV to the global env

60bbb69

oops, need to print

4823e7b

add extra flag

e839adb

Experiment with other clang version

ef7c2ec

Also update where R looks for compilers

e528432

oops, need to create the directory first

63d01bb

try gcc-11

e08c41c

bundle deps to avoid gcc-clang mismatch

5895e36

jonkeane and others added 8 commits April 21, 2022 18:16

a different way of testing

c0f8a7a

skip some tests that complain about somethign unrelated

0b9d21f

try only the parquet tests

b24ab17

Cleanup, rebase

2f63065

oops

c807769

Moved function definitions into a .cc file to avoid the functions bei…

257c63d

…ng implemented in level_conversion_bmi2.cc.o which is always compiled with avx2

try an ARROW_EXPORT

83d7753

Revert FirstTimeBitmapWriter changes to understand issue

0765e30

pitrou force-pushed the ARROW-15678 branch from 24a1764 to 0765e30 Compare April 21, 2022 16:16

westonpace removed their request for review July 11, 2022 14:36

jonkeane closed this Jul 25, 2022

asfimport mentioned this pull request Oct 11, 2022

[C++][CI] a crossbow job with MinRelSize enabled #31132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-15678: [C++][CI] a crossbow job with MinRelSize enabled #12928

ARROW-15678: [C++][CI] a crossbow job with MinRelSize enabled #12928

jonkeane commented Apr 19, 2022 •

edited

Loading

github-actions bot commented Apr 19, 2022

github-actions bot commented Apr 19, 2022

jonkeane commented Apr 19, 2022

github-actions bot commented Apr 19, 2022

jonkeane commented Apr 19, 2022

github-actions bot commented Apr 19, 2022

jonkeane Apr 20, 2022

westonpace Apr 20, 2022

kou Apr 21, 2022

pitrou commented Apr 20, 2022

jonkeane commented Apr 20, 2022

pitrou commented Apr 20, 2022

pitrou commented Apr 20, 2022

ursabot commented Apr 20, 2022 •

edited

Loading

jonkeane commented Apr 20, 2022

pitrou commented Apr 20, 2022

kou commented Apr 21, 2022

pitrou commented Apr 21, 2022

westonpace commented Apr 22, 2022 •

edited by kou

Loading

kszucs commented Apr 24, 2022

pitrou commented Apr 25, 2022

jonkeane commented Apr 25, 2022

pitrou commented Apr 25, 2022

westonpace commented Apr 25, 2022

jonkeane commented Apr 25, 2022

kszucs commented Jul 25, 2022

jonkeane commented Jul 25, 2022

ARROW-15678: [C++][CI] a crossbow job with MinRelSize enabled #12928

ARROW-15678: [C++][CI] a crossbow job with MinRelSize enabled #12928

Conversation

jonkeane commented Apr 19, 2022 • edited Loading

github-actions bot commented Apr 19, 2022

github-actions bot commented Apr 19, 2022

jonkeane commented Apr 19, 2022

github-actions bot commented Apr 19, 2022

jonkeane commented Apr 19, 2022

github-actions bot commented Apr 19, 2022

jonkeane Apr 20, 2022

Choose a reason for hiding this comment

westonpace Apr 20, 2022

Choose a reason for hiding this comment

kou Apr 21, 2022

Choose a reason for hiding this comment

pitrou commented Apr 20, 2022

jonkeane commented Apr 20, 2022

pitrou commented Apr 20, 2022

pitrou commented Apr 20, 2022

ursabot commented Apr 20, 2022 • edited Loading

jonkeane commented Apr 20, 2022

pitrou commented Apr 20, 2022

kou commented Apr 21, 2022

pitrou commented Apr 21, 2022

westonpace commented Apr 22, 2022 • edited by kou Loading

kszucs commented Apr 24, 2022

pitrou commented Apr 25, 2022

jonkeane commented Apr 25, 2022

pitrou commented Apr 25, 2022

westonpace commented Apr 25, 2022

jonkeane commented Apr 25, 2022

kszucs commented Jul 25, 2022

jonkeane commented Jul 25, 2022

jonkeane commented Apr 19, 2022 •

edited

Loading

ursabot commented Apr 20, 2022 •

edited

Loading

westonpace commented Apr 22, 2022 •

edited by kou

Loading