This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1394

bundle2: avoid unbound read when seeking
ClosedPublic

Authored by indygreg on Nov 14 2017, 1:26 AM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHG699b2a759319: bundle2: avoid unbound read when seeking
Summary

Currently, seekableunbundlepart.seek() will perform a read() during
seek operations. This will allocate a buffer to hold the raw data
over the seek distance. This can lead to very large allocations
and cause performance to suffer.

We change the code to perform read(32768) in a loop to avoid
potentially large allocations.

hg perfbundleread on an uncompressed Firefox bundle reveals
a performance impact:

! bundle2 iterparts()
! wall 2.992605 comb 2.990000 user 2.260000 sys 0.730000 (best of 4)
! bundle2 iterparts() seekable
! wall 3.863810 comb 3.860000 user 3.000000 sys 0.860000 (best of 3)
! bundle2 part seek()
! wall 6.213387 comb 6.200000 user 3.350000 sys 2.850000 (best of 3)
! wall 3.820347 comb 3.810000 user 2.980000 sys 0.830000 (best of 3)

Since seekable bundle parts are (only) used by bundlerepo, this /may/
speed up initial loading of bundle-based repos. But any improvement
will likely only be noticed on very large bundles.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Nov 14 2017, 1:26 AM
durin42 accepted this revision.Nov 20 2017, 6:43 PM
This revision is now accepted and ready to land.Nov 20 2017, 6:43 PM
Closed by commit rHG699b2a759319: bundle2: avoid unbound read when seeking (authored by indygreg). · Explain WhyNov 20 2017, 6:50 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/bundle2.py (11 lines)

Diff 3698

mercurial/bundle2.py

def seek(self, offset, whence=os.SEEK_SET): def seek(self, offset, whence=os.SEEK_SET):
if whence == os.SEEK_SET: if whence == os.SEEK_SET:
newpos = offset newpos = offset
elif whence == os.SEEK_CUR: elif whence == os.SEEK_CUR:
newpos = self._pos + offset newpos = self._pos + offset
elif whence == os.SEEK_END: elif whence == os.SEEK_END:
if not self.consumed: if not self.consumed:
self.read() # Can't use self.consume() here because it advances self._pos.
chunk = self.read(32768)
while chunk:
chunk = self.read(32768)
newpos = self._chunkindex[-1][0] - offset newpos = self._chunkindex[-1][0] - offset
else: else:
raise ValueError('Unknown whence value: %r' % (whence,)) raise ValueError('Unknown whence value: %r' % (whence,))
if newpos > self._chunkindex[-1][0] and not self.consumed: if newpos > self._chunkindex[-1][0] and not self.consumed:
self.read() # Can't use self.consume() here because it advances self._pos.
chunk = self.read(32768)
while chunk:
chunk = self.read(32668)
if not 0 <= newpos <= self._chunkindex[-1][0]: if not 0 <= newpos <= self._chunkindex[-1][0]:
raise ValueError('Offset out of range') raise ValueError('Offset out of range')
if self._pos != newpos: if self._pos != newpos:
chunk, internaloffset = self._findchunk(newpos) chunk, internaloffset = self._findchunk(newpos)
self._payloadstream = util.chunkbuffer(self._payloadchunks(chunk)) self._payloadstream = util.chunkbuffer(self._payloadchunks(chunk))
adjust = self.read(internaloffset) adjust = self.read(internaloffset)
if len(adjust) != internaloffset: if len(adjust) != internaloffset: