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

Memoize dir contents #1979

Merged
merged 3 commits into from
Mar 28, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
366 changes: 225 additions & 141 deletions src/dir_contents.ml

Large diffs are not rendered by default.

12 changes: 10 additions & 2 deletions src/dir_contents.mli
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,19 @@ val lookup_module : t -> Module.Name.t -> Dune_file.Buildable.t option
(** All mld files attached to this documentation stanza *)
val mlds : t -> Dune_file.Documentation.t -> Path.t list

val get : Super_context.t -> dir:Path.t -> t
type get_result =
| Standalone_or_root of t
| Group_part of Path.t

(** Produces rules for all group parts when it returns [Standalone_or_root].
Does not generate any rules when it returns [Group_part]. *)
val get : Super_context.t -> dir:Path.t -> get_result

val get_without_rules : Super_context.t -> dir:Path.t -> t

type kind = private
| Standalone
| Group_root of t list Lazy.t (** Sub-directories part of the group *)
| Group_root of (unit -> t list) (** Sub-directories part of the group *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that I dislike about Memo.lazy_ is that now we don't know if a function is memoized or not. I know memoization should be transparent, but for anonymous functions it's something you always have to think about it. Would it make sense to have a 'a Memo.Lazy.t type perhaps?

(This is a bit of a meta comment not to be addressed in this PR)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was weakly thinking that too. I'll make that change in a separate PR.

| Group_part of t

val kind : t -> kind
Expand Down
62 changes: 43 additions & 19 deletions src/dir_status.ml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ open Stdune
open Dune_file

module T = struct
type is_component_of_a_group_but_not_the_root = {
group_root : Path.t;
stanzas : Stanza.t list Dir_with_dune.t option;
}

type t =
| Standalone of
(File_tree.Dir.t * Stanza.t list Dir_with_dune.t option) option
Expand All @@ -13,17 +18,22 @@ module T = struct
* Stanza.t list Dir_with_dune.t
(* Directory with [(include_subdirs x)] where [x] is not [no] *)

| Is_component_of_a_group_but_not_the_root of
Stanza.t list Dir_with_dune.t option
| Is_component_of_a_group_but_not_the_root of is_component_of_a_group_but_not_the_root
(* Sub-directory of a [Group_root _] *)

let to_sexp _ = Sexp.Atom "<dir-status is opaque>"
end
include T

let is_standalone = function
| Standalone _ -> true
| _ -> false
type enclosing_group =
| No_group
| Group_root of Path.t

let current_group dir = function
| Standalone _ -> No_group
| Group_root _ -> Group_root dir
| Is_component_of_a_group_but_not_the_root { group_root; _ } ->
Group_root group_root

let get_include_subdirs stanzas =
List.fold_left stanzas ~init:None ~f:(fun acc stanza ->
Expand Down Expand Up @@ -59,18 +69,23 @@ module DB = struct

let get db ~dir =
let get ~dir = Memo.exec db.fn dir in
let enclosing_group ~dir =
match Path.parent dir with
| None ->
No_group
| Some parent_dir ->
current_group parent_dir (get ~dir:parent_dir)
in
match
Option.bind (Path.drop_build_context dir)
~f:(File_tree.find_dir db.file_tree)
with
| None -> begin
match Path.parent dir with
| None -> Standalone None
| Some dir ->
if is_standalone (get ~dir) then
Standalone None
else
Is_component_of_a_group_but_not_the_root None
match enclosing_group ~dir with
| No_group -> Standalone None
| Group_root group_root ->
Is_component_of_a_group_but_not_the_root
{ stanzas = None; group_root }
end
| Some ft_dir ->
let project_root =
Expand All @@ -79,23 +94,32 @@ module DB = struct
|> Path.of_local in
match stanzas_in db ~dir with
| None ->
if Path.equal dir project_root ||
is_standalone (get ~dir:(Path.parent_exn dir)) then
if Path.equal dir project_root then
Standalone (Some (ft_dir, None))
else
Is_component_of_a_group_but_not_the_root None
(match enclosing_group ~dir with
| No_group ->
Standalone (Some (ft_dir, None))
| Group_root group_root ->
Is_component_of_a_group_but_not_the_root
{ stanzas = None; group_root })
| Some d ->
match get_include_subdirs d.data with
| Some Unqualified ->
Group_root (ft_dir, d)
| Some No ->
Standalone (Some (ft_dir, Some d))
| None ->
if dir <> project_root &&
not (is_standalone (get ~dir:(Path.parent_exn dir)))
if dir <> project_root
then begin
check_no_module_consumer d.data;
Is_component_of_a_group_but_not_the_root (Some d)
match enclosing_group ~dir with
| Group_root group_root ->
(
check_no_module_consumer d.data;
Is_component_of_a_group_but_not_the_root
{ stanzas = (Some d); group_root })
| No_group ->
Standalone (Some (ft_dir, Some d))
end else
Standalone (Some (ft_dir, Some d))

Expand Down
8 changes: 6 additions & 2 deletions src/dir_status.mli
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
open Stdune

type is_component_of_a_group_but_not_the_root = {
group_root : Path.t;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that is slightly weird to me is that you're returning a build path here, but every other constructor is return a source path for the root (File_tree.Dir.t). Is there a way to keep things consistent, perhaps? This way we'd be able to use this record as the payload for everyone of the constructors.

Copy link
Collaborator Author

@aalekseyev aalekseyev Mar 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm returning the same kind of path as what the caller gave me, which is I think what counts here. (at least that's what's necessary at the call site where we just call the function again with that path)

we'd be able to use this record as the payload for everyone of the constructors

I'm not sure that's a good idea: I think File_tree.Dir.t gives you much more than just the path and if the call sites don't need it we probably shouldn't give it to them.

stanzas : Stanza.t list Dir_with_dune.t option;
}

type t =
| Standalone of
(File_tree.Dir.t * Stanza.t list Dir_with_dune.t option) option
Expand All @@ -11,8 +16,7 @@ type t =
* Stanza.t list Dir_with_dune.t
(* Directory with [(include_subdirs x)] where [x] is not [no] *)

| Is_component_of_a_group_but_not_the_root of
Stanza.t list Dir_with_dune.t option
| Is_component_of_a_group_but_not_the_root of is_component_of_a_group_but_not_the_root
(* Sub-directory of a [Group_root _] *)

module DB : sig
Expand Down
22 changes: 13 additions & 9 deletions src/gen_rules.ml
Original file line number Diff line number Diff line change
Expand Up @@ -240,16 +240,20 @@ module Gen(P : sig val sctx : Super_context.t end) = struct
| Some _ ->
(* This interprets "rule" and "copy_files" stanzas. *)
let dir_contents = Dir_contents.get sctx ~dir in
match Dir_contents.kind dir_contents with
| Standalone ->
ignore (gen_rules dir_contents [] ~dir : _ list)
match dir_contents with
| Group_part root ->
Build_system.load_dir ~dir:(Dir_contents.dir root)
| Group_root (lazy subs) ->
let cctxs = gen_rules dir_contents [] ~dir in
List.iter subs ~f:(fun dc ->
ignore (gen_rules dir_contents cctxs ~dir:(Dir_contents.dir dc)
: _ list))
Build_system.load_dir ~dir:root
| Standalone_or_root dir_contents ->
match Dir_contents.kind dir_contents with
| Group_part _ -> assert false
| Standalone ->
ignore (gen_rules dir_contents [] ~dir : _ list)
| Group_root subs ->
let cctxs = gen_rules dir_contents [] ~dir in
let subs = subs () in
List.iter subs ~f:(fun dc ->
ignore (gen_rules dir_contents cctxs ~dir:(Dir_contents.dir dc)
: _ list))
end);
match components with
| [] -> These (String.Set.of_list [".js"; "_doc"; ".ppx"])
Expand Down
4 changes: 2 additions & 2 deletions src/install_rules.ml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ let gen_dune_package sctx ~version ~(pkg : Local_package.t) =
|> List.map ~f:(fun lib ->
let name = Lib.name lib in
let dir_contents =
Dir_contents.get sctx ~dir:(Lib.src_dir lib) in
Dir_contents.get_without_rules sctx ~dir:(Lib.src_dir lib) in
let lib_modules =
Dir_contents.modules_of_library dir_contents ~name in
let foreign_objects =
Expand Down Expand Up @@ -380,7 +380,7 @@ let init_install sctx (package : Local_package.t) entries =
; dune_version = _
} ->
let sub_dir = (Option.value_exn lib.public).sub_dir in
let dir_contents = Dir_contents.get sctx ~dir in
let dir_contents = Dir_contents.get_without_rules sctx ~dir in
lib_install_files sctx ~dir ~sub_dir lib ~scope
~dir_kind ~dir_contents)
in
Expand Down
2 changes: 1 addition & 1 deletion src/odoc.ml
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ let setup_package_aliases sctx (pkg : Package.t) =
)

