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.

Sign up for GitHub

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

Jump to bottom

Speed up Store.last-modified by comparing hashes instead of file contents #2335

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Speed up Store.last-modified by comparing hashes instead of file contents #2335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 5 additions & 5 deletions src/irmin/store.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1153,9 +1153,9 @@ module Make (B : Backend.S) = struct
let current, current_depth = Heap.pop_minimum heap in
let parents = Commit.parents current in
let tree = Commit.tree current in
let* current_value = Tree.find tree key in
let* current_tree = Tree.find_tree tree key in
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could also be optimized away since, except for the very first call, we already fetched the current_tree on the previous recursive call on line 1175 below :) (but we can do this in a follow-up PR if it's unclear how to)

if List.length parents = 0 then
if current_value <> None then Lwt.return (current :: acc)
if current_tree <> None then Lwt.return (current :: acc)
else Lwt.return acc
else
let max_depth =
Expand All @@ -1173,9 +1173,9 @@ module Make (B : Backend.S) = struct
Heap.add heap (commit, current_depth + 1)
in
let tree = Commit.tree commit in
let+ e = Tree.find tree key in
match (e, current_value) with
| Some x, Some y -> not (equal_contents x y)
let+ e = Tree.find_tree tree key in
match (e, current_tree) with
| Some x, Some y -> Tree.hash x <> Tree.hash y
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the generic <> function isn't guaranteed to work for some hash representations, maybe consider using an equal_hash function instead? (as done in other modules)

| Some _, None -> true
| None, Some _ -> true
| _, _ -> false)
Expand Down
7 changes: 3 additions & 4 deletions src/irmin/store_intf.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1006,10 +1006,9 @@ module type S_generic_key = sig
head is not set) and stopping at [min] if specified. *)

val last_modified : ?depth:int -> ?n:int -> t -> path -> commit list Lwt.t
(** [last_modified ?number c k] is the list of the last [number] commits that
modified [path], in ascending order of date. [depth] is the maximum depth
to be explored in the commit graph, if any. Default value for [number] is
1. *)
(** [last_modified ?n c k] is the list of the last [n] commits that modified
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
(** [last_modified ?n c k] is the list of the last [n] commits that modified
(** [last_modified ?depth ?n c k] is the list of the last [n] commits that modified

[path], in ascending order of date. [depth] is the maximum depth to be
explored in the commit graph, if any. Default value for [n] is 1. *)

(** Manipulate branches. *)
module Branch : sig
Expand Down