Browse Source

avoid creating instance method closures with .bind (#10737) (#10738)

* avoid creating instance method closures with .bind (#10737)

* don't create tempvars for locals, add test (closes #10737)

* revert an incorrect optimization

* also test that the functionality doesn't break (really closes #10737)

* simplify
Dan Korostelev 3 years ago
parent
commit
627b616285
3 changed files with 59 additions and 1 deletions
  1. 10 0
      src/typing/calls.ml
  2. 30 1
      tests/optimization/src/TestJs.hx
  3. 19 0
      tests/unit/src/unit/issues/Issue10738.hx

+ 10 - 0
src/typing/calls.ml

@@ -361,6 +361,16 @@ let type_bind ctx (e : texpr) (args,ret) params p =
 			e,var_decls
 			e,var_decls
 		| TField(_,(FStatic(_,cf) | FInstance(_,_,cf))) when is_immutable_method cf ->
 		| TField(_,(FStatic(_,cf) | FInstance(_,_,cf))) when is_immutable_method cf ->
 			e,var_decls
 			e,var_decls
+		| TField(eobj,FClosure(Some (cl,tp), cf)) when is_immutable_method cf ->
+			(*
+				if we're binding an instance method, we don't really need to create a closure for it,
+				since we'll create a closure for the binding anyway, instead store the instance and
+				call its method inside a bind-generated closure
+			*)
+			let vobj = alloc_var VGenerated "`" eobj.etype eobj.epos in
+			let var_decl = mk (TVar(vobj, Some eobj)) ctx.t.tvoid eobj.epos in
+			let eobj = { eobj with eexpr = TLocal vobj } in
+			{ e with eexpr = TField(eobj, FInstance (cl, tp, cf)) }, var_decl :: var_decls
 		| _ ->
 		| _ ->
 			let e_var = alloc_var VGenerated "`" e.etype e.epos in
 			let e_var = alloc_var VGenerated "`" e.etype e.epos in
 			(mk (TLocal e_var) e.etype e.epos), (mk (TVar(e_var,Some e)) ctx.t.tvoid e.epos) :: var_decls
 			(mk (TLocal e_var) e.etype e.epos), (mk (TVar(e_var,Some e)) ctx.t.tvoid e.epos) :: var_decls

+ 30 - 1
tests/optimization/src/TestJs.hx

@@ -488,6 +488,8 @@ class TestJs {
 	static function call(d1:Dynamic, d2:Dynamic) { return d1; }
 	static function call(d1:Dynamic, d2:Dynamic) { return d1; }
 	@:pure(false)
 	@:pure(false)
 	static public function use<T>(t:T) { return t; }
 	static public function use<T>(t:T) { return t; }
+	@:pure(false)
+	static function run(f:()->Void) {}
 
 
 	static var intField = 12;
 	static var intField = 12;
 	static var stringField(default, never) = "foo";
 	static var stringField(default, never) = "foo";
@@ -647,6 +649,28 @@ class TestJs {
 		var f = Issue9227.new.bind(1);
 		var f = Issue9227.new.bind(1);
 		f(3);
 		f(3);
 	}
 	}
+
+	@:js('
+		var c = new Issue10737();
+		var _g = c;
+		var value = 42;
+		TestJs.run(function() {_g.process(value);});
+		c = null;
+	')
+	static function testIssue10737_avoidInstanceMethodClosure() {
+		var c = new Issue10737();
+		run(c.process.bind(42));
+		c = null;
+	}
+
+	@:js('
+		var _g = new Issue10737();
+		var value = 42;
+		TestJs.run(function() {_g.process(value);});
+	')
+	static function testIssue10737_avoidInstanceMethodClosure2() {
+		run(new Issue10737().process.bind(42));
+	}
 }
 }
 
 
 class Issue9227 {
 class Issue9227 {
@@ -665,4 +689,9 @@ abstract Issue8751Int(Int) from Int {
 	inline public function toInt():Int {
 	inline public function toInt():Int {
 		return this;
 		return this;
 	}
 	}
-}
+}
+
+class Issue10737 {
+	public function new() {}
+	public function process(value:Int) {}
+}

+ 19 - 0
tests/unit/src/unit/issues/Issue10738.hx

@@ -0,0 +1,19 @@
+package unit.issues;
+
+class Issue10738 extends Test {
+	function test() {
+		var c = new C();
+		func = c.process.bind(42);
+		c = null;
+		eq(42, func());
+	}
+
+	var func:()->Int;
+}
+
+private class C {
+	public function new() {}
+	public function process(value:Int):Int {
+		return value;
+	}
+}