Skip to content

Conversation

tleonhardt
Copy link
Member

@tleonhardt tleonhardt commented Sep 7, 2025

Optimize performance of the _run_cmdfinalization_hooks method by replacing a subprocess call to stty sane with a call to termios.tcsetattr(). For optimial performance, the initial termios settings are cached when instantiating the cmd2.Cmd class and are restored in the _run_cmdfinalization_hooks method.

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 timiing settable 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:

  • Latest cmd2 from main branch on GitHub
  • Python 3.14.0rc2
  • macOS Sequoia 15.6.1 on an older Intel core i9 Macbook pro
  • term2 3.5.14

in this testing I modified cmd's built-in timing measurement to capture the full command lifecycle timing. With the code as-is on the main branch, 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 sane was 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:

  • Manual testing to verify new code has same beneficial effects as running stty sane
  • Improve test coverage

Copy link

codecov bot commented Sep 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.79%. Comparing base (f7f8a8d) to head (2d8fae5).
⚠️ Report is 1 commits behind head on main.

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     
Flag Coverage Δ
unittests 98.79% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tleonhardt
Copy link
Member Author

tleonhardt commented Sep 7, 2025

I did some manual testing and verified that the termios approach of restoring terminal sanity works just as well as ssty sane for fixing common terminal corruption issues like broken character echo, broken line editing, etc.

I also verified that neither approach works against some really pathological situations such as changing the character set being used by the terminal.

@tleonhardt
Copy link
Member Author

@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.

@tleonhardt
Copy link
Member Author

Score one for Gemini 2.5 Pro for one-shotting the 100% test coverage for the new code.

@tleonhardt tleonhardt merged commit d0b21b5 into main Sep 8, 2025
30 checks passed
@tleonhardt tleonhardt deleted the stty-sane-optimization branch September 8, 2025 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants