diff --git a/CHANGES.md b/CHANGES.md index 2fe5bd066e..520052d906 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -5,6 +5,12 @@ - **irmin** - Add `Conf.pp` and `Conf.equal` to print and compare configuration values (#2227, @samoht) + - Add a `clear` optional arguments to all function that adds a new commit: + `Commit.v`, `set`, `set_tree`, `remove`, `test_and_set`, + `test_and_set_tree`, `test_set_and_get`, `test_set_and_get_tree`, `merge`, + `merge_tree` and `with_tree`. This new argument allows to control whether + the tree caches are cleared up after objects are exported to disk during + the commit. (#2225, @samoht) - **irmin-pack** - Add configuration option, `lower_root`, to specify a path for archiving data diff --git a/src/irmin/store.ml b/src/irmin/store.ml index 197b4438ac..d2c5002214 100644 --- a/src/irmin/store.ml +++ b/src/irmin/store.ml @@ -166,11 +166,11 @@ module Make (B : Backend.S) = struct |+ field "value" B.Commit.Val.t (fun t -> t.v) |> sealr - let v r ~info ~parents tree = + let v ?(clear = true) r ~info ~parents tree = B.Repo.batch r @@ fun contents_t node_t commit_t -> let* node = match Tree.destruct tree with - | `Node t -> Tree.export r contents_t node_t t + | `Node t -> Tree.export ~clear r contents_t node_t t | `Contents _ -> Lwt.fail_invalid_arg "cannot add contents at the root" in let v = B.Commit.Val.v ~info ~node ~parents in @@ -787,7 +787,8 @@ module Make (B : Backend.S) = struct (* Update the store with a new commit. Ensure the no commit becomes orphan in the process. *) - let update ?(allow_empty = false) ~info ?parents t key merge_tree f = + let update ?(clear = true) ?(allow_empty = false) ~info ?parents t key + merge_tree f = let* s = snapshot t key in (* this might take a very long time *) let* new_tree = f s.tree in @@ -801,7 +802,7 @@ module Make (B : Backend.S) = struct let info = info () in let parents = match parents with None -> s.parents | Some p -> p in let parents = List.map Commit.key parents in - let* c = Commit.v (repo t) ~info ~parents root in + let* c = Commit.v ~clear (repo t) ~info ~parents root in let* r = add_commit t s.head (c, root_tree (Tree.destruct root)) in Lwt.return (Ok (Some c, r)) @@ -820,34 +821,35 @@ module Make (B : Backend.S) = struct (c : (commit option, [> `Too_many_retries of int ]) result Lwt.t) = Lwt_result.map (fun _ -> ()) c - let set_tree ?(retries = 13) ?allow_empty ?parents ~info t k v = + let set_tree ?clear ?(retries = 13) ?allow_empty ?parents ~info t k v = [%log.debug "set %a" pp_path k]; ignore_commit @@ retry ~retries @@ fun () -> - update t k ?allow_empty ?parents ~info set_tree_once @@ fun _tree -> + update t k ?clear ?allow_empty ?parents ~info set_tree_once @@ fun _tree -> Lwt.return_some v - let set_tree_exn ?retries ?allow_empty ?parents ~info t k v = - set_tree ?retries ?allow_empty ?parents ~info t k v >>= fail "set_exn" + let set_tree_exn ?clear ?retries ?allow_empty ?parents ~info t k v = + set_tree ?clear ?retries ?allow_empty ?parents ~info t k v + >>= fail "set_exn" - let remove ?(retries = 13) ?allow_empty ?parents ~info t k = + let remove ?clear ?(retries = 13) ?allow_empty ?parents ~info t k = [%log.debug "debug %a" pp_path k]; ignore_commit @@ retry ~retries @@ fun () -> - update t k ?allow_empty ?parents ~info set_tree_once @@ fun _tree -> + update t k ?clear ?allow_empty ?parents ~info set_tree_once @@ fun _tree -> Lwt.return_none - let remove_exn ?retries ?allow_empty ?parents ~info t k = - remove ?retries ?allow_empty ?parents ~info t k >>= fail "remove_exn" + let remove_exn ?clear ?retries ?allow_empty ?parents ~info t k = + remove ?clear ?retries ?allow_empty ?parents ~info t k >>= fail "remove_exn" - let set ?retries ?allow_empty ?parents ~info t k v = + let set ?clear ?retries ?allow_empty ?parents ~info t k v = let v = Tree.of_contents v in - set_tree t k ?retries ?allow_empty ?parents ~info v + set_tree t k ?clear ?retries ?allow_empty ?parents ~info v - let set_exn ?retries ?allow_empty ?parents ~info t k v = - set t k ?retries ?allow_empty ?parents ~info v >>= fail "set_exn" + let set_exn ?clear ?retries ?allow_empty ?parents ~info t k v = + set t k ?clear ?retries ?allow_empty ?parents ~info v >>= fail "set_exn" let test_and_set_tree_once ~test root key ~current_tree ~new_tree = match (test, current_tree) with @@ -857,44 +859,51 @@ module Make (B : Backend.S) = struct if Tree.equal test v then set_tree_once root key ~new_tree ~current_tree else err_test current_tree - let test_set_and_get_tree ?(retries = 13) ?allow_empty ?parents ~info t k - ~test ~set = + let test_set_and_get_tree ?clear ?(retries = 13) ?allow_empty ?parents ~info t + k ~test ~set = [%log.debug "test-and-set %a" pp_path k]; retry ~retries @@ fun () -> - update t k ?allow_empty ?parents ~info (test_and_set_tree_once ~test) + update t k ?clear ?allow_empty ?parents ~info (test_and_set_tree_once ~test) @@ fun _tree -> Lwt.return set - let test_set_and_get_tree_exn ?retries ?allow_empty ?parents ~info t k ~test - ~set = - test_set_and_get_tree ?retries ?allow_empty ?parents ~info t k ~test ~set + let test_set_and_get_tree_exn ?clear ?retries ?allow_empty ?parents ~info t k + ~test ~set = + test_set_and_get_tree ?clear ?retries ?allow_empty ?parents ~info t k ~test + ~set >>= fail "test_set_and_get_tree_exn" - let test_set_and_get ?retries ?allow_empty ?parents ~info t k ~test ~set = + let test_set_and_get ?clear ?retries ?allow_empty ?parents ~info t k ~test + ~set = let test = Option.map Tree.of_contents test in let set = Option.map Tree.of_contents set in - test_set_and_get_tree ?retries ?allow_empty ?parents ~info t k ~test ~set + test_set_and_get_tree ?clear ?retries ?allow_empty ?parents ~info t k ~test + ~set - let test_set_and_get_exn ?retries ?allow_empty ?parents ~info t k ~test ~set = - test_set_and_get ?retries ?allow_empty ?parents ~info t k ~test ~set + let test_set_and_get_exn ?clear ?retries ?allow_empty ?parents ~info t k ~test + ~set = + test_set_and_get ?clear ?retries ?allow_empty ?parents ~info t k ~test ~set >>= fail "test_set_and_get_exn" - let test_and_set_tree ?(retries = 13) ?allow_empty ?parents ~info t k ~test - ~set = + let test_and_set_tree ?clear ?(retries = 13) ?allow_empty ?parents ~info t k + ~test ~set = [%log.debug "test-and-set %a" pp_path k]; ignore_commit - @@ test_set_and_get_tree ~retries ?allow_empty ?parents ~info t k ~test ~set + @@ test_set_and_get_tree ~retries ?clear ?allow_empty ?parents ~info t k + ~test ~set - let test_and_set_tree_exn ?retries ?allow_empty ?parents ~info t k ~test ~set - = - test_and_set_tree ?retries ?allow_empty ?parents ~info t k ~test ~set + let test_and_set_tree_exn ?clear ?retries ?allow_empty ?parents ~info t k + ~test ~set = + test_and_set_tree ?clear ?retries ?allow_empty ?parents ~info t k ~test ~set >>= fail "test_and_set_tree_exn" - let test_and_set ?retries ?allow_empty ?parents ~info t k ~test ~set = + let test_and_set ?clear ?retries ?allow_empty ?parents ~info t k ~test ~set = ignore_commit - @@ test_set_and_get ?retries ?allow_empty ?parents ~info t k ~test ~set + @@ test_set_and_get ?clear ?retries ?allow_empty ?parents ~info t k ~test + ~set - let test_and_set_exn ?retries ?allow_empty ?parents ~info t k ~test ~set = - test_and_set ?retries ?allow_empty ?parents ~info t k ~test ~set + let test_and_set_exn ?clear ?retries ?allow_empty ?parents ~info t k ~test + ~set = + test_and_set ?clear ?retries ?allow_empty ?parents ~info t k ~test ~set >>= fail "test_and_set_exn" let merge_once ~old root key ~current_tree ~new_tree = @@ -903,25 +912,27 @@ module Make (B : Backend.S) = struct | Ok tr -> set_tree_once root key ~new_tree:tr ~current_tree | Error e -> write_error (e :> write_error) - let merge_tree ?(retries = 13) ?allow_empty ?parents ~info ~old t k tree = + let merge_tree ?clear ?(retries = 13) ?allow_empty ?parents ~info ~old t k + tree = [%log.debug "merge %a" pp_path k]; ignore_commit @@ retry ~retries @@ fun () -> - update t k ?allow_empty ?parents ~info (merge_once ~old) @@ fun _tree -> - Lwt.return tree + update t k ?clear ?allow_empty ?parents ~info (merge_once ~old) + @@ fun _tree -> Lwt.return tree - let merge_tree_exn ?retries ?allow_empty ?parents ~info ~old t k tree = - merge_tree ?retries ?allow_empty ?parents ~info ~old t k tree + let merge_tree_exn ?clear ?retries ?allow_empty ?parents ~info ~old t k tree = + merge_tree ?clear ?retries ?allow_empty ?parents ~info ~old t k tree >>= fail "merge_tree_exn" - let merge ?retries ?allow_empty ?parents ~info ~old t k v = + let merge ?clear ?retries ?allow_empty ?parents ~info ~old t k v = let old = Option.map Tree.of_contents old in let v = Option.map Tree.of_contents v in - merge_tree ?retries ?allow_empty ?parents ~info ~old t k v + merge_tree ?clear ?retries ?allow_empty ?parents ~info ~old t k v - let merge_exn ?retries ?allow_empty ?parents ~info ~old t k v = - merge ?retries ?allow_empty ?parents ~info ~old t k v >>= fail "merge_exn" + let merge_exn ?clear ?retries ?allow_empty ?parents ~info ~old t k v = + merge ?clear ?retries ?allow_empty ?parents ~info ~old t k v + >>= fail "merge_exn" let mem t k = tree t >>= fun tree -> Tree.mem tree k let mem_tree t k = tree t >>= fun tree -> Tree.mem_tree tree k @@ -949,7 +960,7 @@ module Make (B : Backend.S) = struct let list t k = tree t >>= fun tree -> Tree.list tree k let kind t k = tree t >>= fun tree -> Tree.kind tree k - let with_tree ?(retries = 13) ?allow_empty ?parents + let with_tree ?clear ?(retries = 13) ?allow_empty ?parents ?(strategy = `Test_and_set) ~info t key f = let done_once = ref false in let rec aux n old_tree = @@ -959,10 +970,10 @@ module Make (B : Backend.S) = struct let* new_tree = f old_tree in match (strategy, new_tree) with | `Set, Some tree -> - set_tree t key ~retries ?allow_empty ?parents tree ~info - | `Set, None -> remove t key ~retries ?allow_empty ~info ?parents + set_tree ?clear t key ~retries ?allow_empty ?parents tree ~info + | `Set, None -> remove ?clear t key ~retries ?allow_empty ~info ?parents | `Test_and_set, _ -> ( - test_and_set_tree t key ~retries ?allow_empty ?parents ~info + test_and_set_tree ?clear t key ~retries ?allow_empty ?parents ~info ~test:old_tree ~set:new_tree >>= function | Error (`Test_was tr) when retries > 0 && n <= retries -> @@ -970,8 +981,8 @@ module Make (B : Backend.S) = struct aux (n + 1) tr | e -> Lwt.return e) | `Merge, _ -> ( - merge_tree ~old:old_tree ~retries ?allow_empty ?parents ~info t key - new_tree + merge_tree ?clear ~old:old_tree ~retries ?allow_empty ?parents ~info + t key new_tree >>= function | Ok _ as x -> Lwt.return x | Error (`Conflict _) when retries > 0 && n <= retries -> @@ -989,8 +1000,9 @@ module Make (B : Backend.S) = struct let* old_tree = find_tree t key in aux 0 old_tree - let with_tree_exn ?retries ?allow_empty ?parents ?strategy ~info f t key = - with_tree ?retries ?allow_empty ?strategy ?parents ~info f t key + let with_tree_exn ?clear ?retries ?allow_empty ?parents ?strategy ~info f t + key = + with_tree ?clear ?retries ?allow_empty ?strategy ?parents ~info f t key >>= fail "with_tree_exn" let clone ~src ~dst = diff --git a/src/irmin/store_intf.ml b/src/irmin/store_intf.ml index 27783a454d..bc6194ba54 100644 --- a/src/irmin/store_intf.ml +++ b/src/irmin/store_intf.ml @@ -342,12 +342,21 @@ module type S_generic_key = sig val pp_hash : t Fmt.t (** [pp] is the pretty-printer for commit. Display only the hash. *) - val v : repo -> info:info -> parents:commit_key list -> tree -> commit Lwt.t + val v : + ?clear:bool -> + repo -> + info:info -> + parents:commit_key list -> + tree -> + commit Lwt.t (** [v r i ~parents:p t] is the commit [c] such that: - [info c = i] - [parents c = p] - - [tree c = t] *) + - [tree c = t] + + When [clear] is set (the default), the tree cache is emptied upon the + function's completion, mirroring the effect of invoking {!Tree.clear}. *) val tree : commit -> tree (** [tree c] is [c]'s root tree. *) @@ -602,6 +611,7 @@ module type S_generic_key = sig instead of the one we were waiting for. *) val set : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -616,11 +626,15 @@ module type S_generic_key = sig This function always uses {!Metadata.default} as metadata. Use {!set_tree} with `[Contents (c, m)] for different ones. + When [clear] is set (the default), the tree cache is emptied upon the + function's completion, mirroring the effect of invoking {!Tree.clear}. + The result is [Error `Too_many_retries] if the concurrent operations do not allow the operation to commit to the underlying storage layer (livelock). *) val set_exn : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -633,6 +647,7 @@ module type S_generic_key = sig type. *) val set_tree : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -644,6 +659,7 @@ module type S_generic_key = sig (** [set_tree] is like {!set} but for trees. *) val set_tree_exn : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -655,6 +671,7 @@ module type S_generic_key = sig (** [set_tree] is like {!set_exn} but for trees. *) val remove : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -669,6 +686,7 @@ module type S_generic_key = sig (livelock). *) val remove_exn : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -680,6 +698,7 @@ module type S_generic_key = sig result type. *) val test_and_set : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -703,6 +722,7 @@ module type S_generic_key = sig (livelock). *) val test_and_set_exn : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -716,6 +736,7 @@ module type S_generic_key = sig of using a result type. *) val test_and_set_tree : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -728,6 +749,7 @@ module type S_generic_key = sig (** [test_and_set_tree] is like {!test_and_set} but for trees. *) val test_and_set_tree_exn : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -740,6 +762,7 @@ module type S_generic_key = sig (** [test_and_set_tree_exn] is like {!test_and_set_exn} but for trees. *) val test_set_and_get : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -755,6 +778,7 @@ module type S_generic_key = sig to the store. *) val test_set_and_get_exn : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -768,6 +792,7 @@ module type S_generic_key = sig instead. *) val test_set_and_get_tree : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -780,6 +805,7 @@ module type S_generic_key = sig (** [test_set_and_get_tree] is like {!test_set_and_get} but for a {!tree} *) val test_set_and_get_tree_exn : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -793,6 +819,7 @@ module type S_generic_key = sig [Failure _] instead. *) val merge : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -816,6 +843,7 @@ module type S_generic_key = sig (livelock). *) val merge_exn : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -829,6 +857,7 @@ module type S_generic_key = sig result type. *) val merge_tree : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -841,6 +870,7 @@ module type S_generic_key = sig (** [merge_tree] is like {!merge_tree} but for trees. *) val merge_tree_exn : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -853,6 +883,7 @@ module type S_generic_key = sig (** [merge_tree] is like {!merge_tree} but for trees. *) val with_tree : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -887,6 +918,7 @@ module type S_generic_key = sig write conflicts are visible on commit. *) val with_tree_exn : + ?clear:bool -> ?retries:int -> ?allow_empty:bool -> ?parents:commit list -> @@ -1090,8 +1122,10 @@ module type S_generic_key = sig [> read_write ] Backend.Node.t -> tree -> kinded_key Lwt.t - (** Save a tree into the database. Does not do any reads. If [clear] is set - (it is by default), the tree cache will be cleared after the save. *) + (** Save a tree into the database. Does not do any reads. + + When [clear] is set (the default), the tree cache is emptied upon the + function's completion, mirroring the effect of invoking {!Tree.clear}. *) (** {Deprecated} *) diff --git a/src/irmin/tree.ml b/src/irmin/tree.ml index cadc385582..3906ba14c8 100644 --- a/src/irmin/tree.ml +++ b/src/irmin/tree.ml @@ -1965,11 +1965,15 @@ module Make (P : Backend.S) = struct let env = Env.empty () in import_with_env ~env repo f + let same_repo r1 r2 = + r1 == r2 || Conf.equal (P.Repo.config r1) (P.Repo.config r2) + (* Given an arbitrary tree value, persist its contents to the given contents and node stores via a depth-first {i post-order} traversal. We must export a node's children before the node itself in order to get the {i keys} of any un-persisted child values. *) let export ?clear repo contents_t node_t n = + [%log.debug "Tree.export clear=%a" Fmt.(option bool) clear]; let cache = match clear with | Some true | None -> @@ -2070,7 +2074,7 @@ module Make (P : Backend.S) = struct let has_repo = match n.Node.v with | Node.Key (repo', _) -> - if repo == repo' then true + if same_repo repo repo' then true else (* Case 1. [n] is a key from another repo. Let's crash. @@ -2078,7 +2082,7 @@ module Make (P : Backend.S) = struct [repo], or completely ignore the issue. *) failwith "Can't export the node key from another repo" | Value (repo', _, _) -> - if repo == repo' then true + if same_repo repo repo' then true else (* Case 2. [n] is a value from another repo. Let's crash. diff --git a/test/irmin/test_tree.ml b/test/irmin/test_tree.ml index ed4d3ee42b..f11403a17d 100644 --- a/test/irmin/test_tree.ml +++ b/test/irmin/test_tree.ml @@ -462,10 +462,10 @@ let test_update _ () = let lazy_stats = Tree.{ nodes = 0; leafs = 0; skips = 1; depth = 0; width = 0 } (* Take a tree and persist it to some underlying store, making it lazy. *) -let persist_tree : Store.tree -> Store.tree Lwt.t = +let persist_tree ?clear : Store.tree -> Store.tree Lwt.t = fun tree -> let* store = Store.Repo.v (Irmin_mem.config ()) >>= Store.empty in - let* () = Store.set_tree_exn ~info:Store.Info.none store [] tree in + let* () = Store.set_tree_exn ?clear ~info:Store.Info.none store [] tree in Store.tree store type path = Store.Path.t [@@deriving irmin ~pp ~equal] @@ -519,6 +519,29 @@ let test_clear _ () = in Lwt.return_unit +let test_minimal_reads _ () = + (* 1. Build a tree *) + let size = 10 in + let* t = + List.init size string_of_int + |> Lwt_list.fold_left_s (fun acc i -> Tree.add acc [ i ] i) (Tree.empty ()) + in + + (* Persist with no clear *) + Tree.reset_counters (); + let* _ = persist_tree ~clear:false t in + let* _ = Tree.find t [ "0" ] in + let cnt = Tree.counters () in + Alcotest.(check int) "no reads" 0 cnt.node_find; + + (* Persist with clear *) + Tree.reset_counters (); + let* _ = persist_tree ~clear:true t in + let* _ = Tree.find_tree t [ "0" ] in + let cnt = Tree.counters () in + Alcotest.(check int) "reads" 1 cnt.node_find; + Lwt.return_unit + let with_binding k v t = Tree.add_tree t k v let clear_and_assert_lazy tree = @@ -846,6 +869,7 @@ let suite = Alcotest_lwt.test_case "remove" `Quick test_remove; Alcotest_lwt.test_case "update" `Quick test_update; Alcotest_lwt.test_case "clear" `Quick test_clear; + Alcotest_lwt.test_case "minimal_reads" `Quick test_minimal_reads; Alcotest_lwt.test_case "fold" `Quick test_fold_force; Alcotest_lwt.test_case "Broken.hashes" `Quick Broken.test_hashes; Alcotest_lwt.test_case "Broken.trees" `Quick Broken.test_trees;