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

Benchmark: Enable 9.6, 9.8 #4118

Merged
merged 14 commits into from
Mar 7, 2024
Merged

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented Mar 5, 2024

~~ Enable 4 version to do bench seems to be taking too long, considering picking two: highest and lowest ~~

Fix bench for newer ghc versions
The following have been done:

  1. No longer use the implicit-hie to generate the hie.yaml for the bench examples and in favor of using "cradle:\n cabal:\n", seems to be working with modern cabal.
  2. upgrade benchmark to use 9.6, 9.8 (The latest two we support for now).
  3. upgrade bench examples to Cabal version: 3.10.2.1, lsp-types version: 2.1.1.0
  4. fix minor error that *.hp files duplicates its extension name

@michaelpj
Copy link
Collaborator

"Two highest" would also be reasonable, as those are the ones we are going to support for longest. But "highest and lowest" captures the most difference, so is maybe indeed the most interesting. If we do this we should write it down.

Due to its error behaviour on lsp-types-3.1.1.0,
In favor of using basic configuration for hie.yaml.
@soulomoon
Copy link
Collaborator Author

soulomoon commented Mar 6, 2024

Some thing problematic with the hie.yaml generated by implicit-hie for lsp-types-2.1.1.0.
Instead of generate the hie.yaml with implicite-hie, fill hie.yaml simply with "cradle:\n cabal:\n" seems
to be working fow more examples in the bench nowadays. And so I did.

Bench for 4 versions of ghc: Total duration
51m 28s.
Acceptable? What do you guys think.

If not I'll chunk it to only run for newest and oldest and add the comment as @michaelpj suggest.

@soulomoon soulomoon marked this pull request as ready for review March 6, 2024 23:57
@soulomoon soulomoon marked this pull request as draft March 6, 2024 23:58
@soulomoon soulomoon marked this pull request as ready for review March 6, 2024 23:59
@soulomoon soulomoon changed the title Pipeline: Enable 9.6 9.8 for bench Pipeline: Enable 9.6, 9.8 for bench Mar 7, 2024
@soulomoon soulomoon changed the title Pipeline: Enable 9.6, 9.8 for bench Benchmark: Enable 9.6, 9.8 Mar 7, 2024
Copy link
Collaborator

@jhrcek jhrcek left a comment

Choose a reason for hiding this comment

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

Great work!
I'm not really sure which option is the best (2 highest vs. newest+oldest) but having all 4 seems too much.

From maintenance point of view 2 highest sounds easiest to maintain.
When we add a new ghc (e.g. 9.10)

  • 2 highest will require dropping the older one and adding a new one (e.g. drop 9.6, add 9.10)
  • newest+oldest will require updating the oldest to one higher (9.2->9.4) + newest to one higher (9.8->9.10)

So I would vote for "2 highest", but don't feel too strongly about it.

@@ -1835,8 +1835,6 @@ test-suite wrapper-test
benchmark benchmark
import: defaults, warnings
-- Depends on shake-bench which is unbuildable after this point
if impl(ghc >= 9.5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment above belongs to this if, so I think it should be removed too.

Copy link
Collaborator Author

@soulomoon soulomoon Mar 7, 2024

Choose a reason for hiding this comment

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

indeed

@@ -17,8 +17,6 @@ source-repository head

library
-- Depends on Chart which is unbuildable after this point
if impl(ghc >= 9.5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this comment still true? Is chart now buildable?

Copy link
Collaborator Author

@soulomoon soulomoon Mar 7, 2024

Choose a reason for hiding this comment

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

Not true, we can build chart, I forget to delete them.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing up the benchmark!

ghcide-bench/src/Experiments.hs Outdated Show resolved Hide resolved
@michaelpj
Copy link
Collaborator

I think just do two. 51 minutes is a lot, and the benchmark job does actually run quite a bit, so we're just burning CPU cycles for not much benefit.

soulomoon and others added 3 commits March 7, 2024 16:57
Co-authored-by: fendor <fendor@users.noreply.github.com>
@soulomoon
Copy link
Collaborator Author

soulomoon commented Mar 7, 2024

After hearing you guys opinion.
I decided we keep just two latest versions.
Benchmark is expensive. I believe @jhrcek "maintenance reason" (lacking man power) and @michaelpj "support longest reason" (make our development more forward looking) does defeat the need to capture the most different(by choosing the lowest and highest)

@soulomoon soulomoon enabled auto-merge (squash) March 7, 2024 10:53
Copy link
Collaborator

@jhrcek jhrcek left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@soulomoon soulomoon merged commit 8a8f59b into haskell:master Mar 7, 2024
39 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.

4 participants