Skip to content

CP-308690: Avoid unnecessary condition evaluations #6575

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

Conversation

minglumlu
Copy link
Member

The "<?>" operator was introduced to simplify deeply nested conditionals. However, it also led to unintended side effects: all condition expressions are evaluated eagerly, even when some may not be needed.

This happens because OCaml uses call-by-value semantics, meaning function arguments are evaluated before the function is applied.

The change makes the evaluations lazily to avoid unnecessary condition evaluations.

The "<?>" operator was introduced to simplify deeply nested conditionals.
However, it also led to unintended side effects: all condition expressions
are evaluated eagerly, even when some may not be needed.

This happens because OCaml uses call-by-value semantics, meaning function
arguments are evaluated before the function is applied.

The change makes the evaluations lazily to avoid unnecessary condition
evaluations.

Signed-off-by: Ming Lu <[email protected]>
let cmp2 () = compare t1.bus t2.bus in
let cmp3 () = compare t1.dev t2.dev in
let cmp4 () = compare t1.fn t2.fn in
(cmp1 <?> cmp2 <?> cmp3 <?> cmp4) ()
Copy link
Contributor

Choose a reason for hiding this comment

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

An optional implementation with lazy module

    let ( <?> ) a b = if a = 0 then Lazy.force b else a in
    compare t1.domain t2.domain
    <?> lazy (compare t1.bus t2.bus)
    <?> lazy (compare t1.dev t2.dev)
    <?> lazy (compare t1.fn t2.fn)

@last-genius
Copy link
Contributor

last-genius commented Jul 7, 2025

I think we want to avoid further usages of Lazy in our codebase, I remember it not being particularly thread-safe.

Overall, I am not sure it's worth optimizing four comparisons of integer pairs, the alternative suggested produces more code, if anything (it also allocates more, can call the GC, reallocate the stack):

https://godbolt.org/z/oW177ca3h

Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

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

These are typically integer comparisons which are fast and memory access. Is the extra work introduced by lazy really providing a benefit? How was the problem manifesting itself?

@minglumlu
Copy link
Member Author

I think the origin of the problem is to avoid nested conditionals (but I guess with minimum machine code and best performance) like:

if (...) then (
  if (...) then (
    if (...) then (
      if (...) then (

Then the code was improved like:

match (cmp1, cmp2, cmp3, cmp4) with
| x, _, _, _  when x <> 0 -> x
| 0, x, _, _  when x <> 0 -> x
....

As you see, this was comment as introducing unnecessary evaluations. So the point is only for the style, instead of performance.

But if what really does matter here is only the number here, I can close this PR as indeed this bit is not in a critical perf path.

@minglumlu minglumlu closed this Jul 11, 2025
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