Optimize performance of running command finalization hooks#1504
Optimize performance of running command finalization hooks#1504tleonhardt merged 5 commits intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1504 +/- ##
==========================================
+ Coverage 98.76% 98.79% +0.02%
==========================================
Files 23 23
Lines 4953 4961 +8
==========================================
+ Hits 4892 4901 +9
+ Misses 61 60 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
I did some manual testing and verified that the I also verified that neither approach works against some really pathological situations such as changing the character set being used by the terminal. |
|
@kmvanbrunt Take a look and let me know what you think. I think the performance improvements are worth it and my manual testing shows it is working well. For the life of me I can't figure out how to mock things to get full coverage of the new code with tests. |
|
Score one for Gemini 2.5 Pro for one-shotting the 100% test coverage for the new code. |
Optimize performance of the
_run_cmdfinalization_hooksmethod by replacing a subprocess call tostty sanewith a call totermios.tcsetattr(). For optimial performance, the initial termios settings are cached when instantiating thecmd2.Cmdclass and are restored in the_run_cmdfinalization_hooksmethod.The motivation behind this was a user report of slowness in Discussion #1503.
I also temporarily tweaked where we we report our elapsed timing when the
timiingsettable is True to include the full command lifecycle timing and did measurements before and after.I did some testing on a system with the following setup:
cmd2from main branch on GitHubin this testing I modified cmd's built-in timing measurement to capture the full command lifecycle timing. With the code as-is on the
mainbranch, the full lifecycle for an empty command on this older computer was about 0.007 seconds (7 thousands of a second). This is not something I would even remotely call "slow" given typical human reaction times.I then re-ran the same tests using the code in this PR. This did substantially speed things up to the point where an empty command ran with an elapsed time of about 0.00011 seconds (about 64 times faster).
I think we should do some manual evaluation with the code here to verify it is fixing the same things that
stty sanewas fixing. Even though the performance difference should be below what would normally be perceptible for the system I tested on, user reports of slowness in some circumstances suggest this may be a beneficial and noticeable change overall.TODO:
stty sane