make Lookup() return all blobs by aawsome · Pull Request #2789 · restic/restic

aawsome · 2020-06-14T19:09:07Z

What is the purpose of this change? What does it change?

Change repository.Index.Lookup() to return all results.

Currently, Lookup returns all results including duplicates within the first index file with first match; it however does not return all duplicates. This PR "corrects" Lookup to return all duplicates.

Was the change discussed in an issue or in the forum before?

see #2523

Checklist

I have read the Contribution Guidelines
I have enabled maintainer edits for this PR
I have added tests for all changes in this PR (and some more)
I have not added documentation for the changes (in the manual)
There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
I have run gofmt on the code in all commits
All commit messages are formatted in the same style as the other commits in the repo
I'm done, this Pull Request is ready for review

greatroar · 2020-06-15T07:16:06Z

Great! I was wanting to do this for #2788, but settled on a more local patch.

internal/repository/index.go

internal/repository/master_index.go

MichaelEischer

I've thought a bit more about the effects of adding Lookup and LookupAll. The main difference is probably that Repository.LoadBlob now has to traverse the full index, instead of only half on average. This is probably most noticeable for the check command which issues a massive amount of LoadBlob calls (via LoadTree).

Just to get an idea of how many Lookup calls are issued by check: I've used PR #2328 (check optimization), which already dramatically reduces the number of LoadBlob calls, on my large test repository which has a 7GB index with 729 files. LoadBlob spends roughly 40% of its runtime in the Lookup call (which amounts to more than half an hour CPU time). Using LookupAll would increase that to nearly 60%. That probably sounds worse than it actually is as the index lookup only amounts to 6% of the overall runtime, so that the slowdown should be tolerable.

For another repository using the standard check implementation the Lookup/LookupAll call also only requires a small part of the overall runtime.

Regarding LookupBlobSize I would be interested to see a small performance benchmark similar to the BenchmarkMasterIndexLookupMultipleIndex benchmark that already exists for the master index.

I think this PR also needs a test that Lookup and LookupAll really work as expected. The adapted tests probably don't completely cover the new semantics, especially of LookupAll.

internal/restorer/filerestorer.go

internal/restorer/restorer.go

aawsome · 2020-06-18T16:09:40Z

@MichaelEischer Maybe we should first talk about #2799 which should solve the "many index files" issue before continuing to work on this?

However, I'll take care of the points you mentioned!

greatroar · 2020-06-28T17:37:53Z

Closes #2701.

aawsome · 2020-07-06T15:17:03Z

Regarding LookupBlobSize I would be interested to see a small performance benchmark similar to the BenchmarkMasterIndexLookupMultipleIndex benchmark that already exists for the master index.

I think this PR also needs a test that Lookup and LookupAll really work as expected. The adapted tests probably don't completely cover the new semantics, especially of LookupAll.

I added this benchmark and tests. All code in Lookup and LookupAll (both in index.go and master_index.go) is now covered by the tests.

greatroar · 2020-07-06T19:18:00Z

LookupBlobSize isn't covered, though. The restorer tests don't hit it either (low coverage), nor do the fuse ones (they use the blobSizeCache).

As an aside, it's funny how the MasterIndex is now the only type that implements restic.Index and everything still works.

aawsome · 2020-07-06T20:26:09Z

LookupBlobSize isn't covered, though. The restorer tests don't hit it either (low coverage), nor do the fuse ones (they use the blobSizeCache).

I added a bunch of easy-to-add tests to improve test coverage.

As an aside, it's funny how the MasterIndex is now the only type that implements restic.Index and everything still works.

😃 Yes - in fact, internal/repository/index.Index doesn't need to implement restic.Index.

MichaelEischer

I just took another look at this PR. Besides tests there's just a single user of Lookup and that is inside cmd_cat.go which doesn't matter for performance as it's just a single call. All other uses which just need a single result blob seem to be covered by LookupSize.

So lets keep this simple: please drop the Lookup method and rename LookupAll to Lookup.

aawsome · 2020-07-25T12:24:33Z

I just took another look at this PR. Besides tests there's just a single user of Lookup and that is inside cmd_cat.go which doesn't matter for performance as it's just a single call. All other uses which just need a single result blob seem to be covered by LookupSize.

So lets keep this simple: please drop the Lookup method and rename LookupAll to Lookup.

You are right and especially after #2818 is merged, there shouldn't be much performance issues any more.
So this PR is now merely a code clean.

I rebased and made your suggested changes.

Seems, we have a CI issue ATM...

MichaelEischer

I found a few further small issues. Once these are fixed, the PR should be ready for merging.

internal/archiver/blob_saver_test.go

internal/repository/index.go

internal/repository/master_index_test.go

+ simplify syntax

The interface is now only implemented by repository.MasterIndex.

MichaelEischer

LGTM. Thanks for the cleanup!

aawsome · 2020-07-25T20:18:24Z

@MichaelEischer Wow you are really working through the PRs today! Thanks a lot for all your work! I really appreciate a lot what you are supplying here! Also know about the burden of "the one who has to make the decission"...
👍

I really like the progress restic is making. Wondering whether all the improvements done in the last months would justify a new release.. 😉

rawtaz · 2020-07-25T20:59:26Z

LOL, you don't think we have thought about a new release? :P

Thing is, it's not just you guys' PRs and work that should go into a new release, but most of the developer time (primarily Michael's) is going to that instead of older PRs and stuff ;)

I too think the last few months have given a lot of improvement to restic, in part thanks to your contributions!

aawsome · 2020-07-26T19:06:11Z

LOL, you don't think we have thought about a new release? :P

Thing is, it's not just you guys' PRs and work that should go into a new release, but most of the developer time (primarily Michael's) is going to that instead of older PRs and stuff ;)

Sure.. is there a roadmap what points you maintainers see as important milestones or priorities towards a next release? (If there is, I just didn't find it so far). Please give me a hint if I can help with something.

rawtaz · 2020-07-26T20:28:39Z

Thank you @aawsome, but I was mostly thinking about having mentioned an upcoming release in a few places by now. But it's been like that for a while. We mostly have discussions amongst ourselves as part of the ongoing work to get a new release out, there's no roadmap I can point you to. It's done when it's done, and that shouldn't be too far into the future :) Thanks again!

greatroar reviewed Jun 15, 2020

View reviewed changes

internal/repository/index.go Outdated Show resolved Hide resolved

greatroar mentioned this pull request Jun 15, 2020

Reimplement LookupBlobSize without loop + allocation #2788

Closed

8 tasks

aawsome force-pushed the fix-lookup branch from d2ece80 to 49617b5 Compare June 15, 2020 19:40

greatroar reviewed Jun 16, 2020

View reviewed changes

internal/repository/master_index.go Outdated Show resolved Hide resolved

greatroar reviewed Jun 16, 2020

View reviewed changes

internal/repository/master_index.go Outdated Show resolved Hide resolved

greatroar mentioned this pull request Jun 16, 2020

Make LookupAll append and optimize its callers aawsome/restic#2

Closed

aawsome force-pushed the fix-lookup branch from 49617b5 to 0ef5175 Compare June 16, 2020 19:01

greatroar mentioned this pull request Jun 17, 2020

Remove blob size cache from restic mount #2787

Merged

8 tasks

MichaelEischer reviewed Jun 17, 2020

View reviewed changes

internal/restorer/filerestorer.go Outdated Show resolved Hide resolved

internal/restorer/restorer.go Outdated Show resolved Hide resolved

aawsome force-pushed the fix-lookup branch from 0ef5175 to 2a4d33d Compare July 6, 2020 15:10

aawsome force-pushed the fix-lookup branch 2 times, most recently from 1563e74 to 50366da Compare July 6, 2020 20:22

MichaelEischer reviewed Jul 25, 2020

View reviewed changes

aawsome force-pushed the fix-lookup branch from 50366da to 663ec95 Compare July 25, 2020 12:17

aawsome changed the title ~~make Lookup() really return just one blob, add LookupAll()~~ make Lookup() return all blobs Jul 25, 2020

MichaelEischer requested changes Jul 25, 2020

View reviewed changes

internal/archiver/blob_saver_test.go Show resolved Hide resolved

internal/repository/index.go Outdated Show resolved Hide resolved

internal/repository/index.go Outdated Show resolved Hide resolved

internal/repository/master_index_test.go Outdated Show resolved Hide resolved

aawsome force-pushed the fix-lookup branch from 663ec95 to e753498 Compare July 25, 2020 18:10

aawsome and others added 2 commits July 25, 2020 21:18

make Lookup() return all blobs

9d1fb94

+ simplify syntax

Rename Index interface to MasterIndex

c847aac

The interface is now only implemented by repository.MasterIndex.

MichaelEischer force-pushed the fix-lookup branch from e753498 to c847aac Compare July 25, 2020 19:21

MichaelEischer approved these changes Jul 25, 2020

View reviewed changes

MichaelEischer merged commit 573a2fb into restic:master Jul 25, 2020

MichaelEischer mentioned this pull request Jul 25, 2020

Prevent double index lookup in restic cat #2701

Closed

8 tasks

aawsome deleted the fix-lookup branch December 7, 2020 08:42

Conversation

aawsome commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of this change? What does it change?

Was the change discussed in an issue or in the forum before?

Checklist

Uh oh!

greatroar commented Jun 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaelEischer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aawsome commented Jun 18, 2020

Uh oh!

greatroar commented Jun 28, 2020

Uh oh!

aawsome commented Jul 6, 2020

Uh oh!

greatroar commented Jul 6, 2020

Uh oh!

aawsome commented Jul 6, 2020

Uh oh!

MichaelEischer left a comment

Choose a reason for hiding this comment

Uh oh!

aawsome commented Jul 25, 2020

Uh oh!

MichaelEischer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaelEischer left a comment

Choose a reason for hiding this comment

Uh oh!

aawsome commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rawtaz commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aawsome commented Jul 26, 2020

Uh oh!

rawtaz commented Jul 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aawsome commented Jun 14, 2020 •

edited

Loading

aawsome commented Jul 25, 2020 •

edited

Loading

rawtaz commented Jul 25, 2020 •

edited

Loading