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

Add Tree.is_val and Tree.Contents.is_val functions #1864

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

samoht
Copy link
Member

@samoht samoht commented Jun 7, 2022

These functions check whether the node/contents are available in
memory or not (ie. if calling a function on those could incur IO
costs).

@samoht
Copy link
Member Author

samoht commented Jun 7, 2022

Not ready to merge, I'm just opening this to start the discussion early.

These functions check whether the node/contents are available in
memory or not (ie. if calling a function on those could incur IO
costs).
@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2022

Codecov Report

Merging #1864 (366fc45) into main (0e6b3de) will increase coverage by 0.01%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##             main    #1864      +/-   ##
==========================================
+ Coverage   64.25%   64.27%   +0.01%     
==========================================
  Files         118      118              
  Lines       14317    14339      +22     
==========================================
+ Hits         9199     9216      +17     
- Misses       5118     5123       +5     
Impacted Files Coverage Δ
src/irmin/tree.ml 80.66% <50.00%> (-0.36%) ⬇️
src/irmin-test/store.ml 94.84% <100.00%> (+0.01%) ⬆️
src/irmin/watch.ml 80.98% <0.00%> (+2.11%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@samoht
Copy link
Member Author

samoht commented Jun 7, 2022

Added a few tests, it's ready to reviewing now.

(** [is_val t k] is [true] iff the path [k] has already been forced in [t]. In
that case, that means that all the nodes traversed by [k] are loaded in
memory. If the leaf node is a contents [c], then [Contents.is_val c]
should also be [true]. *)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe the doc should also mention that it returns true also when there is no node at the path given

@@ -128,6 +134,10 @@ module type S = sig
(** Equivalent to {!val-force}, but raises an exception if the lazy content
value is not present in the underlying repository. *)

val is_val : t -> bool
(** [is_val x] is [true] iff [x] has already been forced (and so is loaded
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(** [is_val x] is [true] iff [x] has already been forced (and so is loaded
(** [is_val t] is [true] iff [t] has already been forced (and so is loaded

@@ -1417,6 +1417,37 @@ module Make (S : Generic_key) = struct
in
run x test

let test_lazy_tree x () =
let is_val_aux v t k =
let str = Fmt.str "empty is_val %a" Irmin.Type.(pp S.path_t) k in
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let str = Fmt.str "empty is_val %a" Irmin.Type.(pp S.path_t) k in
let str = Fmt.str "is_val %a" Irmin.Type.(pp S.path_t) k in

is_not_val v1 [];
is_not_val v1 [ "a" ];

let* _ = S.Tree.find_tree v1 [ "a" ] in
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let* _ = S.Tree.find_tree v1 [ "a" ] in
let* _ = S.Tree.find_tree v1 [ "a" ] in
is_val v1 [];
is_val v1 [ "a" ];
let* _ = S.Tree.find_tree v1 [ "b" ] in
is_val v1 [];
is_val v1 [ "b" ];

the returned value here is None, there is no node at "a" in r1 (see https://github.com/mirage/irmin/blob/main/src/irmin-test/common.ml#L197). I think you meant to test for an existing path in the tree.
However the test for "b" fails, I would have expected it to pass.


S.Tree.clear v1;
is_not_val v1 [];
is_not_val v1 [ "a" ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
is_not_val v1 [ "a" ];
is_not_val v1 [ "b" ];

S.Tree.clear v1;
is_not_val v1 [];
is_not_val v1 [ "a" ];

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to also add a test for an update in lazy tree, for instance:

      let* x = S.Tree.add v1 [ "c"; "d" ] "x" in
      is_val x [ "c"; "d" ];
      is_not_val v1 [ "b" ];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants