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

Use tasty-bench package itself #181

Merged
merged 3 commits into from May 26, 2023
Merged

Use tasty-bench package itself #181

merged 3 commits into from May 26, 2023

Conversation

Bodigrim
Copy link
Contributor

Resurrecting #108.

The immediate reason is that your fork of tasty-bench is susceptible to Bodigrim/tasty-bench#44, which has been fixed upstream.

There is also an ongoing effort to monitor performance of core libraries between GHC releases, but not using the standard tooling means that filepath is likely to miss out. E. g., there is no comparison between runs available, no filtering by patterns, no easy way to swap tasty-bench for tasty-papi / tasty-perfbench to count CPU instructions.

@hasufell
Copy link
Member

So does tasty-bench still depend on optparse-applicative? Can we have a lighter version?

@Bodigrim
Copy link
Contributor Author

We would not be able to avoid optparse-applicative, but I never had a build issue with it. Any particular reason you deem it undesirable?

@hasufell
Copy link
Member

If you rebase against master and then push to this repo instead of your fork, we can benefit from the fixed CI.

@Bodigrim
Copy link
Contributor Author

cabal bench would take ages on CI. Pass cabal bench --benchmark-options='--timeout 1' (or even cabal bench --benchmark-options='-l' if the goal is just to validate that the executable is runnable).

@hasufell
Copy link
Member

It seems to be doing ok, no?

@hasufell
Copy link
Member

cabal bench --benchmark-options='--timeout 1'

I guess that makes sense. The benchmark info on CI won't be very useful.

@hasufell
Copy link
Member

This is quite interesting:

+ cabal bench
Build profile: -w ghc-9.4.4 -O1
In order, the following will be built (use -v for more details):
 - filepath-1.4.100.1 (bench:bench-filepath) (configuration changed)
Configuring benchmark 'bench-filepath' for filepath-1.4.100.1..
Preprocessing benchmark 'bench-filepath' for filepath-1.4.100.1..
Building benchmark 'bench-filepath' for filepath-1.4.100.1..
Running 1 benchmarks...
Benchmark bench-filepath: RUNNING...

Access violation in generated code when reading 0xe47d8008

 Attempting to reconstruct a stack trace...

   Frame	Code address
 * 0x6d372fcdb0	0x7ff75662577e D:\a\filepath\filepath\dist-newstyle\build\x86_64-windows\ghc-9.4.4\filepath-1.4.100.1\b\bench-filepath\build\bench-filepath\bench-filepath.exe+0xb8577e
 * 0x6d372fce20	0x7ff7566248cc D:\a\filepath\filepath\dist-newstyle\build\x86_64-windows\ghc-9.4.4\filepath-1.4.100.1\b\bench-filepath\build\bench-filepath\bench-filepath.exe+0xb848cc
 * 0x6d372fce70	0x7ff7567a0794 D:\a\filepath\filepath\dist-newstyle\build\x86_64-windows\ghc-9.4.4\filepath-1.4.100.1\b\bench-filepath\build\bench-filepath\bench-filepath.exe+0xd00794
 * 0x6d372fcef0	0x7ff75661c5ab D:\a\filepath\filepath\dist-newstyle\build\x86_64-windows\ghc-9.4.4\filepath-1.4.100.1\b\bench-filepath\build\bench-filepath\bench-filepath.exe+0xb7c5ab
 * 0x6d372fcf40	0x7ff75661c44e D:\a\filepath\filepath\dist-newstyle\build\x86_64-windows\ghc-9.4.4\filepath-1.4.100.1\b\bench-filepath\build\bench-filepath\bench-filepath.exe+0xb7c44e
 * 0x6d372fd1c0	0x7ff7563d72e0 D:\a\filepath\filepath\dist-newstyle\build\x86_64-windows\ghc-9.4.4\filepath-1.4.100.1\b\bench-filepath\build\bench-filepath\bench-filepath.exe+0x9372e0
 * 0x6d372fd250	0x7ff755ee673f D:\a\filepath\filepath\dist-newstyle\build\x86_64-windows\ghc-9.4.4\filepath-1.4.100.1\b\bench-filepath\build\bench-filepath\bench-filepath.exe+0x44673f
 * 0x6d372fd290	0x7ff755ee6913 D:\a\filepath\filepath\dist-newstyle\build\x86_64-windows\ghc-9.4.4\filepath-1.4.100.1\b\bench-filepath\build\bench-filepath\bench-filepath.exe+0x446913
 * 0x6d372fd298	0x7ff755ac559c D:\a\filepath\filepath\dist-newstyle\build\x86_64-windows\ghc-9.4.4\filepath-1.4.100.1\b\bench-filepath\build\bench-filepath\bench-filepath.exe+0x2559c

