Skip to content

perf: add ghc 9.12 options -fexpose-overloaded-unfoldings -fspecialise-aggressively #5008

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.

Sign up for GitHub

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

Merged
taimoorzaeem merged 1 commit into from
Jun 17, 2026
+3 −1
Merged

perf: add ghc 9.12 options -fexpose-overloaded-unfoldings -fspecialise-aggressively #5008

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion postgrest.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,9 @@ library
if flag(hpc)
ghc-options: -fhpc -hpcdir .hpc
else
ghc-options: -O2
ghc-options: -O2
if impl(ghc >= 9.12)
ghc-options: -fexpose-overloaded-unfoldings -fspecialise-aggressively
Comment on lines +180 to +182

@taimoorzaeem taimoorzaeem Jun 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, I am not sure if we should do this globally like this. I think losing -O2 could cause unexpected performance degradation.

@mkleczek mkleczek Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hmm... this should be additive (ie. these are additional flags added to -O2), from what I understand.

@taimoorzaeem taimoorzaeem Jun 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh yeah, it is additive. My bad.

I am still skeptical if we should introduce this. I think we should do INLINABLE explicitly only on functions where considerable performance benefits are seen. Doing this globally will incrementally increase binary size as well.

@steve-chavez steve-chavez Jun 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Q: If this does bring a perf improvement, we should be able to see the results on the loadtest? So far I don't see an improvement.

@wolfgangwalther wolfgangwalther Jun 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see quite an improvement across the board. It's most notable on the jwt loadtests, but also the mixed loadtest has a tendency of having many more negative numbers than positive. Sure - there is still noise on top of that, so the highest numbers hover closely around 0%. But that's not what you expect on a no-change PR - on a no-change PR you'd expect these to be more evenly distributed around 0%.

I think we should do INLINABLE explicitly only on functions where considerable performance benefits are seen. Doing this globally will incrementally increase binary size as well.

I think the small increase in binary size is more than worth this optimization. Not having to put INLINABLE in places where we actually don't want to inline, but to specialize across modules (did I get that right?) is a positive for me...

@steve-chavez steve-chavez Jun 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@wolfgangwalther Could you elaborate on how you read the loadtests to see a perf improvement?

The load tests on #4880 leave me puzzled, they show some negatives across the board but that PR shouldn't have impacted the perf at all.

@wolfgangwalther wolfgangwalther Jun 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you elaborate on how you read the loadtests to see a perf improvement?

Sure, I'm looking at https://github.com/PostgREST/postgrest/actions/runs/27569147229?pr=5008, and the "change" column for all loadtests, but only at P50 to make this a bit shorter - you could do the same with all the others and it wouldn't change the outcome:

  • mixed: no positive number at all, From looking at it, the average is probably around -2%.
  • jwt-hs: -6.3%
  • jwt-hs-cache: -3.7%
  • jwt-hs-cache-worst: -4.2%
  • jwt-rsa: -2.1%
  • jwt-rsa-cache: -3.6%
  • jwt-rsa-cache-worst: -3.3%

Literally every test says there is a small improvement.

Compare that to a recent run on main (note I am looking at something from two days ago, because I don't think we can trust results from today / the last couple of hours at all - just look at the crazy numbers on https://github.com/PostgREST/postgrest/actions/runs/27638369318):

  • mixed shows roughly ~1-2%
  • jwt-hs: -0.8%
  • jwt-hs-cache: -0.4%
  • jwt-hs-cache-worst: 2.1%
  • jwt-rsa: -1.3%
  • jwt-rsa-cache: 0.5%
  • jwt-rsa-cache-worst: -1.2%

Some positive, some negative. Noisy, that's all.

The load tests on #4880 leave me puzzled, they show some negatives across the board but that PR shouldn't have impacted the perf at all.

I'll respond in that PR about that.


if !os(windows)
build-depends:
Expand Down