discourse / mini_mime

minimal mime type library
MIT License
95 stars 34 forks source link

Allow custom db paths #24

Closed gmcgibbon closed 5 years ago

gmcgibbon commented 5 years ago

Closes https://github.com/discourse/mini_mime/issues/23.

Adds MiniMime::Configuration for changing DB paths. This avoids having to instantiate multiple databases of the same kind and provides a simple API for switching out:

MiniMime.configure do |config|
  config.ext_db_path = "some_full_path"
  config.content_type_db_path = "some_other_full_path"
end

I'm happy to change/rewrite the implementation if need be.

Benchmark before:

Memory stats for requiring mime/types/columnar
Total allocated: 8964072 bytes (106373 objects)
Total retained:  3159081 bytes (33669 objects)

Memory stats for requiring mini_mime
Total allocated: 41136 bytes (362 objects)
Total retained:  7226 bytes (60 objects)
Warming up --------------------------------------
cached content_type lookup MiniMime
                        76.211k i/100ms
content_type lookup MIME::Types
                        14.888k i/100ms
Calculating -------------------------------------
cached content_type lookup MiniMime
                          1.015M (± 1.9%) i/s -      5.106M in   5.032007s
content_type lookup MIME::Types
                        158.069k (± 3.4%) i/s -    803.952k in   5.091533s
Warming up --------------------------------------
uncached content_type lookup MiniMime
                         1.510k i/100ms
content_type lookup MIME::Types
                        14.667k i/100ms
Calculating -------------------------------------
uncached content_type lookup MiniMime
                         15.343k (± 2.1%) i/s -     77.010k in   5.021662s
content_type lookup MIME::Types
                        156.226k (± 4.7%) i/s -    792.018k in   5.079955s

Benchmark after:

Memory stats for requiring mime/types/columnar
Total allocated: 8964072 bytes (106373 objects)
Total retained:  3159081 bytes (33669 objects)

Memory stats for requiring mini_mime
Total allocated: 44700 bytes (411 objects)
Total retained:  9096 bytes (73 objects)
Warming up --------------------------------------
cached content_type lookup MiniMime
                        74.313k i/100ms
content_type lookup MIME::Types
                        14.482k i/100ms
Calculating -------------------------------------
cached content_type lookup MiniMime
                        981.330k (± 2.3%) i/s -      4.905M in   5.000719s
content_type lookup MIME::Types
                        155.863k (± 3.4%) i/s -    782.028k in   5.022451s
Warming up --------------------------------------
uncached content_type lookup MiniMime
                         1.466k i/100ms
content_type lookup MIME::Types
                        13.281k i/100ms
Calculating -------------------------------------
uncached content_type lookup MiniMime
                         14.546k (± 9.1%) i/s -     73.300k in   5.093917s
content_type lookup MIME::Types
                        150.802k (± 5.2%) i/s -    757.017k in   5.033633s
SamSaffron commented 5 years ago

except for the "configure" api change looks fine to me, can you update the README with updated perf numbers and instructions on how to use the new custom db?

SamSaffron commented 5 years ago

Yeah I say we remove it

On Wed, 20 Feb 2019 at 8:48 am, Gannon McGibbon notifications@github.com wrote:

@gmcgibbon commented on this pull request.

In test/mini_mime_test.rb https://github.com/discourse/mini_mime/pull/24#discussion_r258244352:

@@ -10,6 +10,12 @@ def test_that_it_has_a_version_number

 refute_nil ::MiniMime::VERSION

end

  • def test_configure

  • MiniMime.configure do |config|

See https://thoughtbot.com/blog/mygem-configure-block. Its a configuration pattern I see a lot in ruby gems (1 https://github.com/carrierwaveuploader/carrierwave#configuring-carrierwave, 2 https://docs.bugsnag.com/platforms/ruby/rails/configuration-options/, 3 https://github.com/doorkeeper-gem/doorkeeper#api-mode). While it hides the detail of referencing MiniMime::Configuration, it's not exactly necessary. You could just do MiniMime::Configuration.var = value instead. Shall I 🔥 it?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/discourse/mini_mime/pull/24#discussion_r258244352, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAUXa0hBQIgrE5MlUo1R3nXUi1kfBiiks5vPHE7gaJpZM4bD59w .

gmcgibbon commented 5 years ago

Added docs, updated benchmark, and removed the configure method.

SamSaffron commented 5 years ago

looks good to me! merging, thanks