Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Some GADT matches cause closure allocation with ocamlc -g #7345

Closed
vicuna opened this issue Sep 8, 2016 · 5 comments
Closed

Some GADT matches cause closure allocation with ocamlc -g #7345

vicuna opened this issue Sep 8, 2016 · 5 comments
Labels
back-end bug Stale

Comments

@vicuna
Copy link

vicuna commented Sep 8, 2016 •
edited by stedolan

Original bug ID: 7345
Reporter: @mshinwell
Status: acknowledged (set by @mshinwell on 2016-09-08T11:07:36Z)
Resolution: open
Priority: normal
Severity: minor
Version: 4.04.0 +dev / +beta1 / +beta2
Target version: later
Category: back end (clambda to assembly)
Related to: #7384
Monitored by: @hcarty

Bug description

It looks like let bindings introduced to alias function parameters holding constructed values of inline record type cause debugging events to be inserted that subsequently cause closure allocation. The allocations happen because the code for combining closures in Simplif (see simplif.ml:464) won't apply owing to the intervening events.

This doesn't happen from 4.04 onwards with native code after the changes to remove debugging events in that context. It still happens with bytecode, however.

For the example below:

     f/1207 =
       (function param/1281
         (funct-body inline_allocation.ml(10)<ghost>:130-153
           (let (a/1209 =a param/1281)   (* this let causes the problem *)
             (before inline_allocation.ml(10)<ghost>:136-153
               (function z/1210
                 (funct-body inline_allocation.ml(10)<ghost>:136-153
                   (before inline_allocation.ml(11):142-153
                     (after inline_allocation.ml(11):142-153
                       (apply equal/1204 (field 0 a/1209) z/1210)))))))))

Steps to reproduce

Compile and run the following using ocamlc (not ocamlopt).
The answer should be zero, but it prints four.

type _ t = 
  | A : { x : int } -> [`A] t
  | B : [`B] t

let _unused_constr = B 

let equal (x : int) (y : int) =
  x = y

let f (A a) z =
  equal a.x z

let t = A { x = 5 } 

let () =
  let start = Gc.minor_words () in
  let (_res : bool) = f t 3 in
  let stop = Gc.minor_words () in
  Printf.printf "%f\n" (stop -. start -. 2.)
@vicuna
Copy link
Author

vicuna commented Dec 7, 2016

Comment author: @xavierleroy

Just from reading the description of this PR, not going back to the code:

  • It is strange that debugging events are inserted for these compiler-generated "let"s. Are those "let"s of the correct kind? (In the sense of type Lambda.let_kind.) Do we need an extra "let" kind?

  • Playing devil's advocate: so we have a slight performance degradation in bytecode when compiled with -g. Who really cares?

@vicuna
Copy link
Author

vicuna commented Feb 18, 2017

Comment author: @xavierleroy

Playing some more with the repro case:

  • Inline records are innocent: take

type t = A of {x: int}

in your example and the intermediate closure goes away.

  • GADTs have something to do with it. If t is not a GADT

type t = A of {x: int} | B

you get lambda code of the form

 f/1211 =
   (function param/1214
     (if param/1214
       (function z/1213 (apply equal/1208 (field 0 param/1214) z/1213))
       (raise (makeblock 0 (global Match_failure/17g) [0: "gee.ml" 8 6])))))

even if -g is not given. That makes sense because f must take one argument, check that it is indeed an A, then return a function taking the second parameter. Taking the two parameters at once then doing the matching on the first would be semantically incorrect.

With a GADT, the "B -> raise Match_failure" branch is eliminated, I don't know where, but the debug event was already inserted.

For the time being I'm pushing this to 4.06. But I suggest to close this PR as not worth fixing.

@vicuna
Copy link
Author

vicuna commented Oct 10, 2017

Comment author: @xavierleroy

This issue won't be looked at in time for 4.06. I move it to "later" as I doubt anyone really cares about it.

@vicuna vicuna mentioned this issue Mar 14, 2019
Closed
@vicuna vicuna added the bug label Mar 20, 2019
@github-actions
Copy link

github-actions bot commented May 9, 2020

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label May 9, 2020
@stedolan
Copy link
Contributor

This was fixed in 4.06.0. (I think by #1195)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
back-end bug Stale
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@stedolan @vicuna