Nim icon indicating copy to clipboard operation
Nim copied to clipboard

JS codegen can produce extreme switch statements with `case a of range`

Open skilchen opened this issue 7 years ago • 13 comments

then try to compile the following silly int32 tester for the js backend:

# isInt32.nim
proc isInt32(i: int): bool =
  case i 
  of low(int32)..high(int32):
    return true
  else:
    return false

echo isInt32(1)

Unless you have lots and lots of memory and disk space, running nim js isInt32.nim will make your system unusable. So if you are adventurous enough to try it, save all relevant work before the experiment. Most likely you will have to perform a hard reset... Nim should refuse to compile harmless looking stupidities like this one. For this code on the js backend Nim tries to produce a switch statement with 4294967295 case labels. This is a little bit too much ...

skilchen avatar Aug 30 '18 19:08 skilchen

The C codegen deals with this appropriately. :-)

Araq avatar Aug 30 '18 19:08 Araq

Nim should refuse to compile harmless looking stupidities like this one

a more robust approach: setting limit on process memory usage (not GC memory as it doesn't cover everything), eg on linux ulimit -m

  • on linux this should be settable via a C api and called by Nim compiler
  • what about OSX and windows?

timotheecour avatar Sep 03 '18 11:09 timotheecour

Unless you have lots and lots of memory and disk space, running nim js isInt32.nim will make your system unusable.

Nim should refuse to compile harmless looking stupidities like this one.

Yes, but your system also shouldn't die when a rogue process attempts to allocate too much memory.

dom96 avatar Sep 03 '18 20:09 dom96

This issue has been automatically marked as stale because it has not had recent activity. If you think it is still a valid issue, write a comment below; otherwise it will be closed. Thank you for your contributions.

stale[bot] avatar Aug 04 '20 07:08 stale[bot]

@xflywind the proper fix should be to handle case x of 1 .. 65536: similarly to C codegen, without generating code bloat, eg transforming it to: if x >= 1 and x <= 65536: and then jsgen this (or directly jsgen it with those semantics)

https://github.com/nim-lang/Nim/pull/15809 just patches around the problem, and still generates inefficient code (with code bloat)

timotheecour avatar Nov 02 '20 10:11 timotheecour

I have considered this solution but a bit hard. Nim compiler outputs JS code by concatating strings. If stmt should be put in the default branch and in the front of the origin parts.

ringabout avatar Nov 02 '20 10:11 ringabout

@xflywind

I have considered this solution but a bit hard. Nim compiler outputs JS code by concatating strings. If stmt should be put in the default branch and in the front of the origin parts.

you can take a look at compiler/transf.nim which is an alternative to adding more logic to jsgen (either should work):

# This module implements the transformator. It transforms the syntax tree
# to ease the work of the code generators. Does some transformations:
#
# * inlines iterators

timotheecour avatar Nov 02 '20 10:11 timotheecour

this issue wasn't properly fixed, I'm re-opening. This:

proc main(a: char) =
  case a
  of 'a'..'z', 'A'..'Z': echo "alpha"
  of '0'..'9': echo "digit"
  else: echo "other"
main('x')

produces exhaustive switch statements (even if V8 were to optimize it, you'd be left with code that can't be minified).

And in other cases with more switch items, the compiler would bail out.

And the recommendation of using <= instead of case in code doesn't work in practice, as stdlib or 3rd party libraries, that use the typical switch case would not work (or be inefficient) with js backend.

unction main_687865857(a_687865859) {
  var F={procname:"t11311.main",prev:framePtr,filename:"/Users/timothee/git_clone/nim/timn/tests/nim/all/t11311.nim",line:0};
  framePtr = F;
    F.line = 6;
    switch (a_687865859) {
    case 97:
    case 98:
    case 99:
    case 100:
    case 101:
    case 102:
    case 103:
    case 104:
    case 105:
    case 106:
    case 107:
    case 108:
... skipping for brevity
    case 83:
    case 84:
    case 85:
    case 86:
    case 87:
    case 88:
    case 89:
    case 90:
      F.line = 7;
      rawEcho(makeNimstrLit("alpha"));
      break;
    case 48:
    case 49:
    case 50:
    case 51:
    case 52:
    case 53:
    case 54:
    case 55:
    case 56:
    case 57:
      F.line = 8;
      rawEcho(makeNimstrLit("digit"));
      break;
    default: 
      F.line = 9;
      rawEcho(makeNimstrLit("other"));
      break;
    }
  framePtr = F.prev;

  
}

proposal

Instead, jsgen should simply produce code that transforms of case 'a'..'z, 'A'..'Z' into something similar to

if a >= 'a' and a <= 'z' or a >= 'A' and a <= 'Z'

(and that transformation could be done in transf.nim before reaching jsgen as mentioned in https://github.com/nim-lang/Nim/issues/8821#issuecomment-720394101)

timotheecour avatar Nov 18 '20 17:11 timotheecour

workaround: library defined case, refs https://github.com/nim-lang/RFCs/issues/332#issuecomment-778448264

import macros
macro `case`*(a: string, branches: varargs[untyped]) = ...
case x:
of 10..13: ...
else: ...

but this would require some import std/jscase

timotheecour avatar Feb 13 '21 01:02 timotheecour

I made a patch at jsgen(when the number of branchs exceeds 1000, compiler generates if else statement

function isInt32_452984833(i_452984834) {
    var Temporary1;

  var result_452984835 = false;

  BeforeRet: do {
    Temporary1 = i_452984834;    if (((Temporary1 >= -2147483648) & (Temporary1 <= 2147483647))) {
    result_452984835 = true;
    break BeforeRet;
    }
    else {
    result_452984835 = false;
    break BeforeRet;
    }
    
  } while (false);

  return result_452984835;

}

ringabout avatar Aug 16 '21 03:08 ringabout

I made a patch at jsgen(when the number of branchs exceeds 1000, compiler generates if else statement

sounds good, do you have a WIP PR? why 1000 instead of always transforming consecutive ranges case x of 10..20 into corresponding if x >= x1 and x <= x2 ?

timotheecour avatar Aug 16 '21 05:08 timotheecour

My wip PR is here, I'm still experimenting it

https://github.com/xflywind/Nim/tree/pr_js

ringabout avatar Aug 16 '21 07:08 ringabout

@timotheecour I've done transformed for float range, it seems to me only string, bool, enum need switch , others should transform as well ?

bung87 avatar Sep 14 '22 22:09 bung87