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

Inefficient code emitted for pattern match on unboxed variants #7121

Closed
cknitt opened this issue Oct 23, 2024 · 6 comments · Fixed by #7128
Closed

Inefficient code emitted for pattern match on unboxed variants #7121

cknitt opened this issue Oct 23, 2024 · 6 comments · Fixed by #7128

Comments

@cknitt
Copy link
Member

cknitt commented Oct 23, 2024

let f = (x: JSON.t) => switch x {
  | Null => Console.log("null")
  | _ => ()
}

is compiled to

function f(x) {
  if (!(!Array.isArray(x) && (x === null || typeof x !== "object") && typeof x !== "number" && typeof x !== "string" && typeof x !== "boolean")) {
    return ;
  }
  console.log("null");
}

Ideally, this would just be

function f(x) {
  if (x === null) {
    console.log("null");
  }
}
@cristianoc
Copy link
Collaborator

cristianoc commented Oct 23, 2024

Once the conditional is expressed in this order, no further simplification is possible (the cases are exhaustive only if you carry around the type, which you don't).

So an intervention needs to happen at the point where the conditional is generated.

If one writes the conditional by hand, with variations on negation, order, etc, it would be interesting to see which cases are problematic.

@cristianoc
Copy link
Collaborator

@cristianoc
Copy link
Collaborator

cristianoc commented Oct 24, 2024

Here's another example:

@unboxed
type rec t =
  | Boolean(bool)
  | @as(null) Null
  | @as(undefined) Undefined
  | String(string)
  | Number(float)
  | Object(Js_dict.t<t>)
  | Array(array<t>)
  
let f = (x: t) =>
  switch x {
  | Null | Undefined => Console.log("abc")
  | _ => ()
  }

It seems to happen when all the nullary cases have the same rhs.

@cknitt
Copy link
Member Author

cknitt commented Oct 24, 2024

@cknitt have you observed similar, but not identical cases?

I cannot say for sure. I have observed this behavior (or similar?) a few times before creating this issue, but unfortunately did not collect the exact code involved.
Will let you know when I find a different example.

@cknitt
Copy link
Member Author

cknitt commented Oct 24, 2024

It seems to happen when all the nullary cases have the same rhs.

But even with different rhs,

@unboxed
type rec t =
  | Boolean(bool)
  | @as(null) Null
  | @as(undefined) Undefined
  | String(string)
  | Number(float)
  | Object(Js_dict.t<t>)
  | Array(array<t>)
  
let f = (x: t) =>
  switch x {
  | Null  => Console.log("abc")
  | Undefined  => Console.log("def")
  | _ => ()
  }

yields

function f(x) {
  if (!(!Array.isArray(x) && (x === null || typeof x !== "object") && typeof x !== "number" && typeof x !== "string" && typeof x !== "boolean")) {
    return ;
  }
  if (x === null) {
    console.log("abc");
    return ;
  }
  console.log("def");
}

but could be

function f(x) {
  if (x === null) {
    console.log("abc");
  }
  else if (x === undefined) {
    console.log("def");
  }
}

?

@cristianoc
Copy link
Collaborator

OK the behaviour makes sense.
What the compiler is saying is: if "it's one of the cases with payload" then....
This would be if "x is an object" without unboxed, which is very simple.

With unboxed, there are 2 possibilities of saying the same thing:

  1. it's one of the cases with payloads
  2. it's not one of the cases without payload
    Depending on the specific definition, either 1 or 2 is better.

In these examples, 2 would be better.
Choosing between 1 and 2 seems simpler than trying to change the order, as the order comes out of the pattern matching compiler, and there are many cases one could consider.

@cknitt thoughts?

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 a pull request may close this issue.

2 participants