Benchmark bench-filepath: ERROR
Error: cabal-3.8.1.0.exe: Benchmarks failed for bench:bench-filepath from
filepath-1.4.100.1.

In general, windows on 9.4.4 seems not very reliable, e.g. https://gitlab.haskell.org/ghc/ghc/-/issues/21990

@hasufell
Copy link
Member

@Bodigrim ... @bgamari was suggesting to carefully check the FFI calls. I don't see any in filepath wrt benchmarks. Given that the test suite doesn't trigger it, maybe it lies in tasty-bench?

@Bodigrim
Copy link
Contributor Author

The only FFI in tasty-bench is https://github.com/Bodigrim/tasty-bench/blob/920ca44cd5c45274a8e9ea23b38883bc6278343a/src/Test/Tasty/Bench.hs#L1986-L1987, but it seems benign and tasty-bench builds fine on Windows with GHC 9.4.4 (https://github.com/Bodigrim/tasty-bench/actions/runs/4178046179/jobs/7236375600).

I'll try to reproduce next week, once I get to my Windows machine.

@RyanGlScott
Copy link
Member

I am able to reproduce the access violation locally on Windows. Some obversations:

  • It's worth noting that cabal picks a build plan with an extremely old version of Win32:

      - Win32-2.4.0.0 (lib:Win32) (requires download & build)
    

    This is the last version of Win32 that doesn't depend on filepath, which is why cabal picks it. Trying to force a build plan with a later version of Win32 will fail since filepath and Win32 will induce cyclic dependencies.

    One way to avoid this is to factor out the benchmarks into a separate package, which is what I've done here. The downside is that you will have to build Win32 (and all of its reverse dependencies in the benchmark suite) on each CI run.

    Unfortunately, even after getting a build plan with the latest Win32 version, the access violation still occurs. This is because...

  • The access violation occurs even with a main function as simple as this:

    main = defaultMain []

    In fact, I strongly suspect that the issue doesn't have anything to do with tasty-bench in particular. The access violation goes away if you remove the use of the --nonmoving-gc RTS option. Getting a minimal example of this bug that still triggers the access violation is challenging, but I was able to get a minimal example that incorrectly loops forever when you run it with --nonmoving-gc. I've filed an issue upstream at GHC#23003.

@hasufell
Copy link
Member

This is the last version of Win32 that doesn't depend on filepath, which is why cabal picks it. Trying to force a build plan with a later version of Win32 will fail since filepath and Win32 will induce cyclic dependencies.

@Bodigrim this is a problem... tasty-bench depends on tasty, which depends on ansi-terminal, which depends on Win32 explicitly. The inlined version does not have this problem.

@Bodigrim
Copy link
Contributor Author

This is the last version of Win32 that doesn't depend on filepath, which is why cabal picks it. Trying to force a build plan with a later version of Win32 will fail since filepath and Win32 will induce cyclic dependencies.

@Bodigrim this is a problem... tasty-bench depends on tasty, which depends on ansi-terminal, which depends on Win32 explicitly. The inlined version does not have this problem.

Thanks to @mpilgrem, this is now solved: ansi-terminal-1.0 no longer depends on Win32.

Non-moving GC seems to be solidly broken on Windows though. FWIW since tasty-bench-0.3.2 README stopped suggesting enabling non-moving GC for benchmarks precisely because it's been causing too much trouble.

@hasufell
Copy link
Member

Non-moving GC seems to be solidly broken on Windows though. FWIW since tasty-bench-0.3.2 README stopped suggesting enabling non-moving GC for benchmarks precisely because it's been causing too much trouble.

I'm fine with disabling it, either on windows or in general.

@hasufell
Copy link
Member

CI passes. Is this still considered a Draft?

@Bodigrim Bodigrim marked this pull request as ready for review May 24, 2023 18:04
@Bodigrim
Copy link
Contributor Author

@hasufell ready for review now.

@hasufell hasufell merged commit 280fd8a into haskell:master May 26, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants