Re: [PATCH 1/5] odb/source-files: fix performance when reading objects with alternates

[Justin Tobler <jltobler@gmail.com>]

On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> With the move towards pluggable object databases we reorganized how we
> access alternates. Before the introduction of pluggability, we were
> reading objects by first scanning through packfiles from all alternates
> before scanning through all loose object directories. With the change
> though, we are now iterating through the sources one by one, checking
> each of them for whether they contain the object either as a packed or
> loose object.

Ok, ideally we want to go back to checking all the packfiles across each
source for a given object before looking through the loose objects. This
would be more performant. Make sense.

> Checking whether an object exists in packfiles is comparatively cheap
> because the list of packs is cached, and the pack indices are likely to
> be in the page cache. In contrast to that, checking whether an object
> exists in loose format always incurs one stat(3p) syscall. So scanning
> for objects one alternate at a time led to a performance regression in
> in the case where most of the objects are contained in a packfile of an
> alternate, as reported in [1].

Ok.

> We could fix that issue by introducing another flag that tells the
> backends to only search the optimized storage backend. But that is
> adding more complexity to the object database layers that is ultimately
> only really required because of the way the "files" backend stores its
> data. So for an alternate backend, such new infrastructure would not
> really be a good fit.

I completely agree. Introducing a special flag here would really only
serve the "files" backend and also make the overall interface more
confusing. It would be best to capture the desired behavior as an
implementation detail of the "files" backend itself.

> But since then we have moved handling of alternates into the "files"
> backend itself. One of the consequences of this is that this backend is
> now having full control over all of its sub-sources, and that also makes
> it way easier for us to optimize for this access pattern again.

Yup, now that alternates are controlled/managed by the "files" backend
itself instead of being discrete sources themselves, it becomes possible
us to update how objects are read internal to the source.

> Do so by first trying to read objects from packfiles located in any of
> its object directories before trying to read loose objects.

Make sense.

> The following benchmark demonstrates that this significantly improves
> performance in some cases. The benchmark uses the Git repository itself
> and sets up its object directory as an alternate, while the primary
> object directory is simply an empty object directory. It then reads all
> commits enumerated via `git rev-list HEAD` with git-cat-file(1):
> 
>   Benchmark 1: git cat-file --batch-check --unordered (rev = HEAD~)
>     Time (mean ± σ):     179.4 ms ±   2.6 ms    [User: 114.6 ms, System: 61.4 ms]
>     Range (min … max):   177.1 ms … 187.7 ms    16 runs
> 
>   Benchmark 2: git cat-file --batch-check --unordered (rev = HEAD)
>     Time (mean ± σ):      95.0 ms ±   1.5 ms    [User: 79.8 ms, System: 15.0 ms]
>     Range (min … max):    93.0 ms …  97.9 ms    30 runs
> 
>   Summary
>     git cat-file --batch-check --unordered (rev = HEAD) ran
>       1.89 ± 0.04 times faster than git cat-file --batch-check --unordered (rev = HEAD~)

Nice.

> [1]: <CAFXJcxvpKHoVDwE5mBOd=w-A5vPdUmehqr8SHLUD7qv1qB00rA@mail.gmail.com>
> 
> Reported-by: Wolfgang Kritzinger <wkritzinger@atlassian.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb/source-files.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/odb/source-files.c b/odb/source-files.c
> index 455f409db0..9268f5aa9a 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -426,6 +426,7 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
>  {
>  	struct odb_source_files *files = odb_source_files_downcast(source);
>  	enum odb_read_status status = ODB_READ_NOT_FOUND;
> +	enum odb_read_status ret;
>  
>  	/*
>  	 * Reading an object may fail even though the object exists, for
> @@ -434,15 +435,15 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
>  	 * prefer the first such error in case multiple reads have failed.
>  	 */
>  	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next) {
> -		enum odb_read_status ret;
> -
>  		ret = odb_source_read_object_info(&dir->packed->base, oid, oi, flags,
>  						  status == ODB_READ_NOT_FOUND ? errmsg : NULL);
>  		if (!ret)
>  			return 0;
>  		if (ret != ODB_READ_NOT_FOUND && status == ODB_READ_NOT_FOUND)
>  			status = ret;
> +	}
>  
> +	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next) {

Ok, now we iterate through each source to look through all the packs
before looking through the loose objects for each source. This matches
the behavior desribed above and look good.

-Justin