Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CI][C++] arrow-array-test slow on macOS #35712

Closed
pitrou opened this issue May 22, 2023 · 13 comments · Fixed by #35724
Closed

[CI][C++] arrow-array-test slow on macOS #35712

pitrou opened this issue May 22, 2023 · 13 comments · Fixed by #35724
Assignees
Milestone

Comments

@pitrou
Copy link
Member

pitrou commented May 22, 2023

Describe the bug, including details regarding any error messages, version, and platform.

This is rather minor, but arrow-array-test takes more than one minute on our C++ macOS CI. I even saw it sporadically time out.

It would be nice to find out the cause, as it may be an underlying performance problem in Arrow C++.

 25/101 Test   #1: arrow-array-test ..........................   Passed  103.65 sec

Component(s)

C++

@mapleFU
Copy link
Member

mapleFU commented May 22, 2023

Hi, pitrou, on My MacOS with M1 Pro, test runs much more faster, here are some slow ones:

1: [ RUN      ] TestDictionary.Validate
1: [       OK ] TestDictionary.Validate (8382 ms)

1: [ RUN      ] TestArray.BuildLargeInMemoryArray
1: [       OK ] TestArray.BuildLargeInMemoryArray (2636 ms)

The whole tests are finished within 15secs... So maybe we can print the result in CI?

The tests are runed in DEBUG mode, CMAKE configs are:

-DARROW_PARQUET=ON -DARROW_FLIGHT=ON -DARROW_BUILD_TESTS=ON -DARROW_FUZZING=ON -DARROW_USE_ASAN=ON -DARROW_USE_UBSAN=ON -DARROW_FUZZING=ON -DARROW_WITH_SNAPPY=ON -DARROW_WITH_ZSTD=ON -DARROW_BUILD_EXAMPLES=ON -DARROW_ENABLE_TIMING_TESTS=ON -DARROW_DATASET=ON -DPARQUET_REQUIRE_ENCRYPTION=ON -DCMAKE_CXX_FLAGS="-g -fno-omit-frame-pointer" -DCMAKE_MODULE_PATH="${CMAKE_MODULE_PATH}:/usr/local/include/thrift" -DGTest_SOURCE="BUNDLED"

@pitrou
Copy link
Member Author

pitrou commented May 22, 2023

Perhaps you can submit a draft PR to run the tests in verbose mode indeed.

@mapleFU
Copy link
Member

mapleFU commented May 22, 2023

I'm glad to but it's a bit late in my timezone, if nobody tries it tonight, I'll have a try in the morning :-)

@mapleFU
Copy link
Member

mapleFU commented May 23, 2023

Hi pitrou, I've tried it here: https://github.com/apache/arrow/actions/runs/5053338938/jobs/9067090574?pr=35719

Top 20 slowest tests:
Test: TestDictionary 47785(ms)
Test: TestArray 764(ms)
Test: TestPrimitiveBuilder/12 63(ms)
Test: TestPrimitiveBuilder/3 54(ms)
Test: EncodedArrayTests/TestRunEndEncodedArray 52(ms)

Seems that we should optimize TestDictionary?

@pitrou
Copy link
Member Author

pitrou commented May 23, 2023

Seems that we should optimize TestDictionary?

Definitely!

@mapleFU
Copy link
Member

mapleFU commented May 23, 2023

The test is here: #35719
And I force the test failed, sort and output the testing time. I've no idea why TestDictionary slow, maybe I can take a look tonight

@pitrou
Copy link
Member Author

pitrou commented May 23, 2023

Also cc @felipecrv

@mapleFU
Copy link
Member

mapleFU commented May 23, 2023

I guess I've found out the reason. Maybe it's ASSERT_DEATH making testing slowly:

  ASSERT_DEATH(
      {
        std::shared_ptr<Array> null_dict_arr =
            std::make_shared<DictionaryArray>(dict_type, indices, nullptr);
      },
      "");

In TestDictionary.Validate @pitrou . Can I remove this if I can confirm it?

@mapleFU
Copy link
Member

mapleFU commented May 23, 2023

After remove ASSERT_DEATH:

Top 20 slowest tests:
Test: TestArray 756(ms)
Test: EncodedArrayTests/TestRunEndEncodedArray 78(ms)
Test: Decimal128Test/Decimal128Test 64(ms)
Test: TestPrimitiveBuilder/12 58(ms)
Test: TestPrimitiveBuilder/0 52(ms)
Test: TestDictionary 47(ms)
Test: TestPrimitiveBuilder/8 42(ms)
Test: TestPrimitiveBuilder/11 41(ms)
Test: TestStringDictionaryBuilder 39(ms)
Test: TestPrimitiveBuilder/4 39(ms)
Test: TestPrimitiveBuilder/6 38(ms)
Test: TestPrimitiveBuilder/10 35(ms)
Test: TestPrimitiveBuilder/7 34(ms)
Test: TestPrimitiveBuilder/3 34(ms)
Test: TestPrimitiveBuilder/2 32(ms)
Test: TestDecimal256DictionaryBuilder 31(ms)
Test: TestPrimitiveBuilder/1 31(ms)
Test: TestPrimitiveBuilder/9 30(ms)
Test: TestAdaptiveIntBuilder 29(ms)
Test: TestPrimitiveBuilder/5 29(ms)
~/work/arrow/arrow/build/cpp/src/arrow

@pitrou
Copy link
Member Author

pitrou commented May 23, 2023

Can you validate on the macOS CI?

@mapleFU
Copy link
Member

mapleFU commented May 23, 2023

@pitrou
Copy link
Member Author

pitrou commented May 23, 2023

Thanks for the investigation @mapleFU ! I think we can just disable the death test on macOS, since other platforms don't seem to have that problem.

@mapleFU
Copy link
Member

mapleFU commented May 23, 2023

Okay, let me submit a quick fixing with a new patch

pitrou pushed a commit that referenced this issue May 23, 2023
…35724)

### Rationale for this change

Disable ASSERT_DEATH in MacOS. According to #35712, it would making CI extremely slow.

### What changes are included in this PR?

Disable test in macos

### Are these changes tested?

no

### Are there any user-facing changes?

no

* Closes: #35712

Authored-by: mwish <maplewish117@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@pitrou pitrou added this to the 13.0.0 milestone May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment