fix a caching problem in OO code causing a 4.11 performance regression

[gasche]

The regression was introduced by #9096 in this part of the diff:

             | Tcf_method (name, _, Tcfk_concrete (_, exp)) ->
-                let met_code = msubst true (transl_exp exp) in
+                let scopes = enter_method_definition ~scopes name.txt in
+                let met_code =
+                  msubst true (transl_scoped_exp ~scopes exp) in

transl_exp would use Translobj.oo_wrap on functions, while the new
transl_scoped_exp does not.

The regression was first reported by @giltho in #10092, as it
results in a large slowdown (2x slower) on the Gillian codebase, which
uses an object-oriented visitor in a performance-critical way.

[gasche]

Here is a microbenchmark that reproduces the problem:

let test n =
  let obj = object
    method add42 i = 42 + i
  end in
  obj#add42 n

let n_iters = int_of_string Sys.argv.(1)

let () =
  for i = 1 to n_iters do
    ignore (test i)
  done

On my machine, ./oo.opt 10_000_000 takes

• 0.20s on 4.10 (or with this PR)
• 0.87s on 4.11 (or trunk)

In this microbenchmark, class initialization takes more time than actual usage of the object. I suppose that this corresponds to real-world use-cases where object-oriented visitors are created locally within traversal functions, but then used on very small inputs most of the time.

[garrigue]

I see another occurrence of transl_scoped_exp inside Translcore.transl_bound_exp.
Is that one safe? It seems to me that it is again going to miss a call to oo_wrap.

[lpw25]

This PR fixes transl_scoped_exp so other calls to it should be fine.

The underlying problem was that transl_scoped_exp called straight into transl_function when it saw a function rather than going through transl_exp. This is so that it skips the logic in transl_exp0 that adds an additional scope when it sees a function, but it also skipped the logic for calling Translobj.oo_wrap by mistake.

[gasche]

The fix adds an oo_wrap in transl_scoped_exp itself, in such a way that transforming transl_exp in transl_scoped_exp should preserve the amount of oo-wrapping. In particular, the direct call to transl_scoped_exp which I pointed to is fixed, but transl_bound_exp (the other callsite for transl_scoped_exp) also gains the property that its amount of wrapping is equivalent to transl_exp, so its two calls callsites are now also safe. I believe that the regression observed in the wild (and my repro case) came from the callsite I pointed at, but the uses of transl_bound_exp may have been cause for regressions of their own (I am not sure).

[gasche]

Excellent, we now have two reviewers for this PR!

[lpw25]

This fixes the bug, but there is still a call to:

List.iter (Translattribute.check_attribute e) e.exp_attributes;

is transl_exp that is being inadvertently left out by transl_scoped_exp.

This pattern seems quite error prone. I think replacing transl_scoped_exp by an additional boolean parameter on transl_exp would be better. Perhaps renaming transl_exp to transl_exp1 and then having transl_exp and transl_scoped_exp that call into it with the boolean parameter set to true and false respectively.

[gasche]

@lpw25 Thanks for the suggestion. I pushed a new commit in the direction that you suggest. This makes the code nicer, but the change is also more invasive, so I guess it should be reviewed carefully.

[lpw25]

LGTM. I'd probably have left transl_bound_exp as it was, but the patch is fine either way.

[gasche]

There is an interesting question here, asked at an interesting time of the year: do we want to backport this fix to 4.11 and make a new minor release? Personally I think this would be a good idea, given that (4.11 is still the last release and) we are fixing a performance regression that can be very large (2x slowdown) for some users workflow existing in the wild. (cc @Octachron)

[nojb]

There is an interesting question here, asked at an interesting time of the year: do we want to backport this fix to 4.11 and make a new minor release? Personally I think this would be a good idea, given that (4.11 is still the last release and) we are fixing a performance regression that can be very large (2x slowdown) for some users workflow existing in the wild. (cc @Octachron)

I haven't checked, but I suspect LexiFi's codebase (which uses objects heavily but has not yet been ported to 4.11) would be impacted by this. Based on that, I would be in favour of backporting to 4.11, even if there is no minor release done.

[gasche]

What I will do is to include this change in a "4.11 maintenance branch" section of the Changes, and backport in the 4.11 branch. We can decide having a bugfix release later on.

[Octachron]

I agree that this seems worthwhile to backport this fix to both 4.12 and 4.11 .

[gasche]

I cherry-picked in 4.11 ( 3e3d7f9 ) and 4.12 ( fc5061c ).