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
Compile time performance of opens #5916
Comments
Comment author: @alainfrisch We already incorporated a patch to improve performance of opens in the compiler (#5877). Do you think your proposal brings an additional improvement, or maybe that it should replace the other patch? |
Comment author: @sliquister I don't understand the other patch but judging by the description, I don't think they overlap. This one is about making opening of big structure faster because even one open is already quite costly. It aims for this kind of code: |
Comment author: @lefessan The two patches are independant. From what I understood, this one lazily applies the substitution, but if there are two opens, then the substitution is (lazily) applied twice. Maybe such laziness is enough to solve the problem of multiple opens, it would need more investigation to know. Anyway, I think both patches should be applied for now. |
Comment author: @alainfrisch I'm a bit worried by the introduction of a lazy component in the environment, which needs to be serialized. Shouldn't the "explicit lazy" mechanism that you (Fabrice) introduced be used as well here? |
Comment author: @sliquister I had the impression that the environment was not serialized, but I looked at at cmi files. Do you mean serialized in cmt files? |
Comment author: @garrigue Actually, if I remember correctly the discussion we had about binannot, the full environment is not serialized, only the env_summary field is. |
Comment author: @alainfrisch Precisely, the patch adds laziness to the summary:
|
Comment author: @sliquister Well it adds laziness both to the summary and to the environment. I can change that but what is the problem with serializing lazy values? |
Comment author: @garrigue It is just that a non-evaluated lazy value contains a closure, which cannot be serialized. |
Comment author: @alainfrisch AFAIK, environment summaries are also used by the bytecode debugger. |
Comment author: @sliquister It took me some time, but I attached a new version of the patch. This one was written against a recent version of trunk, so the conflicts with Fabrice's changes have been solved. I retried the whole tree build and the performance improvement is the same. What is new though is that now my /tmp contains 50000 ocamlpp* files (and I don't compile anything else on that machine), but I did not investigate that. |
Comment author: @gasche What is the status of this patch? Alain, Fabrice, do you think it could be applied now that EnvLazy is used? PS: the ocamlpp* problem was independently submitted by sliquister (thanks!) as 0005930 and is now solved. |
Comment author: @sliquister Just a note: this patch would probably be much less useful if we had namespaces, assuming the design of namespaces allowed to only load (thus substitute) the submodules of packed modules that are referenced (or the parts of whatever would replace packed modules). |
Comment author: @alainfrisch I don't have anything against the patch but I won't have time to review it soon. Gabriel: if you review the patch and if you feel comfortable with the patch, I think you can apply it. |
Comment author: @alainfrisch I've uploaded a new version of the patch adapted to current trunk (resolving conflicts with #5965 and #5980). I'm still unable to produce a micro-benchmark to illustrate the gain. I've tried generating huge nested structures and opening only a sub-structure but I could not observe a difference in compilation time (yet). |
Comment author: @sliquister I would need to look at the patch again, but right now I can't compile the compiler at home. I think the description I initially gave was more general than what the patch actually does. Have you tried: a.ml: big structure and then comparing the compilation time of a file containing only 'open C.B' or only 'open C.A' (both should be slow without the patch whereas the first one should be fast with the patch). |
Comment author: @alainfrisch a.ml: let x1 = 1;; let x2 = 2;; ...;; let x10000 = 10000 Compiling with trunk: $ time ocamlc -c da.ml So "open C.B" already seems to be quite fast. To eliminate the effect of the memoization patch, I've also tried with a single "open C.A" vs "open C.B": $ time ocamlc -c da.ml Even there, "open C.B" is much faster than "open C.A" (without the patch). Could you run your experiment again on your large code base to confirm the impact of the patch? |
Comment author: @xavierleroy Any new information about this patch? (Inactive since 2013.) If not, it won't be considered for 4.03. |
Comment author: @sliquister I haven't tried anything new, no. I think module aliases and -no-alias-deps essentially solved this problem. |
Comment author: @xavierleroy
Right. Then, I'm "suspending" this PR to mark it as not requiring action and to keep it for future reference. |
Original bug ID: 5916
Reporter: @sliquister
Status: resolved (set by @xavierleroy on 2015-12-11T12:49:06Z)
Resolution: suspended
Priority: normal
Severity: minor
Version: 4.00.0
Target version: 4.03.0+dev / +beta1
Category: typing
Tags: patch
Has duplicate: #5877
Monitored by: @diml @alainfrisch
Bug description
The attached patch is a try at improving compilation speed.
It does two things:
The reason for this patch is because many libraries at jane street like core and async are packed as one module, and are accessed through the Std submodule. As a consequence, the aforementioned substitutions are currenty applied on the whole library (twice) when writing [open Lib.Std], as opposed to once on the modules used by the current compilation unit with the patch.
I haven't tried the patch completely by itself, but compiling (without linking) one big tree using
If we just looked at user time, the improvement would be more visible (3. would be roughly 3x faster than 1. if I remember right).
File attachments
The text was updated successfully, but these errors were encountered: