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

Have sh_test directly invoke benchmarks #4552

Merged
merged 3 commits into from
Nov 20, 2024
Merged

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Nov 18, 2024

These tests typically take 10-20s, but I'm seeing some timeouts here. This seemed particularly suspicious due to the absence of output (copied below). That got me looking, and maybe the subprocessing tickles a cpu bottleneck, so proposing this approach to remove the exec. Even if this doesn't solve the flakiness, I think it's a simpler implementation.

Note I believe this is intended to work. The sh rules rely on shebangs (as noted at https://bazel.build/reference/be/shell#sh_test), and are essentially just subprocessing to the input. Note this could've also had args on a cc_test rule, but I'd expect the same args to be passed to run where instead the benchmark behavior should be default (and I'm assuming you'd rather not have args there). Fundamentally this becomes a symlink:

bazel-bin/common/map_benchmark_test -> .../execroot/_main/bazel-out/k8-fastbuild/bin/common/map_benchmark

Copying snippet from timeout below:

==================== Test output for //common:map_benchmark_test:
      /private/var/tmp/_bazel_runner/e591f63ed099023de1f206992dfce127/execroot/_main/bazel-out/darwin_arm64-fastbuild/testlogs/common/map_benchmark_test/test.log
-- Test timed out at 2024-11-18 19:32:13 UTC --
INFO: From Testing //common:map_benchmark_test:
================================================================================

@jonmeow jonmeow changed the title Have sh_test directly invoke scripts Have sh_test directly invoke benchmarks Nov 20, 2024
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super neat! TIL about teh sh_test being able to just run a binary. This is much nicer, thanks!

@jonmeow jonmeow added this pull request to the merge queue Nov 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2024
@jonmeow jonmeow added this pull request to the merge queue Nov 20, 2024
@jonmeow
Copy link
Contributor Author

jonmeow commented Nov 20, 2024

Well, clearly not a fix. May need to talk to you about how important the "enormous" benchmarks are.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2024
@jonmeow jonmeow added this pull request to the merge queue Nov 20, 2024
Merged via the queue into carbon-language:trunk with commit 493d766 Nov 20, 2024
8 checks passed
@jonmeow jonmeow deleted the sh-test branch November 20, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants