This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2022

ui: improve ui.write performance when not coloring on Windows
ClosedPublic

Authored by joerg.sonnenberger on Feb 3 2018, 6:31 PM.

Details

Reviewers
yuja
Group Reviewers
hg-reviewers
Commits
rHG0ff41ced4c12: diff: improve ui.write performance when not coloring on Windows

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

yuja requested changes to this revision.Feb 4 2018, 6:02 AM
yuja added a subscriber: yuja.
yuja added inline comments.
mercurial/logcmdutil.py
97

I slightly prefer passing chunks as an argument in place of oldchunks trick. But this is really minor nit.

If we make all diffui() batchable (e.g. _exportsingle()) as a follow up, difflabel() can
just take chunks as an argument.

mercurial/ui.py
873

Can you remove unused opts parameter?

I don't think it will be any useful since we want to feed chunks
at once where opts may vary.

878

Perhaps "label -> true" would be preferable than double negative "no label -> false".

882

I think this assert is irrelevant since "no label" writes can be batched.

This revision now requires changes to proceed.Feb 4 2018, 6:02 AM
yuja accepted this revision.Feb 7 2018, 8:24 AM

Dropped **opts and queued, thanks.

This revision is now accepted and ready to land.Feb 7 2018, 8:24 AM
Closed by commit rHG0ff41ced4c12: diff: improve ui.write performance when not coloring on Windows (authored by joerg.sonnenberger). · Explain WhyFeb 7 2018, 8:38 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/logcmdutil.py (35 lines)
Mmercurial/ui.py (11 lines)

Diff 5297

mercurial/logcmdutil.py

ui.warn(_('warning: %s not inside relative root %s\n') % ( ui.warn(_('warning: %s not inside relative root %s\n') % (
match.uipath(matchroot), uirelroot)) match.uipath(matchroot), uirelroot))
if stat: if stat:
diffopts = diffopts.copy(context=0, noprefix=False) diffopts = diffopts.copy(context=0, noprefix=False)
width = 80 width = 80
if not ui.plain(): if not ui.plain():
width = ui.termwidth() width = ui.termwidth()
chunks = patch.diff(repo, node1, node2, match, changes, opts=diffopts, chunks = patch.diff(repo, node1, node2, match, changes, opts=diffopts,
prefix=prefix, relroot=relroot, prefix=prefix, relroot=relroot,
hunksfilterfn=hunksfilterfn) hunksfilterfn=hunksfilterfn)
for chunk, label in patch.diffstatui(util.iterlines(chunks),
width=width): if fp is not None or ui.canwritewithoutlabels():
write(chunk, label=label) if stat:
chunks = patch.diffstat(util.iterlines(chunks), width=width)
for chunk in util.filechunkiter(util.chunkbuffer(chunks)):
write(chunk)
else:
if stat:
chunks = patch.diffstatui(util.iterlines(chunks), width=width)
else:
chunks = patch.difflabel(lambda chunks, **kwargs: chunks, chunks,
opts=diffopts)
yujaUnsubmitted
Not Done

I slightly prefer passing chunks as an argument in place of oldchunks trick. But this is really minor nit.

If we make all diffui() batchable (e.g. _exportsingle()) as a follow up, difflabel() can
just take chunks as an argument.

yuja: I slightly prefer passing `chunks` as an argument in place of `oldchunks` trick. But this is…
if ui.canbatchlabeledwrites():
def gen():
for chunk, label in chunks:
yield ui.label(chunk, label=label)
for chunk in util.filechunkiter(util.chunkbuffer(gen())):
write(chunk)
else: else:
for chunk, label in patch.diffui(repo, node1, node2, match, for chunk, label in chunks:
changes, opts=diffopts, prefix=prefix,
relroot=relroot,
hunksfilterfn=hunksfilterfn):
write(chunk, label=label) write(chunk, label=label)
if listsubrepos: if listsubrepos:
ctx1 = repo[node1] ctx1 = repo[node1]
ctx2 = repo[node2] ctx2 = repo[node2]
for subpath, sub in scmutil.itersubrepos(ctx1, ctx2): for subpath, sub in scmutil.itersubrepos(ctx1, ctx2):
tempnode2 = node2 tempnode2 = node2
try: try:
if node2 is not None: if node2 is not None:

mercurial/ui.py

self._bufferstates.pop() self._bufferstates.pop()
if self._bufferstates: if self._bufferstates:
self._bufferapplylabels = self._bufferstates[-1][2] self._bufferapplylabels = self._bufferstates[-1][2]
else: else:
self._bufferapplylabels = None self._bufferapplylabels = None
return "".join(self._buffers.pop()) return "".join(self._buffers.pop())
def canwritewithoutlabels(self):
yujaUnsubmitted
Not Done

Can you remove unused opts parameter?

I don't think it will be any useful since we want to feed chunks
at once where opts may vary.

yuja: Can you remove unused `opts` parameter? I don't think it will be any useful since we want to…
'''check if write skips the label'''
if self._buffers and not self._bufferapplylabels:
return True
return self._colormode is None
yujaUnsubmitted
Not Done

Perhaps "label -> true" would be preferable than double negative "no label -> false".

yuja: Perhaps "label -> true" would be preferable than double negative "no label -> false".
def canbatchlabeledwrites(self):
'''check if write calls with labels are batchable'''
# Windows color printing is special, see ``write``.
return self._colormode != 'win32'
yujaUnsubmitted
Not Done

I think this assert is irrelevant since "no label" writes can be batched.

yuja: I think this assert is irrelevant since "no label" writes can be batched.
def write(self, *args, **opts): def write(self, *args, **opts):
'''write args to output '''write args to output
By default, this method simply writes to the buffer or stdout. By default, this method simply writes to the buffer or stdout.
Color mode can be set on the UI class to have the output decorated Color mode can be set on the UI class to have the output decorated
with color modifier before being written to stdout. with color modifier before being written to stdout.
The color used is controlled by an optional keyword argument, "label". The color used is controlled by an optional keyword argument, "label".