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

Basic project configuration engine #1909

Merged
merged 3 commits into from
Oct 4, 2015
Merged

Basic project configuration engine #1909

merged 3 commits into from
Oct 4, 2015

Conversation

ujifgc
Copy link
Member

@ujifgc ujifgc commented Apr 23, 2015

ref #1832

I retracted from the idea of configuring Padrino modules by calling their fields as it would overly complicate the implementation and repeat the existing fine gems like https://github.com/beatrichartz/configurations and https://github.com/markbates/configatron.

The configuration now only supports very basic OpenStruct syntax with an environment filter. Just Ruby, no magic, no DSL:

Padrino.config.value1 = 42

Padrino.configure :development do |config|
  config.value2 = 'only development'
end

Padrino.configure :development, :production do |config|
  config.value2 = 'both development and production'
end

Padrino.configure do |config|
  config.value2 = 'any environment'
end

I think this implementation covers the basic needs of configuring a project while not bloating it and leaving space for more comprehensive solutions.

@sschepens
Copy link

OpenStruct is much slower than Struct, i've seen lots of benchmarks, and seen some gems that provide a similar interface for a Hash and are faster than OpenStruct.
I believe it would be greate if we could define previously which configuration parameters are supported and create a Struct instead of OpenStruct, if not maybe analyze alternatives.

Just some thoughts,
Cheers

@ujifgc
Copy link
Member Author

ujifgc commented Apr 24, 2015

Thank you for your concern, it's always pleasant to see people care about speed. I modified one of the benchmarks you are talking about to test out configuration use case. As you can see, the problem is not very drastic if you don't create OpenStructs from hash in a loop.

$ ruby -v
ruby 2.1.5p273 (2014-11-13) [x86_64-linux-gnu]
$ ruby ~/rb/bmL.rb
                           user     system      total        real
OpenStruct slow        2.140000   0.000000   2.140000 (  2.136333)
OpenStruct fast        2.050000   0.000000   2.050000 (  2.050478)
OpenStruct blazing     0.050000   0.000000   0.050000 (  0.053958)
OpenStruct usage       0.030000   0.000000   0.030000 (  0.027085)
Struct slow            0.060000   0.000000   0.060000 (  0.058391)
Struct fast            0.040000   0.000000   0.040000 (  0.044056)
Struct blazing         0.050000   0.000000   0.050000 (  0.042647)
Struct usage           0.010000   0.000000   0.010000 (  0.012637)
require 'benchmark'
require 'ostruct'

REP = 200000

User = Struct.new(:name, :age)

USER = "User".freeze
AGE = 21
HASH = {:name => USER, :age => AGE}.freeze

Benchmark.bm 20 do |x|
  x.report 'OpenStruct slow' do
    REP.times do |index|
       OpenStruct.new(:name => "User", :age => 21)
    end
  end

  x.report 'OpenStruct fast' do
    REP.times do |index|
       OpenStruct.new(HASH)
    end
  end

  x.report 'OpenStruct blazing' do
    REP.times do |index|
       OpenStruct.new
    end
  end

  x.report 'OpenStruct usage' do
    o = OpenStruct.new
    o.data = 'test'
    REP.times do |index|
       o.data
    end
  end

  x.report 'Struct slow' do
    REP.times do |index|
       User.new("User", 21)
    end
  end

  x.report 'Struct fast' do
    REP.times do |index|
       User.new(USER, AGE)
    end
  end

  x.report 'Struct blazing' do
    REP.times do |index|
       User.new
    end
  end

  x.report 'Struct usage' do
    Tester = Struct.new(:data)
    s = Tester.new
    s.data = 'test'
    REP.times do |index|
       s.data
    end
  end
end

@sschepens
Copy link

What you're mostly testing is read time, a struct would mostly benefit from initialization time, a benchmark like this:

require 'benchmark'
require 'ostruct'

REP = 200000

User = Struct.new(:name, :age)

Benchmark.bm 20 do |x|
  x.report 'OpenStruct initialization' do
    REP.times do
       o = OpenStruct.new
       20.times do |index|
          o[index.to_s.to_sym] = "test"
       end
    end
  end
  x.report 'Struct initialization' do
    names = 20.times.collect{|i| i.to_s.to_sym}
    Tester = Struct.new(*names)
    REP.times do
       s = Tester.new
       20.times do |index|
         s[index.to_s.to_sym] = "test"
       end
    end
  end
end

Gives this results:

                           user     system      total        real
OpenStruct initialization 30.940000   0.080000  31.020000 ( 31.462440)
Struct initialization  3.020000   0.000000   3.020000 (  3.059638)

Which shows that an OpenStruct is 10 times slower at initializing a 20 keys, also, from your tests reads take 3 times more from an OpenStruct.

But of course to be able to use a Struct you have to know all keys beforehand 😄
I'm not against OpenStruct, just suggesting if possible not to use it, after all, we're talking about a few milliseconds more of app start time.

Cheers! 🍻

@ujifgc
Copy link
Member Author

ujifgc commented Apr 24, 2015

I'm not arguing on OpenStruct speed. I'm showing you a benchmark of the use case of Padrino configuration. Do you actually think Padrino project would initialize tons of it's configuration keys?

@sschepens
Copy link

@ujifgc of course not, maybe i didn't express myself correctly, i don't think using any OpenStruct would make such a big difference, i don't believe tons of configuration keys will be written or read.
I just suggested to see if it's viable to not use an OpenStruct

@ujifgc
Copy link
Member Author

ujifgc commented Apr 25, 2015

I fail to see any reason to not use OpenStruct. I think it's very convenient and easy. Using Struct would limit the usage of configuration storage and provide no real benefits.

@nesquena
Copy link
Member

Might be a stupid questions but how does this differ from sinatra's configuration itself: http://www.sinatrarb.com/intro.html#Configuration itself? is the primary difference the assignment syntax?

@ujifgc
Copy link
Member Author

ujifgc commented Apr 27, 2015

Sinatra configuration is application-specific. This PR allows to configure your project and potentially Padrino modules.

@ujifgc ujifgc closed this Apr 28, 2015
@namusyaka
Copy link
Contributor

Why did you close this PR? This essence seems reasonable, though the configuration style can be discussed.

@ujifgc ujifgc reopened this Apr 28, 2015
@ujifgc
Copy link
Member Author

ujifgc commented Apr 28, 2015

I came to conclusion that it's lame and can be avoided.

Reopened for discussion.

@ujifgc ujifgc mentioned this pull request Sep 29, 2015
@namusyaka
Copy link
Contributor

Looks good.

namusyaka added a commit that referenced this pull request Oct 4, 2015
Basic project configuration engine
@namusyaka namusyaka merged commit af1eff0 into master Oct 4, 2015
@ujifgc ujifgc deleted the config branch October 26, 2015 15:33
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.

4 participants