Feature #4197

Improvement of the benchmark library

Added by Benoit Daloze about 4 years ago. Updated over 3 years ago.

[ruby-core:33846]
Status:Closed
Priority:Normal
Assignee:Benoit Daloze

Description

=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

Merry Christmas !

Benoit Daloze
=end

Associated revisions

Revision 32269
Added by Yui NARUSE over 3 years ago

  • lib/benchmark.rb: merge eregon/benchmark.
    https://github.com/eregon/ruby/tree/benchmark
    patched by Benoit Daloze. [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

Revision 32269
Added by Yui NARUSE over 3 years ago

  • lib/benchmark.rb: merge eregon/benchmark.
    https://github.com/eregon/ruby/tree/benchmark
    patched by Benoit Daloze. [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

History

#1 Updated by Yui NARUSE about 4 years ago

  • Status changed from Open to Assigned
  • Assignee set to Koichi Sasada

=begin

=end

#2 Updated by Benoit Daloze about 4 years ago

=begin
Hi,

Any update on this ?

Could you have a look, Koichi ?

(Or anyone, I welcome comments)
=end

#3 Updated by Konstantin Haase about 4 years ago

=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

#4 Updated by Yui NARUSE about 4 years ago

  • Assignee changed from Koichi Sasada to 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

#5 Updated by Benoit Daloze about 4 years ago

=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 , 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

#6 Updated by Yui NARUSE about 4 years ago

  • 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
    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

#7 Updated by Motohiro KOSAKI about 4 years ago

=begin
Hi

  • lib/benchmark.rb: fix benchmarck to work with current ruby.
     patched by Benoit Daloze
     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 kosaki.motohiro@gmail.com
+
+ * 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 ryan@lust.local

     * 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 ' 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

#8 Updated by Benoit Daloze about 4 years ago

=begin
Hi,
On 1 February 2011 05:38, KOSAKI Motohiro kosaki.motohiro@gmail.com 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

#9 Updated by Motohiro KOSAKI about 4 years ago

=begin
2011/2/4 Benoit Daloze eregontp@gmail.com:

Hi,
On 1 February 2011 05:38, KOSAKI Motohiro kosaki.motohiro@gmail.com 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

#10 Updated by Tomoyuki Chikanaga about 4 years ago

=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

#11 Updated by Benoit Daloze about 4 years ago

=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

#12 Updated by Nobuyoshi Nakada about 4 years ago

=begin
Hi,

At Sun, 6 Feb 2011 03:38:48 +0900,
Benoit Daloze wrote in :

(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

#13 Updated by Tomoyuki Chikanaga about 4 years ago

=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

#14 Updated by Motohiro KOSAKI about 4 years ago

=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

#15 Updated by Benoit Daloze about 4 years ago

=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

#16 Updated by Yui NARUSE almost 4 years ago

=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

#17 Updated by Benoit Daloze almost 4 years ago

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.

#18 Updated by Yui NARUSE almost 4 years ago

  • Status changed from Closed to Assigned
  • Assignee changed from Yui NARUSE to Benoit Daloze

#19 Updated by Yui NARUSE over 3 years ago

  • 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. [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

Also available in: Atom PDF