Page MenuHomePhabricator

LLDB Windows x64 buildbot set lit -j8
ClosedPublic

Authored by omjavaid on Jan 14 2022, 2:35 PM.

Details

Summary

LLDB windows x64 buildbot has falky behavior on testsuite. I am setting
number of parallel tests to 8 on this buildbot in hope that it improves
testsuite stability like it did for Linux/Arm.

Diff Detail

Event Timeline

omjavaid created this revision.Jan 14 2022, 2:35 PM
omjavaid requested review of this revision.Jan 14 2022, 2:35 PM
buildbot/osuosl/master/config/builders.py
1204

Right now the getLLDBCMakeBuildFactory function will separately set LLVM_LIT_ARGS as well. I think they should be set in just one place to make the code more readable. Perhaps we can remove the LLVM_LIT_ARGS from getLLDBCMakeBuildFactory if they are not used anywhere?

omjavaid added inline comments.Jan 14 2022, 2:50 PM
buildbot/osuosl/master/config/builders.py
1204

extra flags override those as they appended last to cmake_options. That way we set lit args in case use has not supplied any specific ones in extra_cmake_args.

buildbot/osuosl/master/config/builders.py
1204

OK, if @gkistanova is OK with this approach also, it's fine by me.

1204

Why -s (succinct)?

omjavaid added inline comments.Jan 14 2022, 3:00 PM
buildbot/osuosl/master/config/builders.py
1204

its mostly cosmetic to show a progress bar.

gkistanova added a comment.EditedJan 14 2022, 3:01 PM

I think they should be set in just one place to make the code more readable.

I was about to suggest the same.

It might be simpler and more straight forward to revert D116972 and just explicitly specify the needed LLVM_LIT_ARGS in the extra_cmake_args.
If you would like to set -v and -j by default, that could be done by something like this:

defult_lit_args = '-v'
if jobs:
    defult_lit_args += " -j{jobs}".format(jobs=jobs)
CmakeCommand.applyDefaultOptions(cmake_options, [
        ('-DLLVM_LIT_ARGS=', defult_lit_args),
        ])

This is not a blocker for this patch, though.

A separate thing. The patch specifies the both s and v, which kinds of contradict each other, What's the point?

I think they should be set in just one place to make the code more readable.

I was about to suggest the same.

It might be simpler and more straight forward to revert D116972 and just explicitly specify the needed LLVM_LIT_ARGS in the extra_cmake_args.
If you would like to set -v and -j by default, that could be done by something like this:

defult_lit_args = '-v'
if if jobs:
    defult_lit_args += " -j{jobs}".format(jobs=jobs)
CmakeCommand.applyDefaultOptions(cmake_options, [
        ('-DLLVM_LIT_ARGS=', defult_lit_args),
        ])

This is not a blocker for this patch, though.

A separate thing. The patch specifies the both s and v, which kinds of contradict each other, What's the point?

I conveniently copied lit args from arm lldb bot. AFAIK both giving and sv together shows output as well as a progress bar but I am not too sure.

I think they should be set in just one place to make the code more readable.

I was about to suggest the same.

It might be simpler and more straight forward to revert D116972 and just explicitly specify the needed LLVM_LIT_ARGS in the extra_cmake_args.
If you would like to set -v and -j by default, that could be done by something like this:

defult_lit_args = '-v'
if if jobs:
    defult_lit_args += " -j{jobs}".format(jobs=jobs)
CmakeCommand.applyDefaultOptions(cmake_options, [
        ('-DLLVM_LIT_ARGS=', defult_lit_args),
        ])

What about adding a separate parameter for the jobs for the tests? That way a different level of parallelism could be specified for build and test? That's the route I would have gone.

What about adding a separate parameter for the jobs for the tests? That way a different level of parallelism could be specified for build and test? That's the route I would have gone.

+1

What about adding a separate parameter for the jobs for the tests? That way a different level of parallelism could be specified for build and test? That's the route I would have gone.

I would guess a particular value depends on available worker hardware resources. So, it could be a property, something like a lit_jobs, which gets rendered to LLVM_LIT_ARGS if defined. Similar how jobs property is defined for workers.

# Windows Server 2016 Intel Xeon(R) Quad 2.30 GHz, 56GB of RAM
create_worker("win-py3-buildbot", properties={'jobs' : 64, 'lit_jobs' : 8}, max_builds=1),

I conveniently copied lit args from arm lldb bot. AFAIK both giving and sv together shows output as well as a progress bar but I am not too sure.

I see. The current LIT implementation ignores -v and executes -s no matter in what order they are specified. So, you need the one or the other, not two together. With automated builds, you may want -v, as it prints more information which could be useful for troubleshooting build issues and test failures.

Anyway, feel free to commit this patch as is. It would solve your immediate problem and everything else we are discussing here could be done as a follow up.

I have still setting up the Arm WoA worker which requires a bit of tweaking with visual studio config as well. I have not found a final patch for it and going to commit this one while I work on a follow up.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 19 2022, 4:04 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.