Too many frames created

[sagotch]

https://github.com/tategakibunko/jingoo/commit/f0432b5cc10cc32fdd9916c77a897130fd58ea7c introduce a lot of new frames, when previous behavior only added values to current scope.

I do not know if a proper fix exists (or if it is needed), but we could imagine this

diff --git a/src/jg_interp.ml b/src/jg_interp.ml
index 6849b7f..5b03fdb 100755
--- a/src/jg_interp.ml
+++ b/src/jg_interp.ml
@@ -162,6 +162,14 @@ and eval_statement env ctx = function
   | ExpandStatement(expr) ->
     jg_output ctx (value_of_expr env ctx expr) ~autoescape:env.autoescape ~safe:(is_safe_expr expr)
 
+  | SetStatement(SetExpr([IdentExpr ident]), expr) ->
+    { ctx with frame_stack = match ctx.frame_stack with
+          | hd :: tl ->
+            let expr = value_of_expr env ctx expr in
+            (fun s -> if s = ident then expr else hd s) :: tl
+          | [] -> assert false
+    }
+
   | SetStatement(SetExpr(ident_list), expr) ->
     jg_frame_table ctx (fun table ->
       jg_bind_names table (ident_names_of ident_list) (value_of_expr env ctx expr)

I only wrote this example for a single value, but in case of multiple definitions ({% set a,b,c = %}) I think that building an associative list and using

fun s -> match List.assoc_opt s list with Some v -> v | None -> hd s

would be better than pushing a new Hashtbl.t. In real life, the list will never be long enough so Hashtbl.t would be better than a simple list.

[tategakibunko]

I'm not sure if I'm understanding you completely, but are you arguing that Hashtbl.t is excessive for binding a mere few variables?

I certainly agree that jg_frame_table adds a Hashtbl.t of size 10 to every call, and is probably wasteful in many cases.

I also don't particularly care if the contents of jg_frame_table are implemented in an assoc list.

[tategakibunko]

I think it's a good idea if SetStatement binds only one variable, then just push one closure instead of Hashtbl.t(size 10).

[tategakibunko]

Ahh, ok, in our old code base, we just added values to current frame in SetStatement, but in new code base, we create new frame...

[tategakibunko]

Anyway, side effect is required, we may need to pass list ref to callbacks of jg_frame_table.

 let jg_frame_table ctx f =
-  let table = (Hashtbl.create 10) in
+  let table = ref [] in
   f table;
-  jg_push_frame ctx (Hashtbl.find table)
+  jg_push_frame ctx (fun name -> List.assoc name !table)

 let jg_set_value table name value =
-  Hashtbl.add table name value
+  table := (name, value) :: !table

[sagotch]

Just a side note, while I was re-reading issues, I saw that you talked about a Hashtbl.t of size 10

With current implementation, it can not exists. Hashtbl.t always have a power of two size, and the minimum is 16.

https://github.com/ocaml/ocaml/blob/trunk/stdlib/hashtbl.ml#L73