Skip to content

Commit 263e7ac

Browse files
committed
Fix exponential short-paths issue
1 parent c1cb3d6 commit 263e7ac

File tree

1 file changed

+54
-13
lines changed

1 file changed

+54
-13
lines changed

src/ocaml/typing/short_paths_graph.ml

Lines changed: 54 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -663,6 +663,36 @@ end = struct
663663

664664
end
665665

666+
and Find_module_cache : sig
667+
(** Without [Find_module_cache.t], [Graph.find_module] would be exponential. In the
668+
[Pdot] and [Papply] cases, [Graph.find_module] makes a recursive call to
669+
[Graph.find_module] followed by a call to [Module.find_module]. [Module.find_module]
670+
then also makes a (mutually) recursive call to [Graph.find_module], often with the
671+
exact same input as the first [Graph.find_module] recursive call. This leads to
672+
exponential blow up if we don't re-use the output of the first [Graph.find_module]
673+
recursive call. [Find_module_cache.t] is used to thread through the result of that
674+
first call.
675+
676+
It's unclear to me (liam923) at the time of writing this whether storing
677+
[path_of_last_found_module] is necessary as I am unsure whether the second call to
678+
[Graph.find_module] will always pass the exact same path as the first call to
679+
[Graph.find_module]. But doing so is cheap and is the safe option.
680+
681+
One could imagine caching more than just the most recent call to [Graph.find_module]
682+
and instead caching every result or using some sort of LRU cache. But it's not clear
683+
this would actually result in any performance benefits, so this additional
684+
complexity doesn't seem worth it. *)
685+
type t = {
686+
path_of_last_found_module : Path.t;
687+
last_found_module : Module.t;
688+
}
689+
end = struct
690+
type t = {
691+
path_of_last_found_module : Path.t;
692+
last_found_module : Module.t;
693+
}
694+
end
695+
666696
and Module : sig
667697

668698
type t
@@ -703,11 +733,11 @@ and Module : sig
703733

704734
val find_module_type : Graph.t -> t -> string -> Module_type.t
705735

706-
val find_module : Graph.t -> t -> string -> Module.t
736+
val find_module : ?cache:Find_module_cache.t -> Graph.t -> t -> string -> Module.t
707737

708-
val find_application : Graph.t -> t -> Path.t -> Module.t
738+
val find_application : ?cache:Find_module_cache.t -> Graph.t -> t -> Path.t -> Module.t
709739

710-
val normalize : Graph.t -> t -> normalized
740+
val normalize : ?cache:Find_module_cache.t -> Graph.t -> t -> normalized
711741

712742
val unnormalize : normalized -> t
713743

@@ -853,11 +883,18 @@ end = struct
853883
| aliased -> normalize_loop root aliased
854884
end
855885

856-
let normalize root t =
886+
let normalize ?(cache : Find_module_cache.t option) root t =
857887
match t with
858888
| Definition { sort = Sort.Defined } -> normalize_loop root t
859889
| Definition { sort = Sort.Declared _ } | Declaration _ ->
860-
match Graph.find_module root (raw_path t) with
890+
let raw_path_of_t = raw_path t in
891+
let md =
892+
match cache with
893+
| Some { path_of_last_found_module; last_found_module }
894+
when Path.equal raw_path_of_t path_of_last_found_module -> last_found_module
895+
| _ -> Graph.find_module root raw_path_of_t
896+
in
897+
match md with
861898
| exception Not_found -> normalize_loop root t
862899
| t -> normalize_loop root t
863900

@@ -877,8 +914,8 @@ end = struct
877914
| Declaration _ -> Unknown
878915
| Definition { definition; _ } -> definition
879916

880-
let force root t =
881-
let t = Module.normalize root t in
917+
let force ?cache root t =
918+
let t = Module.normalize ?cache root t in
882919
match definition t with
883920
| Alias _ -> assert false
884921
| Unknown
@@ -993,8 +1030,8 @@ end = struct
9931030
| Signature { components = Forced { module_types; _ }; _ } ->
9941031
String_map.find name module_types
9951032

996-
let find_module root t name =
997-
let t = force root t in
1033+
let find_module ?cache root t name =
1034+
let t = force ?cache root t in
9981035
match definition t with
9991036
| Alias _
10001037
| Signature { components = Unforced _ } ->
@@ -1006,8 +1043,8 @@ end = struct
10061043
| Signature { components = Forced { modules; _ }; _ } ->
10071044
String_map.find name modules
10081045

1009-
let find_application root t path =
1010-
let t = Module.normalize root t in
1046+
let find_application ?cache root t path =
1047+
let t = Module.normalize ?cache root t in
10111048
match definition t with
10121049
| Alias _ -> assert false
10131050
| Signature _ -> raise Not_found
@@ -1389,10 +1426,14 @@ end = struct
13891426
Ident_map.find id t.modules
13901427
| Path.Pdot(p, name) ->
13911428
let md = find_module t p in
1392-
Module.find_module t md name
1429+
Module.find_module
1430+
~cache:{ last_found_module = md; path_of_last_found_module = p }
1431+
t md name
13931432
| Path.Papply(p, arg) ->
13941433
let md = find_module t p in
1395-
Module.find_application t md arg
1434+
Module.find_application
1435+
~cache:{ last_found_module = md; path_of_last_found_module = p }
1436+
t md arg
13961437
| Path.Pextra_ty _ ->
13971438
raise Not_found
13981439

0 commit comments

Comments
 (0)