This is an archive of the discontinued Mercurial Phabricator instance.

progress: avoid ui.configbool() lookup when progress bar is active
ClosedPublic

Authored by indygreg on Dec 10 2018, 3:15 PM.

Details

Summary

Profiling revealed that the ui.configbool('progress', 'debug') during
progress bar updates was consuming a significant amount of overhead.

This commit adds an attribute on progress bar instances that caches
this config option.

The impact on hg perfprogress with default options is significant:

before: ! wall 4.641942 comb 4.580000 user 4.210000 sys 0.370000 (best of 3)
after: ! wall 1.948626 comb 1.950000 user 1.950000 sys 0.000000 (best of 5)

After this change, profiling reveals that progress.progbar.progress()
is now consuming ~73% of time.

This change does not improve the execution time if the progress bar
is disabled. We may want a more comprehensive solution for that case,
as the progress bar won't be enabled in a number of scenarios (e.g.
servers and processes not attached to an interactive TTY).

I also think that overhead of ~2.0s for 1M updates is a bit high.
I suspect further refactoring of the progress bar can significantly
reduce overhead. I don't have plans to do this, however.

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.Dec 10 2018, 3:15 PM
pulkit added a subscriber: pulkit.Dec 11 2018, 6:00 AM

To add on what commit message says, we have also internally seen progress bar taking a lot of time. I am +1 on getting this change in core.

yuja added a subscriber: yuja.Dec 11 2018, 8:45 AM

The idea sounds good to me. Going further, maybe we can get rid of the ui
reference from the progbar. There's a reference cycle.

DO NOT LAND. test-check-config.t COMPLAINS FOR SOME REASON.

Probably missing # developer config: progress.debug. The checker isn't
aware of self being ui.

pulkit edited the summary of this revision. (Show Details)Dec 26 2018, 12:06 PM
pulkit retitled this revision from progress: [RFC] avoid ui.configbool() lookup when progress bar is active to progress: avoid ui.configbool() lookup when progress bar is active.
pulkit updated this revision to Diff 12982.

It turned out to be important optimization for us. Since it's holiday in US, I updated this with Yuya's comment.

This revision was automatically updated to reflect the committed changes.