This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1386

bundle2: extract logic for seeking bundle2 part into own class
ClosedPublic

Authored by indygreg on Nov 13 2017, 11:59 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHG073eec083e25: bundle2: extract logic for seeking bundle2 part into own class
Summary

Currently, unbundlepart classes support bi-directional seeking.
Most consumers of unbundlepart only ever seek forward - typically
as part of moving to the end of the bundle part so they can move
on to the next one. But regardless of the actual usage of the
part, instances maintain an index mapping offsets within the
underlying raw payload to offsets within the decoded payload.

Maintaining the mapping of offset data can be expensive in terms of
memory use. Furthermore, many bundle2 consumers don't have access
to an underlying seekable stream. This includes all compressed
bundles. So maintaining offset data when the underlying stream
can't be seeked anyway is wasteful. And since many bundle2 streams
can't be seeked, it seems like a bad idea to expose a seek API
in bundle2 parts by default. If you provide them, people will
attempt to use them.

Seekable bundle2 parts should be the exception, not the rule. This
commit starts the process dividing unbundlepart into 2 classes: a
base class that supports linear, one-time reads and a child class
that supports bi-directional seeking. In this first commit, we
split various methods and attributes out into a new
"seekableunbundlepart" class. Previous instantiators of "unbundlepart"
now instantiate "seekableunbundlepart." This preserves backwards
compatibility. The coupling between the classes is still tight:
"unbundlepart" cannot be used on its own. This will be addressed
in subsequent commits.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Nov 13 2017, 11:59 PM
durin42 accepted this revision.Nov 20 2017, 6:40 PM
This revision is now accepted and ready to land.Nov 20 2017, 6:40 PM
Closed by commit rHG073eec083e25: bundle2: extract logic for seeking bundle2 part into own class (authored by indygreg). · Explain WhyNov 20 2017, 6:49 PM
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.Nov 28 2017, 2:05 PM
martinvonz added inline comments.
mercurial/bundle2.py
1210

This seems unrelated. The field is still assigned in this class e.g. on line 1311.

Revision Contents
Changeset List

PathPackages
Mmercurial/bundle2.py (109 lines)

Diff 3464

mercurial/bundle2.py

"""yield all parts contained in the stream""" """yield all parts contained in the stream"""
# make sure param have been loaded # make sure param have been loaded
self.params self.params
# From there, payload need to be decompressed # From there, payload need to be decompressed
self._fp = self._compengine.decompressorreader(self._fp) self._fp = self._compengine.decompressorreader(self._fp)
indebug(self.ui, 'start extraction of bundle2 parts') indebug(self.ui, 'start extraction of bundle2 parts')
headerblock = self._readpartheader() headerblock = self._readpartheader()
while headerblock is not None: while headerblock is not None:
part = unbundlepart(self.ui, headerblock, self._fp) part = seekableunbundlepart(self.ui, headerblock, self._fp)
yield part yield part
# Seek to the end of the part to force it's consumption so the next # Seek to the end of the part to force it's consumption so the next
# part can be read. But then seek back to the beginning so the # part can be read. But then seek back to the beginning so the
# code consuming this generator has a part that starts at 0. # code consuming this generator has a part that starts at 0.
part.seek(0, os.SEEK_END) part.seek(0, os.SEEK_END)
part.seek(0, os.SEEK_SET) part.seek(0, os.SEEK_SET)
headerblock = self._readpartheader() headerblock = self._readpartheader()
indebug(self.ui, 'end of bundle2 stream') indebug(self.ui, 'end of bundle2 stream')
self.ui.debug('bundle2-input-stream-interrupt:' self.ui.debug('bundle2-input-stream-interrupt:'
' opening out of band context\n') ' opening out of band context\n')
indebug(self.ui, 'bundle2 stream interruption, looking for a part.') indebug(self.ui, 'bundle2 stream interruption, looking for a part.')
headerblock = self._readpartheader() headerblock = self._readpartheader()
if headerblock is None: if headerblock is None:
indebug(self.ui, 'no part found during interruption.') indebug(self.ui, 'no part found during interruption.')
return return
part = unbundlepart(self.ui, headerblock, self._fp) part = seekableunbundlepart(self.ui, headerblock, self._fp)
op = interruptoperation(self.ui) op = interruptoperation(self.ui)
hardabort = False hardabort = False
try: try:
_processpart(op, part) _processpart(op, part)
except (SystemExit, KeyboardInterrupt): except (SystemExit, KeyboardInterrupt):
hardabort = True hardabort = True
raise raise
finally: finally:
self.consumed = False self.consumed = False
# part data # part data
self.id = None self.id = None
self.type = None self.type = None
self.mandatoryparams = None self.mandatoryparams = None
self.advisoryparams = None self.advisoryparams = None
self.params = None self.params = None
self.mandatorykeys = () self.mandatorykeys = ()
self._payloadstream = None
martinvonzUnsubmitted
Not Done

This seems unrelated. The field is still assigned in this class e.g. on line 1311.

martinvonz: This seems unrelated. The field is still assigned in this class e.g. on line 1311.
self._readheader() self._readheader()
self._mandatory = None self._mandatory = None
self._chunkindex = [] #(payload, file) position tuples for chunk starts
self._pos = 0 self._pos = 0
def _fromheader(self, size): def _fromheader(self, size):
"""return the next <size> byte from the header""" """return the next <size> byte from the header"""
offset = self._headeroffset offset = self._headeroffset
data = self._headerdata[offset:(offset + size)] data = self._headerdata[offset:(offset + size)]
self._headeroffset = offset + size self._headeroffset = offset + size
return data return data
# make it read only to prevent people touching it by mistake. # make it read only to prevent people touching it by mistake.
self.mandatoryparams = tuple(mandatoryparams) self.mandatoryparams = tuple(mandatoryparams)
self.advisoryparams = tuple(advisoryparams) self.advisoryparams = tuple(advisoryparams)
# user friendly UI # user friendly UI
self.params = util.sortdict(self.mandatoryparams) self.params = util.sortdict(self.mandatoryparams)
self.params.update(self.advisoryparams) self.params.update(self.advisoryparams)
self.mandatorykeys = frozenset(p[0] for p in mandatoryparams) self.mandatorykeys = frozenset(p[0] for p in mandatoryparams)
def _payloadchunks(self, chunknum=0):
'''seek to specified chunk and start yielding data'''
if len(self._chunkindex) == 0:
assert chunknum == 0, 'Must start with chunk 0'
self._chunkindex.append((0, self._tellfp()))
else:
assert chunknum < len(self._chunkindex), \
'Unknown chunk %d' % chunknum
self._seekfp(self._chunkindex[chunknum][1])
pos = self._chunkindex[chunknum][0]
payloadsize = self._unpack(_fpayloadsize)[0]
indebug(self.ui, 'payload chunk size: %i' % payloadsize)
while payloadsize:
if payloadsize == flaginterrupt:
# interruption detection, the handler will now read a
# single part and process it.
interrupthandler(self.ui, self._fp)()
elif payloadsize < 0:
msg = 'negative payload chunk size: %i' % payloadsize
raise error.BundleValueError(msg)
else:
result = self._readexact(payloadsize)
chunknum += 1
pos += payloadsize
if chunknum == len(self._chunkindex):
self._chunkindex.append((pos, self._tellfp()))
yield result
payloadsize = self._unpack(_fpayloadsize)[0]
indebug(self.ui, 'payload chunk size: %i' % payloadsize)
def _findchunk(self, pos):
'''for a given payload position, return a chunk number and offset'''
for chunk, (ppos, fpos) in enumerate(self._chunkindex):
if ppos == pos:
return chunk, 0
elif ppos > pos:
return chunk - 1, pos - self._chunkindex[chunk - 1][0]
raise ValueError('Unknown chunk')
def _readheader(self): def _readheader(self):
"""read the header and setup the object""" """read the header and setup the object"""
typesize = self._unpackheader(_fparttypesize)[0] typesize = self._unpackheader(_fparttypesize)[0]
self.type = self._fromheader(typesize) self.type = self._fromheader(typesize)
indebug(self.ui, 'part type: "%s"' % self.type) indebug(self.ui, 'part type: "%s"' % self.type)
self.id = self._unpackheader(_fpartid)[0] self.id = self._unpackheader(_fpartid)[0]
indebug(self.ui, 'part id: "%s"' % pycompat.bytestr(self.id)) indebug(self.ui, 'part id: "%s"' % pycompat.bytestr(self.id))
# extract mandatory bit from type # extract mandatory bit from type
self._pos += len(data) self._pos += len(data)
if size is None or len(data) < size: if size is None or len(data) < size:
if not self.consumed and self._pos: if not self.consumed and self._pos:
self.ui.debug('bundle2-input-part: total payload size %i\n' self.ui.debug('bundle2-input-part: total payload size %i\n'
% self._pos) % self._pos)
self.consumed = True self.consumed = True
return data return data
class seekableunbundlepart(unbundlepart):
"""A bundle2 part in a bundle that is seekable.
Regular ``unbundlepart`` instances can only be read once. This class
extends ``unbundlepart`` to enable bi-directional seeking within the
part.
Bundle2 part data consists of framed chunks. Offsets when seeking
refer to the decoded data, not the offsets in the underlying bundle2
stream.
To facilitate quickly seeking within the decoded data, instances of this
class maintain a mapping between offsets in the underlying stream and
the decoded payload. This mapping will consume memory in proportion
to the number of chunks within the payload (which almost certainly
increases in proportion with the size of the part).
"""
def __init__(self, ui, header, fp):
# (payload, file) offsets for chunk starts.
self._chunkindex = []
super(seekableunbundlepart, self).__init__(ui, header, fp)
def _payloadchunks(self, chunknum=0):
'''seek to specified chunk and start yielding data'''
if len(self._chunkindex) == 0:
assert chunknum == 0, 'Must start with chunk 0'
self._chunkindex.append((0, self._tellfp()))
else:
assert chunknum < len(self._chunkindex), \
'Unknown chunk %d' % chunknum
self._seekfp(self._chunkindex[chunknum][1])
pos = self._chunkindex[chunknum][0]
payloadsize = self._unpack(_fpayloadsize)[0]
indebug(self.ui, 'payload chunk size: %i' % payloadsize)
while payloadsize:
if payloadsize == flaginterrupt:
# interruption detection, the handler will now read a
# single part and process it.
interrupthandler(self.ui, self._fp)()
elif payloadsize < 0:
msg = 'negative payload chunk size: %i' % payloadsize
raise error.BundleValueError(msg)
else:
result = self._readexact(payloadsize)
chunknum += 1
pos += payloadsize
if chunknum == len(self._chunkindex):
self._chunkindex.append((pos, self._tellfp()))
yield result
payloadsize = self._unpack(_fpayloadsize)[0]
indebug(self.ui, 'payload chunk size: %i' % payloadsize)
def _findchunk(self, pos):
'''for a given payload position, return a chunk number and offset'''
for chunk, (ppos, fpos) in enumerate(self._chunkindex):
if ppos == pos:
return chunk, 0
elif ppos > pos:
return chunk - 1, pos - self._chunkindex[chunk - 1][0]
raise ValueError('Unknown chunk')
def tell(self): def tell(self):
return self._pos return self._pos
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