Heuristic for pattern matching untagged variants. #7128
Conversation
cristianoc
force-pushed
the
uncurried-variant-heuristic
branch
from
bdc0d05 to
c825694
Compare
| } | ||
|
|
||
| function ff(x) { | ||
| if (x === "G" || x === "F" || x === "E" || x === "D" || x === "C" || x === "B" || x === "A") { |
There was a problem hiding this comment.
@cknitt see this case that is now worse
This is why I propose a heuristic based on how many cases with, and without payloads exist.
To determine which code to generate.
There was a problem hiding this comment.
@cometkim do you have a suggestion on how to set the threshold to maximise things like perf/code size etc?
There are these 2 ways of generating code. One is clearly better with a lot of cases without payload (ff here). The other is better when there are very few cases without payload and many cases with payload (e.g. json). The question is how to decide in the middle.
There was a problem hiding this comment.
@cknitt see this case that is now worse
This is why I propose a heuristic based on how many cases with, and without payloads exist. To determine which code to generate.
Yes, sounds good to me, a heuristic-based approach will definitely improve the current situation without making things worse like in the example here. 👍
There was a problem hiding this comment.
I think this improves the predictability because the output is easier to understand. Untagged variants are not generally expected to be the best performance.
However, from a perf perspective, typeof checks are always slightly better than full literal checks, especially if it allows you to skip other branches.
I value how much more understandable it is rather than the actual code size. If the number of circuits is the same or similar, there is no real difference. It's just that typeof checks tend to be harder to read and understand when they get complex.
There are these 2 ways of generating code. One is clearly better with a lot of cases without payload (ff here). The other is better when there are very few cases without payload and many cases with payload (e.g. json). The question is how to decide in the middle.
Not sure if possible, I'd guess it would be nice to use the current on the default strategy but do the opposite if the check matches the _ pattern.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Syntax Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.
| Benchmark suite | Current: b12f189 | Previous: e1b7fb7 | Ratio |
|---|---|---|---|
Parse RedBlackTree.res - time/run |
1.3672911466666666 ms |
1.2123143266666667 ms |
1.13 |
This comment was automatically generated by workflow using github-action-benchmark.
|
@cknitt I have chosen size of generated code as heuristic, ready for final review. Btw |
tests/tests/src/UntaggedVariants.mjs
Outdated
| @@ -386,11 +386,11 @@ function check$1(s) { | |||
| return; | |||
| } | |||
| let match = s[0]; | |||
| if (match === true) { | |||
| if ((match === null || match === true || match === false || match === undefined) && match === true) { | |||
There was a problem hiding this comment.
@cristianoc Thanks a lot for looking into this so quickly!
Most cases look better now. But this one still feels a bit weird as the lhs of the && is obviously unnecessary.
Is it possible to improve it?
There was a problem hiding this comment.
Yes and it's not related to this PR.
You're asking for a back end code simplification.
There was a problem hiding this comment.
Ok, then I think this PR is ready to go and we can track the above case in a different issue. 👍
There was a problem hiding this comment.
with_literal_cases:
(e == Null) || (e ==true) || (e == false) || (e == Undefined)without_literal_cases:
!(e instanceof Array) && ((e == Null) || (typeof e != Object)) && (typeof e != Int) && (typeof e != String)So it is true that it generates the smaller expression (the former).
The second one must be simplified in the back-end, and not the first one.
There was a problem hiding this comment.
I've added extra simplification directly to this PR.
tests/tests/src/UntaggedVariants.mjs
Outdated
| @@ -386,11 +386,11 @@ function check$1(s) { | |||
| return; | |||
| } | |||
| let match = s[0]; | |||
| if (match === true) { | |||
| if ((match === null || match === true || match === false || match === undefined) && match === true) { | |||
There was a problem hiding this comment.
Ok, then I think this PR is ready to go and we can track the above case in a different issue. 👍
|
@cknitt ready for a final review. |
Fixes #7121