ItsDeltin / Overwatch-Script-To-Workshop

Converts scripts to Overwatch workshops.
208 stars 26 forks source link

Proposal: Optimize switch statement in special (but common) cases #137

Closed OrangeUtan closed 4 months ago

OrangeUtan commented 4 years ago

The current switch statement is implemented with a, lets call it, "LookupTable". With the new update the same execution path can be achieved using if/elseIf actions instead.

Example OSTW Code:

switch(x) {
    case 0: 
        SmallMessage(EventPlayer(), "0");
        break;
    case 1: SmallMessage(EventPlayer(), "1");
    case 2: SmallMessage(EventPlayer(), "2");
}

Current transpiled code:

// "LookupTable"
Skip(Value In Array(Append To Array(Append To Array(Append To Array(Append To Array(Empty Array, 4), 0), 2), 3), Add(Index Of Array Value(Append To Array(Append To Array(Append To Array(Empty Array, 0), 1), 2), Global Variable(x)), 1)));
Small Message(Event Player, Custom String("0", Null, Null, Null));
Skip(2); // Break statement of first case
Small Message(Event Player, Custom String("1", Null, Null, Null));
Small Message(Event Player, Custom String("2", Null, Null, Null));

Proposed transpiled code:

If(Compare(Global Variable(x), ==, 0));
    Small Message(Event Player, Custom String("0", Null, Null, Null));
Else If(Compare(Global Variable(x), ==, 1));
    Small Message(Event Player, Custom String("1", Null, Null, Null));
    Skip(1); // If/Else/ElseIf actions can be skipped. This enables fallthrough behaviour
Else If(Compare(Global Variable(x), ==, 2));
    Small Message(Event Player, Custom String("2", Null, Null, Null));
End;

Some things to consider:

In the example above, the current method produces 90 Elements, while the proposed method produces only 65 Elements. So at least in this case, the proposed method can be significantly better, Element Count wise.

Some findings through analysis:

As one can see, the current method uses less Elements per NB-cases, but more Elements per WB-case. It also has more boilerplate. Generally, as long as NB <= 2WB + 19/2 (roughly half as many non-breaking cases as breaking cases), the proposed method is shorter.

To give a more visual example:

switch(x) {
    case 0:
        ...
        break;
    case 1:
        ...
        break;
    case 2:
        ...
        break;
}

Because there are more WB-Cases, the proposed method is better.

switch(x) {
    case 0:
        ...
    case 1:
        ...
    case 2:
        ...
}

Because there are more NB-Cases, the current method is better. However, because of the boilerplate of the current method, for < 9 cases the proposed method is always better. Here is a small insight of when the proposed method is better:

WB-Cases Smallest number of NB-Cases nedded so that current method is better Total cases
0 10 10
1 12 13
2 14 16
3 16 19
4 18 22
5 20 25
6 22 28
7 24 31
... ... ...
34 78 112

As one can see, for the current method to be better than the proposed method, there have to be quite a lot of cases that do not break. Seeing as switch statements are often used as replacements for huge if/else-if structures and cases without a break are rare in general, implementing the proposed method would optimize the length of the transpiled code. However, considering there is a way to calculate which method is better, the best solution would be to consider which method to use during transpilation (i.e. leave the old method and also implement the new).

Problems with the proposed method:

Problematic scenario:

switch(HeroOf(FirstOf(AllPlayers()))) {
        case 0: 
            ...
            break
        case 1: 
            ...
            break
    }

Solution:

x = HeroOf(FirstOf(AllPlayers()))
switch(x) {
        case 0: 
            ...
            break
        case 1: 
            ...
            break
    }
Zezombye commented 4 years ago

But why not directly use an if-else chain instead of a switch, then :p

OrangeUtan commented 4 years ago

In Overwatch switch statements can only be syntactic sugar, but they offer better readability:

if(FirstOf(AllPlayers()) == Hero.Ana) {
 ...
} else if(FirstOf(AllPlayers()) = Hero.Reihardt) {
 ...
} else if(FirstOf(AllPlayers()) = Hero.Symmetra) {
 ...
}

vs

switch(FirstOf(AllPlayers()))  {
 case Hero.Ana: ...
 case Hero.Reinhardt: ...
 case Hero.Symmetra: ...
}

Especially fallthrough behaviour is easier to understand with the switch statement. Its kind of ridiculous to implement switch with if-else chains, but hey, at the end of the day, I want to have readable code and don't have to type 20 unnecessary "else if (...)" statements.

Zezombye commented 4 years ago

Right, though switches are also easier for performance (if you are switching on an expensive value). Therefore compiling them to if/else could not be a wanted behavior.

You should recalculate the elements saved btw, as the method to calculate elements has changed on PTR.

OrangeUtan commented 4 years ago

Right, though switches are also easier for performance

You are certainly right about most languages, like C, Java, etc. They create a lookup table at compile time which speeds thinks up considerably. However, Overwatch does not have a switch statement. Thats why OSTW transpiles its own implementation of a lookup table. Here the lookup table is a long array that is constructed every time the code is executed. I don't see how that has any performance benefits over simple else-if statements.

You should recalculate the elements saved btw, as the method to calculate elements has changed on PTR.

I will. The only benefit of this proposal is that it uses less elements. It is neither slower nor faster than the current method (to my knowledge).

EDIT: I forgot this:

if you are switching on an expensive value

Yes, you are certainly right about that. I talked about this in the top comment. The proposed method is only better in certain cases. However there is a way to calculate when the proposed method is better. And you can save the expensive condition in a temporary variable. The proposed method isn't always better, thats why I said the current method souldn't be thrown away

Protowalker commented 4 years ago

Marked as invalid - not because I don't believe there might be merits to this method, but because I currently don't see any noticeable benefit. If someone is able to show a situation where this is better, I'll be happy to mark it with enhancement