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

OptionParser: make flag addition code simpler and faster #3247

Merged
merged 1 commit into from
Sep 5, 2016

Conversation

grosser
Copy link
Contributor

@grosser grosser commented Sep 4, 2016

I'd guess I'm missing something, but this code is imo easier to read and a simple/naive benchmark shows it's 50x faster to do " " * x ... (5s vs 0.1s)

require "benchmark"
y = 0
z = Benchmark.realtime do
  1000000.times do
    x = " " * 100
    y += x.size
  end
end
puts y
puts z

vs

x = String.build do |str|
  100.times do
  str << " "
  end
str
end

@RX14
Copy link
Member

RX14 commented Sep 4, 2016

Can you use Benchmark.ips to test that whole method please?

@grosser
Copy link
Contributor Author

grosser commented Sep 4, 2016

unable to run ips :(

with bin/crystal I get:

./bin/crystal test.cr --release
Error in ./src/fiber.cr:26: this 'initialize' doesn't explicitly initialize instance variable '@stack_top' of Fiber, rendering it nilable

with latest crystal I get:

Empty enumerable (Enumerable::EmptyError)
[4312558] ???
[4312118] ???
[4308862] main +11918
[140672253235013] __libc_start_main +245
[4249449] ???
[0] ???

...

script is:

require "benchmark"

class Testeroo1
  def initialize
    @flags = [] of String
  end

  def append_flag(flag, description)
    @flags << "    #{flag}#{" " * (33 - flag.size)}#{description}"
  end
end

class Testeroo2
  def initialize
    @flags = [] of String
  end

  def append_flag(flag, description)
    @flags << String.build do |str|
      str << "    "
      str << flag
      (33 - flag.size).times do
        str << " "
      end
      str << description
     end
  end
end

Benchmark.ips(warmup: 4, calculation: 10) do |r|
  r.report("New") do
    test = Testeroo1.new
    5.times do
      test.append_flag "-f", "a test flag to figure out how fast things are"
    end
  end
  r.report("Old") do
    test = Testeroo2.new
    5.times do
      test.append_flag "-f", "a test flag to figure out how fast things are"
    end
  end
end

@RX14
Copy link
Member

RX14 commented Sep 4, 2016

@grosser please read the Benchmark.ips docs again, you are missing the syntax to define the tests.

@grosser
Copy link
Contributor Author

grosser commented Sep 4, 2016

updated script above ...

New 421.11k (± 8.33%)       fastest
Old  288.8k (± 5.66%)  1.46× slower

.. ideally ips should not fail with a cryptic error though :D

@asterite
Copy link
Member

asterite commented Sep 5, 2016

@grosser Thanks!

Regardless of the time it takes to perform it, I agree that it reduces the code a lot. And in this case the performance gain or loss in any case shouldn't be noticeable.

@asterite asterite added this to the 0.19.1 milestone Sep 5, 2016
@asterite asterite merged commit 9365b25 into crystal-lang:master Sep 5, 2016
@grosser grosser deleted the grosser/fast-string branch September 5, 2016 01:34
@grosser
Copy link
Contributor Author

grosser commented Sep 5, 2016

❤️ first language I could contribute too, thx for making it easy :D

@paulo-roberto-a
Copy link

congratulations @grosser

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.

None yet

4 participants