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
compilation of "module rec" makes runtime assumptions on closure representation #5500
Comments
Comment author: @xavierleroy The current compilation scheme for recursive modules goes out of its way to ensure that recursive functions thus defined run as fast as functions defined using "let rec". Your patch breaks this optimization completely, and I am pretty sure it degrades performance significantly for programs that use recursive modules. So, I cannot accept this patch. I don't know how js_of_ocaml works, but would it be possible to achieve correct results by providing an alternative implementation of camlinternalMod to be used for js_of_ocaml-ized code, without changing the OCaml compiler? |
Comment author: @lefessan I think js_of_ocaml uses Javascript closures to represent OCaml closures. Maybe Javascript has a way to access and modify its own closures, but it would really surprise me (that's the kind of things you don't want to expose to programmers, especially on the web). Do you have benchmarks for "module rec", or applications where performance of "module rec" is critical ? I would like to measure the difference of performance between the two implementations. |
Comment author: @gasche Alain has his patch to turn any module into a self-recursive one for backward declarations. I suppose he's therefore an heavy user of recursive modules and could provide you with the necessary performance information. |
Comment author: @xavierleroy The "A / ASet" example of recursive modules from the OCaml manual (section 7.8) could make a decent microbenchmark. Discussing the issue with the js_of_ocaml developers would help, too. |
Comment author: @lefessan [edit: english translation below] Comme il ne contient rien de personnel, je recopie la réponse de jérôme à mon mail l'informant du problème et du patch: Salut Fabrice,
On a le même genre de problème avec les récursions entre fonctions et Ce serait bien si ton patch arrive à passer. :-) -- Jerome As it is not intended to be private, I quote here Jérôme's answer to my email about the issue and this patch:
|
Comment author: @lefessan Gabriel: Alain uses recursive modules only for typing, not for compiling, so the "module rec" compilation scheme does not apply in his case, I think. |
Comment author: @lefessan I ran the attached micro-benchmark (you know what I think about such benchmarks ;-) ), using the manual example. The new version is only 25% slower than the old one. |
Comment author: Julien Signoles only 25% slower? That is huge! |
Comment author: @lefessan The micro-benchmark is 25% slower, which means that a real application would probably not see any difference (when I have speed-up of 200% on micro-benchmarks, people usually tell me that it means only a few % on a real application, so it is the same reasoning... ;-) ) |
Comment author: @gasche The problem with the reasoning in this case is that recursive functions are at the heart of quite a lot of performance-critical code paths (while that is much less clear with recursive values). Your "micro-benchmark" is actually a very reasonable use of recursive modules for a non-stupid purpose (mutually recursively defining datatypes and balanced set of those datatypes) which may be representative of real-world code. Indeed I have myself observed comparison functions to be the bottleneck of very real code examples. I'm wondering whether it is possible to fix this issue without impacting performances as your current patch does. If I understand correctly, recursive closure compilation does not use a backpatching semantics, it uses absolute position prediction to generate a shared closure for the mutually defined functions. Would it be possible to use the same trick for mixed function/value recursion, or module recursions? Define the values first then build a shared closure for the functions. I'm really not sure of the absolute feasability of this, and I'm sure that getting a good patch, giving the subtlety and importance of those parts of the compiler, would be a lot of work. |
Original bug ID: 5500
Reporter: @lefessan
Status: resolved (set by @xavierleroy on 2012-02-05T10:20:49Z)
Resolution: suspended
Priority: normal
Severity: minor
Version: 3.12.1
Category: back end (clambda to assembly)
Monitored by: @gasche @glondu @jberdine "Julien Signoles" @alainfrisch
Bug description
When compiling "module rec", the compiler makes assumptions on the fact that "camlinternalMod.ml" will be able to patch the closure in place.
This assumption prevents correct compilation of projects containing "module rec" with compilers changing the representation of closures (js_of_ocaml in my case).
The attached patch fixes the problem, by using references instead. Each closure is compiled as (fun _ -> ! (prototypes.(pos)) ), where prototypes.(pos) is initialized as (ref (fun _ -> raise (Undefined_recursive_module ..))). Then, the reference is modified when the final closure is known. This process makes no assumption on the representation of closures.
I didn't do any performance test, as I was only interested in having the application compiled, but I assume that the extra indirection is not very expensive with modern processors.
File attachments
The text was updated successfully, but these errors were encountered: