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

A handful of tentative performance improvements #295

Closed
wants to merge 5 commits into from

Conversation

krtab
Copy link
Collaborator

@krtab krtab commented Jun 5, 2024

@zapashcanon let me know if you want them in separate PRs

src/cmd/cmd_sym.ml Outdated Show resolved Hide resolved
dune-project Outdated Show resolved Hide resolved
@krtab krtab force-pushed the improv_multicore branch from 7dea337 to 658c0c3 Compare June 5, 2024 11:08
@@ -366,7 +366,7 @@ let get_model_or_stop symbol =
| Some v -> return v
end

let select (cond : Symbolic_value.vbool) =
let select_inner ?(explore_first = true) (cond : Symbolic_value.vbool) =
Copy link
Member

Choose a reason for hiding this comment

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

Could you use ~explore_first instead ? I don't really like optional arguments TBH, and here the function is used in only two places so it's not that tedious

@zapashcanon
Copy link
Member

LGTM once we checked that the 1st and 3rd one are actually leading to better results, which I would expect (+ my nitpicks comments).

@zapashcanon
Copy link
Member

@krtab, I wanted to run the benchmarks on the server, but the code actually does not typecheck

@krtab krtab force-pushed the improv_multicore branch 4 times, most recently from 8501a5a to 4e76f58 Compare June 7, 2024 13:35
@krtab krtab force-pushed the improv_multicore branch from 4e76f58 to 246b273 Compare June 7, 2024 14:23
@krtab
Copy link
Collaborator Author

krtab commented Jun 18, 2024

Split in #320 #321 #322 #323

@krtab krtab closed this Jun 18, 2024
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