Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for non-STDOUT streams #5

Closed
keithrbennett opened this issue Jan 30, 2021 · 14 comments
Closed

Add support for non-STDOUT streams #5

keithrbennett opened this issue Jan 30, 2021 · 14 comments

Comments

@keithrbennett
Copy link
Contributor

For methods that print to standard output (benchmark, bm, and bmbm), it would be nice to be able to specify that they go to another stream instead, such as an open file or a string buffer (StringIO), or even $stdout, in case it has been changed to something other than STDOUT.

In cases where the method signature would not support another parameter, an alternate method could be added in which the stream is a parameter, the body of the original method moved there, and the original method remaining to call the new method with STDOUT, e.g.:

def benchmark(caption = "", label_width = nil, format = nil, *labels)
  ...
end

...could become...

def benchmark_to(output_stream, caption = "", label_width = nil, format = nil, *labels)
  # body of original benchmark method
end

...and...

def benchmark(caption = "", label_width = nil, format = nil, *labels)
  benchmark_to(STDOUT, caption, label_width, format, *labels)
end

I believe all the likely objects for output would support puts, print, etc. I'm not sure about sync, but I suppose we could put a respond_to? guard around that call.

@marcandre
Copy link
Member

I agree that it should use $stdout instead of of STDOUT.

I believe sync is supported in all io-like objects including stringio.

@marcandre
Copy link
Member

@keithrbennett
Copy link
Contributor Author

Thanks for creating that issue, and for the information about sync.

Regarding changing the output to $stdout, might that not break existing code? The main point of this issue is to support all non-STDOUT outputs (see code examples), not only $stdout, but also stringio's and files. Doing so would enable the explicit use of $stdout where it would differ from STDOUT. What are your thoughts about supporting all non-STDOUT ouputs?

@marcandre
Copy link
Member

The idea is that you could at least do $stdout = my_non_stdout_output; benchmark { ... }; $stdout = STDOUT in the (rare) case where you need redirecting it.

@marcandre
Copy link
Member

might that not break existing code

Any change, including bug fixes, can break existing code. I'm not sure I see a good usecase that would be broken this way. Do you?

@keithrbennett
Copy link
Contributor Author

The idea is that you could at least do $stdout = my_non_stdout_output; benchmark { ... }; $stdout = STDOUT in the (rare) case where you need redirecting it.

Now I understand how you were accommodating the other streams, by temporarily modifying $stdout. That would usually work, but the developer might have other output intended for the "real" standard output, using a dedicated benchmark.log file for all benchmarking output, for example. Supporting providing an output stream for the benchmark would enable this, and also be simpler for the caller.

Any change, including bug fixes, can break existing code. I'm not sure I see a good usecase that would be broken this way. Do you?

I have no investment in this, I don't have any use case of my own -- but I can imagine that someone, somewhere, might be relying on having the benchmark output go to STDOUT while they have $stdout pointing somewhere else. That said, the judgment of whether or not to care about this is "above my paygrade" and I can go either way. :)

FWIW, I'm willing to be the one to modify the code to implement this, assuming I don't run into any weird hugely difficult obstacles.

@marcandre
Copy link
Member

marcandre commented Jan 31, 2021

If someone needs to benchmark output to $stdout (seems unlikely), then it would break, but it is easy to move the $stdout = their_output inside the benchmark block and restore it at before the end of the block.

@keithrbennett
Copy link
Contributor Author

Yes, easy, but requires additional code, at a level lower than merely specifying an output stream. Seeing the option in the benchmarking docs would be much more obvious to a not-very-experienced Rubyist who might not know about the $stdout workaround. And for people who use benchmarking a lot, the output stream option would be quite convenient. However, I do believe that your approach to use $stdout would be a big improvement.

@duerst
Copy link
Member

duerst commented Jan 31, 2021

The idea is that you could at least do $stdout = my_non_stdout_output; benchmark { ... }; $stdout = STDOUT in the (rare) case where you need redirecting it.

Now I understand how you were accommodating the other streams, by temporarily modifying $stdout. That would usually work, but the developer might have other output intended for the "real" standard output, using a dedicated benchmark.log file for all benchmarking output, for example. Supporting providing an output stream for the benchmark would enable this, and also be simpler for the caller.

99.99... of programs and programmers are used to the idea that there's stdin, stdout, stderr, and then other files. Having $stdout, STDOUT, my_stdout,... and so on differ may make the code a tiny bit shorter occasionally, but isn't worth breaking the model in most programmer's head.

@keithrbennett
Copy link
Contributor Author

@duerst Could you elaborate? I don't understand what you are advocating.

@duerst
Copy link
Member

duerst commented Feb 1, 2021

@keithrbennett I'm advocating the same thing as Marc-André. Sorry if that wasn't clear.

@keithrbennett
Copy link
Contributor Author

@marcandre @duerst Could you help me understand why you object to supporting an output stream parameter? I understand that you are pointing to saving and restoring $stdout as a workaround, but it seems really kludgy to me. $stdout would potentially have to be saved and restored several times if the caller wanted to log benchmarking information to a file, but send other program output to STDOUT. Imposing this burden on the developer to do this would be a disincentive to using the benchmarking functionality, which ideally should require as little modification to the measured code's environs as possible.

Also, is $stdout thread-global or global? If global, then changing its value could damage running code in other threads or ractors. (I'm assuming that ractors each run in their own thread; if not, then it may be even riskier. I'm also assuming that benchmarking can be run in any ractor.)

I think the code change would be pretty simple. Is it that you believe this feature would be used extremely rarely?

@marcandre
Copy link
Member

Is it that you believe this feature would be used extremely rarely?

Yes.

Also consider using benchmark-ips gem which has automatic persist to file results and is altogether more modern.

Or if you are testing complex systems (with other threads and Ractors that would output to stdout ??) consider using stackprof.

@marcandre
Copy link
Member

Closing; lib will now use $stdout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants