Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0006217OCamlOCaml backend (code generation)public2013-10-30 15:082016-06-26 17:39
Reporterppedrot 
Assigned Togasche 
PrioritynormalSeveritytweakReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version4.01.0 
Target Version4.03.1+devFixed in Version4.04.0 +dev / +beta1 / +beta2 
Summary0006217: Compilation of record functional modification is costly
DescriptionCurrently, compilation of the { r with ... } expression seems to have a length threshold that triggers a change in the way it is compiled. For instance,

type t = {
  t1 : int option;
  t2 : int option;
  t3 : int option;
  t4 : int option;
  t5 : int option;
}

let set1 x t = {
  t with t1 = Some x;
}

produces a direct allocation on the minor heap, together with a bunch of assembly moves that copy the record fieldwise. Problem is, adding a [t6 : int option] field changes this to an [Obj.dup] followed by a [caml_modify].

I don't know if this has been decided with respect to some benchmarks, but [caml_modify] is really costly, and for the record (no pun intended), this overhead was indeed observable in Coq. Maybe this threshold should be incremented?
Tagsjunior_job
Attached Files

- Relationships

-  Notes
(0012139)
doligez (administrator)
2014-09-15 23:17

There is another problem with this: if the record is immutable it's possible that, in the near future, the call to [caml_modify] will break the GC's invariants.
(0012154)
bobot (reporter)
2014-09-17 10:50

The current heuristic is `3 + 2 * number_of_modified_file >= number_of_field` for the piece by piece copy (transl_record in translcore.ml)

Instead of modifying this heuristic and for taking into account Damien's comment, can't we just replace the setfield:

 `Psetfield(lbl.lbl_pos, maybe_pointer expr)` by
 `Psetfield(lbl.lbl_pos, false)`

Since we are sure that the values assigned are older than the duplicated record?
(0012674)
gasche (developer)
2014-12-06 00:06

Note: Fran├žois Pottier also hit this limit when experimenting with converting the Menhir parser generator from a mutable field update to a pure record update. While this comes at basically no cost if the state of the parser is a record of 5 fields, changing it to 6 fields gives an overall 30% performance overhead (in the generated parsers).
(0013111)
doligez (administrator)
2015-01-15 19:00

@bobot, it's not enough to be sure that the assigned values are older than the record, you also have to be sure that the record is in the minor heap.
(0015039)
madroach (reporter)
2015-12-04 21:01

@doligez
Wouldn't this be as easy to do as comparing the record size to Max_young_wosize ?
(0015703)
Otini (reporter)
2016-04-10 20:19

I have run some benchmarks, and fieldwise copy is indeed always faster while the new record is allocated on the minor heap, that is, while it has less than 256 fields. For bigger records (up to 513 fields) the second method is 1.5 to 4 times faster.
(0015998)
gasche (developer)
2016-06-26 17:39

This issue was solved by merging Otini's pull request in trunk ( b3216f0f84a33811fe5c446349f0601470e267ee )

  https://github.com/ocaml/ocaml/pull/538 [^]

Now the immutable field-by-field copy is always preferred below max_young_wosize (the maximal size of objects that go in the minor heap), that is currently 256.

(The pull request is a bit noisy with three commits instead of one, apologies for forgetting to rebase.)

- Issue History
Date Modified Username Field Change
2013-10-30 15:08 ppedrot New Issue
2014-06-19 17:42 gasche Tag Attached: junior_job
2014-07-16 13:49 doligez Status new => acknowledged
2014-07-30 21:01 doligez Target Version => 4.02.1+dev
2014-09-04 00:25 doligez Target Version 4.02.1+dev => undecided
2014-09-15 23:17 doligez Note Added: 0012139
2014-09-15 23:17 doligez Target Version undecided => 4.02.2+dev / +rc1
2014-09-17 10:50 bobot Note Added: 0012154
2014-12-06 00:06 gasche Note Added: 0012674
2015-01-15 19:00 doligez Note Added: 0013111
2015-01-15 19:01 doligez Target Version 4.02.2+dev / +rc1 => 4.02.3+dev
2015-07-10 18:19 doligez Target Version 4.02.3+dev => 4.03.0+dev / +beta1
2015-12-04 21:01 madroach Note Added: 0015039
2016-04-10 20:19 Otini Note Added: 0015703
2016-04-14 17:06 doligez Target Version 4.03.0+dev / +beta1 => 4.03.1+dev
2016-06-26 17:39 gasche Note Added: 0015998
2016-06-26 17:39 gasche Status acknowledged => resolved
2016-06-26 17:39 gasche Fixed in Version => 4.04.0 +dev / +beta1 / +beta2
2016-06-26 17:39 gasche Resolution open => fixed
2016-06-26 17:39 gasche Assigned To => gasche


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker