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

Make type-checking of "open" constant-time #834

Merged
merged 23 commits into from Mar 24, 2017

Conversation

Projects
None yet
9 participants
@alainfrisch
Copy link
Contributor

commented Oct 3, 2016

This PR is built on top of #828 and implements the idea in http://caml.inria.fr/mantis/view.php?id=6826, namely that type-checking an "open" statement should take constant time, independently of the size of the opened module.

This is done by keeping a layered representation for the environment: a set of local bindings, and possibly a symbolic representation of an open (just its path and its computed set of components, which is memoized) + the bindings before that open. Each lookup traverses this linked list of partial environments. This might be a bit slower, but of course, each map is smaller. For instance, previously, a lookup in an environment obtained by opening two modules with 1000 components would take O(log(2000)), while now it takes O(2*log(1000)).

Moreover, the new representation allows a lighter representation, since e.g. support for warnings on open don't pollute the representation for local bindings. In addition to that, find_same lookups don't need to go through parts representing components imported by an open statement, so they might actually be faster than in the current implementation.

All in all, I could not find a case where the new strategy slows down lookup enough to make the entire type-checking slower.

@alainfrisch alainfrisch force-pushed the alainfrisch:open_revisited branch from 30d4a4d to ef0d736 Oct 3, 2016

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2016

Timings for ocamlc.opt, compiling:

module A = struct
  let x1 = 1
  ...
  let xn = n
end
let x = A.(x1)
...
let x = A.(xn)

Results:

n trunk branch
1000 0.8s 0.06s
2000 4.0s 0.09s
3000 10s 0.14s
4000 21s 0.2s
@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2016

And a worst-case scenario, with many lookups that need to traverse a lot of "open" (here, 40):

module A = struct end
let x = 42
open A  (* 1 *)
....
open A  (* 40 *)
let y1 = x
...
let yn = x

Timings (seconds):

n trunk branch
10000 0.23 0.26
20000 0.49 0.53
40000 1.0 1.14

With only 10 opens:

n trunk branch
10000 0.23 0.23
20000 0.47 0.48
40000 0.98 1.03
@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2016

Code typical of let-open-in intensive style:

let r1 = Pervasives.(ref, succ)
....
let rn = Pervasives.(ref, succ)
n trunk branch
10000 2.8 0.6
20000 6.7 1.2
@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2016

An even worst-case scenario:

let x = 42
open List (* 1 *)
....
open List  (* n *)
let y1 = x
...
let y40000 = x

Here one needs to look for x in all the Tbl.t representing each open. Timings:

n trunk branch
10 1.00 1.03
40 1.00 1.21
80 1.03 1.40

I've tried to memoize the lookup results in the environment tables (so that further lookups on the same name don't need to traverse all the chain), and this reduces the overhead by about 1/2. Considering how small the overhead is for typical number of nested opens, I don't believe this is worth the extra complexity.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2016

I don't understand the problem with the testsuite (tests/typing-poly/poly.ml). This is an "expect" test. The current source is:

module M
: sig val f : (<m : 'b. 'b * ('b * <m:'c. 'c * 'bar> as 'bar)>) -> unit end
= struct let f (x : <m : 'a. 'a * ('a * 'foo)> as 'foo) = () end;;
module M
: sig type t = <m : 'b. 'b * ('b * <m:'c. 'c * 'bar> as 'bar)> end
= struct type t = <m : 'a. 'a * ('a * 'foo)> as 'foo end;;
[%%expect {|
Line _, characters 2-64:
Error: Signature mismatch:
       Modules do not match:
         sig val f : (< m : 'a. 'a * ('a * 'b) > as 'b) -> unit end
       is not included in
         sig
           val f : < m : 'b. 'b * ('b * < m : 'c. 'c * 'a > as 'a) > -> unit
         end
       Values do not match:
         val f : (< m : 'a. 'a * ('a * 'b) > as 'b) -> unit
       is not included in
         val f : < m : 'b. 'b * ('b * < m : 'c. 'c * 'a > as 'a) > -> unit
|}];;

and the correct version is:

module M
: sig val f : (<m : 'b. 'b * ('b * <m:'c. 'c * 'bar> as 'bar)>) -> unit end
= struct let f (x : <m : 'a. 'a * ('a * 'foo)> as 'foo) = () end;;
module M
: sig type t = <m : 'b. 'b * ('b * <m:'c. 'c * 'bar> as 'bar)> end
= struct type t = <m : 'a. 'a * ('a * 'foo)> as 'foo end;;
[%%expect {|
Line _, characters 2-64:
Error: Signature mismatch:
       ...
       Values do not match:
         val f : (< m : 'a. 'a * ('a * 'b) > as 'b) -> unit
       is not included in
         val f : < m : 'b. 'b * ('b * < m : 'c. 'c * 'a > as 'a) > -> unit
|}, Principal{|
Line _, characters 2-64:
Error: Signature mismatch:
       Modules do not match:
         sig val f : (< m : 'a. 'a * ('a * 'b) > as 'b) -> unit end
       is not included in
         sig
           val f : < m : 'b. 'b * ('b * < m : 'c. 'c * 'a > as 'a) > -> unit
         end
       Values do not match:
         val f : (< m : 'a. 'a * ('a * 'b) > as 'b) -> unit
       is not included in
         val f : < m : 'b. 'b * ('b * < m : 'c. 'c * 'a > as 'a) > -> unit
|}];;

The trouble is that I cannot reproduce the new "bad" behavior (showing "..." instead of "Module do not match") manually outside the expect tool, even in the toplevel. @diml : do you see how to investigate that?

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2016

Argh, the choice to print "..." in Includemod.report_error is based on the size of the marshaling of the internal representation of the object to be printed (here, a Module_type of module_type * module_type constructor) -- and whether marshaling succeeds or not. I don't see immediately why this would be impacted by the current PR, but this seems rather fragile.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2016

The size in the example is 501 bytes (with a limit at 500). This is probably related to a change of sharing (perhaps related to the fact that some Path.t, including simple Pident cases, are now recomputed on each access instead of being stored in the Env.t). I'm tempted to simply validate the test, since the current failure is more the sign of a weakness of the printer heuristics.

@diml

This comment has been minimized.

Copy link
Member

commented Oct 6, 2016

For the test you should be able to raise the limit by writing this in poly.ml:

Clflags.error_size := 1000;;

(I just discovered this variable)

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2016

I was actually considering setting it to 0 (no limit), since any value can make the test quite fragile. This seems to work fine for the current tests.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2016

So I've set error_size to 0 directly in expect_test.ml (and no test had to be adjusted). The rationale is that any arbitrary limit is likely to create irrelevant failures in the testsuite at some point (or be useless if it is set too high), considering how fragile the criterion is (which might be ok for interactive use, not for non-regression testing).

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2016

Does anyone want to review this?

@xavierleroy : you told me that you were concerned with degrading performances of lookups. How do the benchmarks above look to you?

@let-def

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

I started reviewing. Overall it seems fine.
I made a few esthetic comments, I didn't find anything wrong while reading the implementation.

As for the performance profile, it seems to better reflect the actual use of open (a few of them in a given branch, sometime used for very short-time). (Except maybe a code generator that outputs a huge number opening of small modules?)

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2016

Great, thanke @let-def .

I made a few esthetic comments

I cannot see the comments. If you started a review, you need to "submit" to make comments visible.

end


module EnvTbl2 =

This comment has been minimized.

Copy link
@let-def

let-def Oct 31, 2016

Contributor

Maybe use more descriptive names than EnvTbl & EnvTbl2?

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Oct 31, 2016

Author Contributor

Do you have a suggestion?

This comment has been minimized.

Copy link
@let-def

let-def Nov 1, 2016

Contributor

Good question... The distinction is between components that have physical representation and those which are "floating"?

Something like LabelTable & RootedTable?
I don't know of a general term for distinguishing those two categories in the compiler, though it would be relevant.

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Nov 1, 2016

Author Contributor

One is for labels and constructors, for which several definitions can exist in the same module (and all need to be retrieved by name).

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Nov 2, 2016

Author Contributor

Ok, proposing IdTbl for components with a physical representation (i.e. identified by an id) and TycompTbl for "components of types", i.e. labels and constructors. I'll happily rename if better names are proposed (this is purely internal to env.ml, so trivial to rename).

@@ -281,29 +492,35 @@ let is_in_signature env = env.flags land in_signature_flag <> 0
let is_implicit_coercion env = env.flags land implicit_coercion_flag <> 0

let diff_keys is_local tbl1 tbl2 =

This comment has been minimized.

Copy link
@let-def

let-def Oct 31, 2016

Contributor

diff_keys(2) could be lifted inside the EnvTbl(2) module.
Also, the only purpose of EnvTbl.local_keys is to implement diff_keys.

So all could be replaced by just an export of EnvTbl.diff_keys & EnvTbl2.diff_keys.

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Oct 31, 2016

Author Contributor

Done, thanks!

env
with Not_found ->
env
(* update summary?? *)

This comment has been minimized.

Copy link
@let-def

let-def Oct 31, 2016

Contributor

I think summary should be updated too. The updated environment appears in a Typedtree, so if inspected from a CMT the reconstructed environment would be wrong.
(I cannot think of an existing codepath in Merlin that can fail because of that, but summary affects cmt processing and its implementation of short-paths)

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Oct 31, 2016

Author Contributor

I've pushed a partial fix to that. It's not clear to me what to do when changing the value_description of an identifier imported from an open statement. In the old version, the id of these values was "hidden" (lookup can only be done by name); now there is no dummy id for these values so I don't see how to record the information in the summary (except by introducing a new Env_value_by_name of summary * string * value_description). But I'm not even sure that rewriting the type of such values is required... Do you know? @garrigue?

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Nov 2, 2016

Author Contributor

Ok, I've pushed a more complete fix: now the "copy_types" operation is exposed by Env and represented as such in the "summary".

After discussing with @garrigue, it seems the current situation is not ideal anyway, since the type of all values should be copied in non-principal mode, including those accessed with a module qualifier (which is not the case now, yielding to a different behavior when a value is accessed through an "open" or not). Cleaning that would be nice but is quite independent of this PR, so I prefer to keep the current behavior untouched, and it seems cleaner to make the "copy" operation supported directly by Env.

@let-def

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

@alainfrisch thanks, I never used that :)

@alainfrisch alainfrisch force-pushed the alainfrisch:open_revisited branch from eabc2e6 to afe0e41 Oct 31, 2016

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2016

I really think this is worth merging, as it enables a style with many small local opens (which are arguably less risky than global ones) without degrading much the style with global opens; moreover, it makes the support for open-related warnings arguably simpler. @let-def is happy with the current implementation. Does someone else want to review?

@gasche

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

(cc @garrigue who may be interested in reviewing and may or may not have been notified about this PR.)

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Nov 17, 2016

@alainfrisch you should remove the "work-in-progress" label if you think this is ready for merge.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2016

you should remove the "work-in-progress" label if you think this is ready for merge.

Yes, indeed. Thanks!

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2016

@garrigue Are you indeed interested in reviewing this?

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2016

Sorry. I read the diff, and answered @alainfrisch personally, but did not write here.

I understand the goal, but I am not convinced by the implementation.
My main concern is that env.ml is already fairly complex, and this adds extra complexity.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Jan 1, 2017

My main concern is that env.ml is already fairly complex, and this adds extra complexity.

I really believe that the implementation makes the treatment of open-related warnings much simpler to understand, and removes a weird notion of "signature substitution". It also opens the door to removing some of the current caching logic. Moreover, the "extra complexity" is tiny compared to other parts of the compiler (cf recent huge changes for flambda or spacetime); the new data structure is relatively simple and documented. This extra complexity seems well deserved to me considering the performance gains.

@garrigue Do you see a simpler way to achieve the same effect?

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2017

Not simpler. I was just thinking about how to avoid the overhead with lots of open's.
If you say that open is free, people could start using hundreds of them...

One solution would be to amortize the cost by copying each definition to the current environment after accessing it. I.e. some kind of lazy copying.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2017

people could start using hundreds of them...

I'm a bit puzzled with the argument that the PR makes "open" faster, and could thus encourage people to use it too much, and so we should make "open" even faster. This kind of arguments could be used against most kinds of optimizations. Honestly, if we make open faster, I don't think that people will realistically start nesting hundreds of open; this would be terrible for code readibility anyway. What they could do is to use a lot of local opens instead of a few global ones, which is, in the eyes of some at least, an improvement.

Not simpler.

In the previous note, you wrote "My main concern is that env.ml is already fairly complex, and this adds extra complexity." Are you now ok with the complexity of the suggested new implementation?

One solution would be to amortize the cost by copying each definition to the current environment after accessing it. I.e. some kind of lazy copying

I've tried it. A naive implementation slows down the common case (few nested opens) quite a bit, since it complexifies the data structure and yield some extra mutations. Moreover, treatment of the open warnings is made more complex.

More complex strategies could certainly be considered to get better worst-case complexity without degrading performances in most cases. Can we keep that for later, if the need arise?

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

@alainfrisch My point is that we should be careful that using open does not slow down the subsequent typechecking. However your tests seems to show the contrary, so maybe it's fine.
Can we discuss that at the developer meeting?

Also, my feeling is that at some point we will need a big clean up of env.ml, since we have been piling optimization over optimization for about 20 years...

@alainfrisch alainfrisch force-pushed the alainfrisch:open_revisited branch from a6803b3 to f91f561 Mar 24, 2017

@alainfrisch alainfrisch merged commit e692b5e into ocaml:trunk Mar 24, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.