let entry_modules_by_lib sctx lib =
Dir_contents.get sctx ~dir:(Lib.src_dir lib)
Dir_contents.get_without_rules sctx ~dir:(Lib.src_dir lib)
|> Dir_contents.modules_of_library ~name:(Lib.name lib)
|> Lib_modules.entry_modules

Expand Down
2 changes: 1 addition & 1 deletion src/packages.ml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ let mlds_by_package_def =
|> List.concat_map ~f:(fun (w : _ Dir_with_dune.t) ->
List.filter_map w.data ~f:(function
| Documentation d ->
let dc = Dir_contents.get sctx ~dir:w.ctx_dir in
let dc = Dir_contents.get_without_rules sctx ~dir:w.ctx_dir in
let mlds = Dir_contents.mlds dc d in
Some (d.package.name, mlds)
| _ ->
Expand Down
2 changes: 1 addition & 1 deletion src/virtual_rules.ml
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ let impl sctx ~dir ~(lib : Dune_file.Library.t) ~scope ~modules =
| Local, Local ->
let name = Lib.name vlib in
let dir_contents =
Dir_contents.get sctx ~dir:(Lib.src_dir vlib) in
Dir_contents.get_without_rules sctx ~dir:(Lib.src_dir vlib) in
let modules =
let pp_spec =
Pp_spec.make lib.buildable.preprocess
Expand Down