Feature #4197: Improvement of the benchmark library

• Status: Closed
• Assignee: Eregon (Benoit Daloze)
• Target version: 2.0.0

[Eregon (Benoit Daloze)]

=begin
Hello Rubyists !

The Benchmark library's code is now becoming a bit old, and I would like to keep it up to date.

http://redmine.ruby-lang.org/wiki/ruby/Maintainers says there is no maintainer for lib/benchmark.
However, lib/benchmark.rb says it was created by Gotoken, and he made a commit on Feb 19, 2009.
The library has not been touched from 2004 (2002 for the code base), except for minor fixes.

I made several commits to clean the code, test it, watch it fail, and fix 2 minor bugs.
The commits are small, so feel free to discuss them separately.

You can see the commits here:
https://github.com/eregon/ruby/commits/benchmark

I did not modify the API, except Benchmark#benchmark's first parameter,
which do not need anymore to be manually padded with spaces:
Benchmark.benchmark(" "*7 + CAPTION, 7, ...
become
Benchmark.benchmark(CAPTION, 7, ...

(and Benchmark#bm now returns an Array of Benchmark::Tms times instead of STDOUT.sync)

I would like to improve Benchmark#bm, to not have to give the maximum label width, and let the library calculate it.
This will be part of another Feature request, to focus on cleaning code in this one.

This is my first direct contribution to Ruby, so please be indulgent.

If you are interested in some context about myself, please see [ruby-core:33845]

Merry Christmas !

Benoit Daloze
=end

[naruse (Yui NARUSE)]

• Status changed from Open to Assigned
• Assignee set to ko1 (Koichi Sasada)

=begin

=end

[Eregon (Benoit Daloze)]

=begin
Hi,

Any update on this ?

Could you have a look, Koichi ?

(Or anyone, I welcome comments)
=end

[rkh (Konstantin Haase)]

=begin
This eases review: https://github.com/eregon/ruby/compare/trunk...benchmark

Konstantin

On Jan 25, 2011, at 13:14 , Benoit Daloze wrote:

Issue #4197 has been updated by Benoit Daloze.

Hi,

Any update on this ?

Could you have a look, Koichi ?

(Or anyone, I welcome comments)

http://redmine.ruby-lang.org/issues/show/4197

---

http://redmine.ruby-lang.org

=end

[naruse (Yui NARUSE)]

• Assignee changed from ko1 (Koichi Sasada) to naruse (Yui NARUSE)

=begin
Koichi said the lib is not his, so I reviewed it.

I almost accept your patch except following:

4291a26b94a63e3066fe

Tests for MRI should use test/unit.
See also test files in test directory.

f20763674b732b2734f9

This introduces API change.
Yeah, the change won't be a problem, but the rdoc must describe about its return value.
attr_reader :list also should have the rdoc.
=end

[Eregon (Benoit Daloze)]

=begin
Hi,

Koichi said the lib is not his, so I reviewed it.

Thank you.

I almost accept your patch except following:

4291a26b94a63e3066fe

Tests for MRI should use test/unit.

I thought minitest/spec were fine, as it is in the standard library (and the fact test/unit is now a compatibility layer using minitest/unit).
fiddle, minitest, net/smtp, psych, rdoc and rubygems use minitest (according to $ grep -r minitest test).
But only minitest/spec's tests use minitest/spec.

I also understood in [ruby-core:32280], that minitest/spec was accepted (but not rspec/mspec of course, because they are not bundled).

I can change to (mini)test/unit if you want.

See also test files in test directory.

Do you mean another file than test/benchmark/test_benchmark.rb ?
I included that test in mine, as I thought it would be better to merge all tests together.
(The code is not so similar as there is no assert_nothing_raised in minitest)

f20763674b732b2734f9

This introduces API change.
Yeah, the change won't be a problem, but the rdoc must describe about its return value.
attr_reader :list also should have the rdoc.

Sorry, I forgot about it.
It is fixed in 227034b9 lib/benchmark: document the return value of #benchmark and the :list attribute in Report

P.S.:
Git sha are not so handy to describe a commit, maybe the description is better (especially if the history is rewritten).

May I assign to you further benchmark issues ?
=end

[naruse (Yui NARUSE)]

• Status changed from Assigned to Closed
• % Done changed from 0 to 100

=begin
This issue was solved with changeset r30747.
Benoit, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

• lib/benchmark.rb: fix benchmarck to work with current ruby.
  patched by Benoit Daloze [ruby-core:33846] [ruby-dev:43143]
  merged from https://github.com/eregon/ruby/commits/benchmark

• lib/benchmark (Report#width): update documentation

• lib/benchmark: document the return value of #benchmark and the
  :list attribute in Report

• lib/benchmark (Tms#format): rename variables, use String#%
  instead of Kernel.format

• lib/benchmark: remove undocumented Benchmark::times (an alias
  of Process::times used twice)

• lib/benchmark (#benchmark): use label_width for the caption

• lib/benchmark (Tms#initialize): rename variables

• lib/benchmark: allow title to not be a String and call #to_s

• lib/benchmark (Benchmark#bm): return an Array of the times with
  the labels

• lib/benchmark: correct output for Benchmark#bmbm
  (remove the extra space)

• lib/benchmark: add a few tests for Benchmark::Tms output

• lib/benchmark: improve style (enumerators, ljust, unused vars)

• lib/benchmark: add spec about output and return value

• lib/benchmark: improve basic style and consistency
  no parenthesis for print and use interpolation instead of printf

• lib/benchmark: remove unnecessary conversions and variables

• lib/benchmark: correct indentation

• lib/benchmark: rename the FMTSTR constant and variable to FORMAT

• lib/benchmark: remove useless exception

• test/benchmark: remove unused variable warnings
  =end

[kosaki (Motohiro KOSAKI)]

=begin
Hi

• lib/benchmark.rb: fix benchmarck to work with current ruby.
   patched by Benoit Daloze [ruby-core:33846] [ruby-dev:43143]
   merged from https://github.com/eregon/ruby/commits/benchmark

•  lib/benchmark (Report#width): update documentation

•  lib/benchmark: document the return value of #benchmark and the
    :list attribute in Report

•  lib/benchmark (Tms#format): rename variables, use String#%
    instead of Kernel.format

•  lib/benchmark: remove undocumented Benchmark::times (an alias
    of Process::times used twice)

•  lib/benchmark (#benchmark): use label_width for the caption

•  lib/benchmark (Tms#initialize): rename variables

•  lib/benchmark: allow title to not be a String and call #to_s

•  lib/benchmark (Benchmark#bm): return an Array of the times with
    the labels

•  lib/benchmark: correct output for Benchmark#bmbm
    (remove the extra space)

•  lib/benchmark: add a few tests for Benchmark::Tms output

•  lib/benchmark: improve style (enumerators, ljust, unused vars)

•  lib/benchmark: add spec about output and return value

•  lib/benchmark: improve basic style and consistency
    no parenthesis for print and use interpolation instead of printf

•  lib/benchmark: remove unnecessary conversions and variables

•  lib/benchmark: correct indentation

•  lib/benchmark: rename the FMTSTR constant and variable to FORMAT

•  lib/benchmark: remove useless exception

•  test/benchmark: remove unused variable warnings

The change log says, test/benchmark has been changed only warnings issue
by this commit. However it has more widely change and it made a false positive
test failure on windows.

1. Failure:
   test_0001__ruby_dev_40906_can_add_in_place_the_time_of_execution_of_the_block_gi
   ven(Benchmark::Bugs) [C:/ruby/trunk/test/benchmark/test_benchmark.rb:127]:
   Expected 0.0 to not be equal to 0.

Therefore, I commited following additional patch.
Thanks.

===================================================================
--- ChangeLog (revision 30752)
+++ ChangeLog (working copy)
@@ -1,3 +1,10 @@
+Tue Feb 1 13:20:39 2011 KOSAKI Motohiro [e-mail]
+

•   * test/benchmark/test_benchmark.rb (#capture_bench_output):
  

•     Added explict sleep. Windows have imprecise time support.
  

•     Thus Tms.new.Add!{} may be or may be not equal 0.
  

•     The test failure started since r30747.
  

Tue Feb 1 11:03:47 2011 Ryan Davis [e-mail]

     * lib/rubygems*: Import rubygems 1.5.0 (released version @ 1fb59d0)

Index: test/benchmark/test_benchmark.rb

--- test/benchmark/test_benchmark.rb (revision 30752)
+++ test/benchmark/test_benchmark.rb (working copy)
@@ -123,7 +123,7 @@
it '[ruby-dev:40906] can add in-place the time of execution of the block gi
ven' do
t = Benchmark::Tms.new
t.real.must_equal 0

•  t.add! {}
  

•  t.add! { sleep 0.1 }
   t.real.wont_equal 0
  

  end
  end

=end

[Eregon (Benoit Daloze)]

=begin
Hi,
On 1 February 2011 05:38, KOSAKI Motohiro [e-mail] wrote:

Hi

The change log says, test/benchmark has been changed only warnings issue
by this commit. However it has more widely change and it made a false positive
test failure on windows.

Sorry, I forgot about windows' imprecision.

I intended to do a sleep(), but did not want to slow down the test and
thought just running the block was enough.

Thanks for the patch.

=end

[kosaki (Motohiro KOSAKI)]

=begin
2011/2/4 Benoit Daloze [e-mail]:

Hi,
On 1 February 2011 05:38, KOSAKI Motohiro [e-mail] wrote:

Hi

The change log says, test/benchmark has been changed only warnings issue
by this commit. However it has more widely change and it made a false positive
test failure on windows.

Sorry, I forgot about windows' imprecision.

I intended to do a sleep(), but did not want to slow down the test and
thought just running the block was enough.

Thanks for the patch.

No problem. :)
At minimum, your code is very clean and good readable. therefore I could find
the test failure reason and fix it quickly.

Thank you too.

=end

[nagachika (Tomoyuki Chikanaga)]

=begin
Hi,
In Benchmark#bmbm, I think it's better to use ensure clause instead of Object#tap to restore STDOUT.sync. How about a patch below?

diff --git a/lib/benchmark.rb b/lib/benchmark.rb
index 052b9ad..ac17ba4 100644
--- a/lib/benchmark.rb
+++ b/lib/benchmark.rb
@@ -266,9 +266,9 @@ module Benchmark
GC.start
print label.ljust(width)
Benchmark.measure(&item).tap { |res| print res.format }

• }.tap {

•  STDOUT.sync = sync
  

  }

• ensure
• STDOUT.sync = sync unless sync.nil?
  end

#

=end

[Eregon (Benoit Daloze)]

=begin
Hi,
On 5 February 2011 16:08, Tomoyuki Chikanaga wrote:

Hi,
In Benchmark#bmbm, I think it's better to use ensure clause instead of Object#tap to restore STDOUT.sync.

Do you have a particular scenario in mind ?
(If it is an Interrupt, I think restoring STDOUT's sync is not relevant.)

#tap is used to return the list of the Benchmark::Tms.
So, your patch would need to store that list in a variable, and return it.

Also, sync would never be nil without ensure.
This extra check shows "ensure" is complicating things in this case.
So, I think it is overkill to use ensure here.

On IO#sync's subject, I thought once it would be nice to have a block form of IO#sync, which could use ensure.

Anyway, I am going to propose to move that print behavior in Report/Job, and I will probably use IO#flush, to make things simpler.
=end

[nobu (Nobuyoshi Nakada)]

=begin
Hi,

At Sun, 6 Feb 2011 03:38:48 +0900,
Benoit Daloze wrote in [ruby-core:35103]:

(If it is an Interrupt, I think restoring STDOUT's sync is not relevant.)

A library should not leave such globally shared states changed.

#tap is used to return the list of the Benchmark::Tms.
So, your patch would need to store that list in a variable, and return it.

`ensure' clause does not affect the return value.
So tap trick is not needed here.

Also, sync would never be nil without ensure.
This extra check shows "ensure" is complicating things in this case.
So, I think it is overkill to use ensure here.

Consider the case interrupted during `yield'.

On IO#sync's subject, I thought once it would be nice to have a block form of IO#sync, which could use ensure.

It's another story, please file new ticket.

--
Nobu Nakada

=end

[nagachika (Tomoyuki Chikanaga)]

=begin
Hi,

Well, Nakada-san already said almost all my points.

Anyway, I am going to propose to move that print behavior in Report/Job, and I will probably use IO#flush, to make things simpler.
I see. It's better than change STDOUT.sync because of less side effects.
I withdraw my previous patch.
thanks.
=end

[kosaki (Motohiro KOSAKI)]

=begin

Hi,

Well, Nakada-san already said almost all my points.

Anyway, I am going to propose to move that print behavior in Report/Job, and I will probably use IO#flush, to make things simpler.
I see. It's better than change STDOUT.sync because of less side effects.
I withdraw my previous patch.
thanks.

I agree.
Anyway, your patch is nicer than current. I hope you will commit it.

=end

[Eregon (Benoit Daloze)]

=begin
Hi,
On 6 February 2011 03:07, Nobuyoshi Nakada wrote:

Hi,

A library should not leave such globally shared states changed.

I know, that is why I prefer to use IO#flush in libraries.

`ensure' clause does not affect the return value.
So tap trick is not needed here.

My mistake, I did not know `ensure' clause preserve the return value.
That's definitely handful.

I guess then the patch is fine.

Tomoyuki: Please commit it (if you want), that will clean that part until I work on it.

On IO#sync's subject, I thought once it would be nice to have a block form of IO#sync, which could use ensure.

It's another story, please file new ticket.

I now think it is not a good idea, giving it is probably best not to use IO#sync in a library.
=end

[naruse (Yui NARUSE)]

=begin
Hi, Benoit.
How about following skipped test?

it 'correctly guess the label width even when not given' do
  skip :not_implemented
  capture_bench_output(:bm).must_equal BM_OUTPUT
end

=end

[Eregon (Benoit Daloze)]

Hello,

I said I would soon make it pass, but I unfortunately did not take the time yet to report the feature, I'm sorry.

However, the code is already there, I just need to split it correctly into commits.
Also, I believe my code should be discussed because there are alternatives.

If that skip is bothering you, please feel free to comment or even remove that test.
I'll try to report the feature this week.

[naruse (Yui NARUSE)]

• Status changed from Closed to Assigned
• Assignee changed from naruse (Yui NARUSE) to Eregon (Benoit Daloze)

[naruse (Yui NARUSE)]

• Status changed from Assigned to Closed

This issue was solved with changeset r32269.
Benoit, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

• lib/benchmark.rb: merge eregon/benchmark.
  https://github.com/eregon/ruby/tree/benchmark
  patched by Benoit Daloze. [ruby-core:37593] [Bug #4940]

• lib/benchmark (Benchmark#bmbm): bmbm should be consistent with bm
  for the return value.

• test/benchmark: remove preemptive test instead of skipping
  I removed the preemptive test I wrote for Feature #4197.
  I'll add it back when the implementation will be able to satisfy it.

• lib/benchmark (Benchmark#bmbm): remove useless explicit call,
  #format is an alias of #to_s test/benchmark: add a test for
  format of long time.

• lib/benchmark: fix label width: always add 1 to ensure there is a
  space delimiter even with times over 100s
  When I asked for Feature #4197, I wanted to make delimiting spaces
  consistent for #bm and #bmbm.
  But with times over 100s, the output contains no space between the
  label and the first time (user).
  Now both ensure there is always a space, even if that means 3 spaces
  with times under 10s (because it is formatted as %10.6f)

• test/benchmark: let labels be a constant
  lib/benchmark (Benchmark#realtime): avoid creating an unused Proc
  lib/benchmark (Benchmark#benchmark): use ensure clause to restore
  STDOUT.sync, as in #bmbm