-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Optimize pack readHeader() implementation #1574
Conversation
Load pack header length and 15 header entries with single backend request. This eliminates separate header Load() request for most pack files and significantly improves index.New() performance. Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
6e299a6
to
953f3d5
Compare
const maxHeaderSize = 16 * 1024 * 1024 | ||
|
||
// we require at least one entry in the header, and one blob for a pack file | ||
var minFileSize = entrySize + crypto.Extension | ||
|
||
// number of header enries to download as part of header-length request | ||
var eagerEntries = uint(15) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fd0 This number is based on stats from single 435GB repository (where >98% of all packs have 15 or less header entries, fwiw). Maybe useful to get stats from other repositories, assuming you have access or have interested users who can provide the info.
|
||
return binary.LittleEndian.Uint32(buf), nil | ||
} | ||
|
||
const maxHeaderSize = 16 * 1024 * 1024 | ||
|
||
// we require at least one entry in the header, and one blob for a pack file | ||
var minFileSize = entrySize + crypto.Extension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to my change, but I believe minFileSize
should be 4 bytes longer to account for header length record at the end of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh indeed, that's right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks!
Optimize pack readHeader() implementation
What is the purpose of this change? What does it change?
Load pack header length and 15 header entries with single backend
request. This eliminates separate header Load() request for most pack
files and significantly improves index.New() performance.
Was the change discussed in an issue or in the forum before?
See #1567
Checklist
changelog/x.y.z
that describe the changes for our users (template here)gofmt
on the code in all commits