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.

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

Jump to bottom

Reduce allocations by optimizing branching and black holes #5492

Merged
merged 6 commits into from
Dec 10, 2024
Merged

Reduce allocations by optimizing branching and black holes #5492

merged 6 commits into from
Dec 10, 2024

Conversation

ChrisPenner
Copy link
Contributor

@ChrisPenner ChrisPenner commented Dec 9, 2024 •
edited
Loading

Overview

We've long had optimized Branch enums for one-branch and two-branch cases, but just never emitted them in code-gen.

Cutting down on allocations helps performance a bit by avoiding GCs

Implementation notes

This changes code-gen to use these optimized cases which skips an EnumMap lookup and more importantly, avoids superfluous allocations.

It also NOINLINE's a single black hole value so we're not dynamically allocating those.

Fixes an oversight in codeValidate which caused it to fail on recursive functions.
Dan and I think the failure was always there, but after this change things are stricter Test1 and Test2 are strict in their branches whereas TestW isn't.

Test coverage

Benchmarks:

trunk -> better-branches

fib1
326.288µs -> 314.807µs

fib2
2.308349ms -> 2.259432ms

fib3
2.724128ms -> 2.646835ms

Decode Nat
349ns -> 337ns

Generate 100 random numbers
210.259µs -> 207.108µs

List.foldLeft
2.137183ms -> 2.025939ms

Count to 1 million
128.04945ms -> 124.7779ms

Json parsing (per document)
268.95µs -> 261.794µs

Count to N (per element)
192ns -> 188ns

Count to 1000
193.911µs -> 188.466µs

Mutate a Ref 1000 times
321.721µs -> 314.031µs

CAS an IO.ref 1000 times
430.627µs -> 425.158µs

List.range (per element)
338ns -> 325ns

List.range 0 1000
351.001µs -> 345.243µs

Set.fromList (range 0 1000)
1.679125ms -> 1.592714ms

Map.fromList (range 0 1000)
1.270356ms -> 1.173887ms

NatMap.fromList (range 0 1000)
5.025834ms -> 4.871296ms

Map.lookup (1k element map)
2.633µs -> 2.546µs

Map.insert (1k element map)
7.1µs -> 6.888µs

List.at (1k element list)
299ns -> 277ns

Text.split /
33.074µs -> 32.688µs

@ChrisPenner
Copy link
Contributor Author

Hrmm, really not sure how this could cause that transcript to fail 🤔

dolio
dolio approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@dolio dolio left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@ChrisPenner ChrisPenner marked this pull request as ready for review December 10, 2024 00:04
@ChrisPenner ChrisPenner merged commit 2f2fb81 into trunk Dec 10, 2024
32 checks passed
@ChrisPenner ChrisPenner deleted the cp/better-branches branch December 10, 2024 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dolio dolio dolio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ChrisPenner @dolio