Skip to content

Conversation

@bscottm
Copy link
Contributor

@bscottm bscottm commented Mar 20, 2025

Don't repeatedly call GetConsoleMode() on Win32 each time sim_console_write() is called.

Use an output function pointer to invoke WriteConsoleA (console output) or WriteFile (output is not a console). The console output destination doesn't change during the lifetime of the simulator, so avoid extraneous overhead for each character output (sometimes strings, but mostly characters.)

@markpizz
Copy link
Contributor

Improving efficiency is a good idea, but will hardly matter in a running simulator. Measurements would justify it.

Meanwhile, either the steering committee has changed the project commit message goals, or you forgot to format the commit message identifying the affected component (or simulator). The original commit message rules were specifically recommended by one of the steering committee members.

Additionally, when I was the gateway for commits, the convention I had folks follow was:

  1. when changing existing source modules, to follow the existing layout and indentation conventions already in use in the module being changed.
  2. New source modules let the author follow his preferred conventions (hopefully consistently in each new module).

Otherwise this code in this PR is OK with me.

Copy link

@LegalizeAdulthood LegalizeAdulthood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bscottm
Copy link
Contributor Author

bscottm commented Mar 22, 2025

@markpizz: GetConsoleMode() invokes the DeviceIOControl() system call, which incurs a system call context switch overhead. That a context switch is unnecessary overhead should be immediately obvious to a casual observer,

While I'm at it, other than using astyle to reformat code in the "banner" style, are there actual "banner" style guidelines laid down somewhere? Because it seems to be so obscure as to be largely forgotten or generally discarded.

@pkoning2: This is the equivalent of calling isatty() for each output character, which incurs the system call context switch overhead incurred by an ioctl().

@markpizz
Copy link
Contributor

Improving efficiency is a good idea., but will hardly matter in a running simulator. Measurements would justify it.

@markpizz: GetConsoleMode() invokes the DeviceIOControl() system call, which incurs a system call context switch overhead.

I agree totally and never said anything to the contrary.

@pkoning2: This is the equivalent of calling isatty() for each output character, which incurs the system call context switch overhead incurred by an ioctl().

I agree totally and never said anything to the contrary.

Since you:

  1. have already done the leg work to implement it
    AND
  2. it involves a handful of lines of code in one source module

I suggested it be adopted with consideration of my other points about:

  1. commit message format,
  2. indentation style issue

I also suggested that the net result of this change would probably not be measurable in a running simulator.

That a context switch is unnecessary overhead should be immediately obvious to a casual observer,

AGAIN, I said it should be adopted.

My suspicion is that since ALL of the simh the simulators do at least 99% of their work in code not explicitly part of the SCP framework, this very real optimization will be hard to see a difference to those simulators.

Feel free to provide measurements of a complete simulator run (maybe one of the existing build tests) before and after the adoption to get everyone excited about the change.

AGAIN, I said it should be adopted.

@bscottm
Copy link
Contributor Author

bscottm commented Mar 22, 2025

Improving efficiency is a good idea., but will hardly matter in a running simulator. Measurements would justify it.

@markpizz: GetConsoleMode() invokes the DeviceIOControl() system call, which incurs a system call context switch overhead.

I agree totally and never said anything to the contrary.

So, you didn't say "Improving efficiency is a good idea, but will hardly matter in a running simulator. Measurements would justify it."?

@markpizz
Copy link
Contributor

So, you didn't say "Improving efficiency is a good idea, but will hardly matter in a running simulator. Measurements would justify it."?

What are you talking about? I said exactly that in my first comment on this PR. By saying that I DIDN'T mean that the change was bad, but that measurements would justify the claim that it would make a difference to a simulator user. Sorry if that caused confusion.

@bscottm
Copy link
Contributor Author

bscottm commented Mar 24, 2025

So, you didn't say "Improving efficiency is a good idea, but will hardly matter in a running simulator. Measurements would justify it."?

What are you talking about? I said exactly that in my first comment on this PR. By saying that I DIDN'T mean that the change was bad, but that measurements would justify the claim that it would make a difference to a simulator user. Sorry if that caused confusion.

The overhead of GetConsolemode() is not visually observable. It doesn't take a rocket scientist to observe that the system call is unnecessary overhead and measurements are not needed.

Don't repeatedly call GetConsoleMode() on Win32 each time
sim_console_write() is called.

Use an output function pointer to invoke WriteConsoleA (console output)
or WriteFile (output is not a console). The console output destination
doesn't change during the lifetime of the simulator, so avoid
extraneous overhead for each character output (sometimes strings, but
mostly characters.)
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.

3 participants