Thank you for posting the interesting improvement.

Your patch is very fast, but I think an application should not have codes which rely on the internal structure of third-party libraries. Your patch accesses mini_mime's instance variable '@db'.

I updated your patch not to manipulate an internal variable of mini_mime. It is 15% slower than your patch but 70% faster than the current @Redmine::MimeType.of@.

without patches:
<pre>
known 226.301k (± 5.0%) i/s - 1.131M in 5.010006s
unknown 225.491k (± 4.6%) i/s - 1.131M in 5.024550s
</pre>

replace-mime-types-with-mini_mime.diff: (faster):
<pre>
known 382.020k (± 6.2%) i/s - 1.923M in 5.052268s
unknown 382.602k (± 6.6%) i/s - 1.908M in 5.008955s
</pre>

mime_type.rb.patch by Pavel Rosický (fastest):
<pre>
known 431.349k (± 5.4%) i/s - 2.167M in 5.038918s
unknown 432.307k (± 5.5%) i/s - 2.172M in 5.040046s
</pre>

--------------------------------------------------------------------------------
Sorry, there was a problem with my benchmark script. After fixing the problem, I found that my patch is very slow for unknown extensions.

without patches:
<pre>
known 170.009k (± 7.2%) i/s - 859.560k in 5.083952s
unknown 169.572k (± 6.2%) i/s - 844.816k in 5.000900s
</pre>

replace-mime-types-with-mini_mime.diff:
<pre>
known 244.827k (± 5.3%) i/s - 1.228M in 5.031343s
unknown 79.055k (± 5.5%) i/s - 394.128k in 5.000338s
</pre>

mime_type.rb.patch by Pavel Rosický:
<pre>
known 256.660k (± 5.7%) i/s - 1.291M in 5.047902s
unknown 150.151k (± 5.5%) i/s - 753.483k in 5.033494s
</pre>

<pre><code class="ruby">
require 'benchmark/ips'
require 'mini_mime'
require './lib/redmine/mime_type'

Benchmark.ips do |x|
x.report('known') { Redmine::MimeType.of('file.txt') }
x.report('unknown') { Redmine::MimeType.of('file.zip') }
end
</code></pre>
--------------------------------------------------------------------------------
I updated my patch to preserve existing cache mechanism. Now it is fast also for unknown extensions.

<pre>
known 232.028k (± 4.9%) i/s - 1.170M in 5.052806s
unknown 236.938k (± 4.6%) i/s - 1.199M in 5.071712s
</pre>
--------------------------------------------------------------------------------
Lets ask sam saffron if he could add it to the public api.
--------------------------------------------------------------------------------
@known_types is a class variable. There're two problems:
It isn't threadsafe.
It's unlimited and because it's related to user's actions it could grow indefinetly. This memory won't be released. That's why I was trying to avoid it or limit it. Minimime has the upper bound for caches. Please correct me if I'm wrong.
--------------------------------------------------------------------------------
Pavel Rosický wrote:
> @known_types is a class variable. There're two problems:
> It isn't threadsafe.
> It's unlimited and because it's related to user's actions it could grow indefinetly. This memory won't be released.

Although the patch still has the same problem with the current implementation, simply replacing mime-types with mini_mime will reduce memory usage and improve performance.

I suggest that we discuss the problem as a new issue, after committing the patch. Is it OK with you?
--------------------------------------------------------------------------------
It's ok, thanks
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Committed. Thank you for your contribution.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
please also remove
<pre>
require 'mime/types'
</pre>
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Pavel Rosický wrote:
> please also remove
> [...]

Committed. Thanks.
--------------------------------------------------------------------------------
mini_mime 1.0.1 supports @lookup_by_extension@. We can remove the cache mechanism using @known_types@ hash by making use of the new method. Attaching a patch to remove the hash.

Even without the cache mechanism, @Redmine::MimeTypes.of@ is faster than the original code.

r17467 (before applying replace-mime-types-with-mini_mime-v3.diff):
<pre>
Warming up --------------------------------------
known 20.028k i/100ms
unknown 12.677k i/100ms
Calculating -------------------------------------
known 246.855k (± 6.0%) i/s - 1.242M in 5.049235s
unknown 138.044k (± 5.9%) i/s - 697.235k in 5.068005s
</pre>

with the patch 29359-remove-known-types-hash.patch:
<pre>
Warming up --------------------------------------
known 20.249k i/100ms
unknown 11.787k i/100ms
Calculating -------------------------------------
known 264.152k (± 8.0%) i/s - 1.316M in 5.015411s
unknown 142.650k (± 8.6%) i/s - 719.007k in 5.076025s
</pre>
--------------------------------------------------------------------------------
29359-remove-known-types-hash.patch - .downcase shouldn't be removed, both EXTENSIONS[] and lookup_by_extension expect lower-case

and the benchmark looks odd, in r17467 both known/unknown scenarios were cached in a class var, so I would expect simmilar performance
--------------------------------------------------------------------------------
Pavel Rosický wrote:
> 29359-remove-known-types-hash.patch - .downcase shouldn't be removed, both EXTENSIONS[] and lookup_by_extension expect lower-case

Fixed.

> and the benchmark looks odd, in r17467 both known/unknown scenarios were cached in a class var, so I would expect simmilar performance

You are right. Maybe I pasted a wrong result. The following results are correct ones. @Redmine::MimeType.of@ will be faster than r17467 in many cases.

r17467 (before switching to mini_mime):
<pre>
found in array (file.patch)
164.476k (± 6.5%) i/s - 832.300k in 5.081748s
found in gem (file.zip)
167.251k (± 5.7%) i/s - 841.580k in 5.047872s
not found in gem (file.go)
168.700k (± 5.7%) i/s - 851.440k in 5.063132s
</pre>

with the patch:
<pre>
found in array (file.patch)
418.181k (± 6.0%) i/s - 2.099M in 5.036624s
found in gem (file.zip)
183.820k (± 6.9%) i/s - 923.232k in 5.047995s
not found in gem (file.go)
147.241k (± 6.7%) i/s - 734.046k in 5.007225s
</pre>

benchmark script:
<pre><code class="ruby">
require 'benchmark/ips'
require 'mini_mime'
require './lib/redmine/mime_type'

Benchmark.ips do |x|
x.report('found in array (file.patch)') { Redmine::MimeType.of('file.patch') }
x.report('found in gem (file.zip)') { Redmine::MimeType.of('file.zip') }
x.report('not found in gem (file.go)') { Redmine::MimeType.of('file.go') }
end
</code></pre>
--------------------------------------------------------------------------------
my results (ruby 2.5.0):

with the patch (mini_mime)
<pre>
found in array (file.patch)
1.079M (± 1.9%) i/s - 5.407M in 5.011560s
found in gem (file.zip)
486.991k (± 2.6%) i/s - 2.456M in 5.047320s
not found in gem (file.go)
428.810k (± 2.3%) i/s - 2.174M in 5.072125s
</pre>

r17467 (mime_types)
<pre>
found in array (file.patch)
293.348k (± 1.6%) i/s - 1.477M in 5.035304s
found in gem (file.zip)
295.426k (± 1.8%) i/s - 1.500M in 5.080399s
not found in gem (file.go)
293.534k (± 5.3%) i/s - 1.477M in 5.048996s
</pre>

r17467 (mime_types without @known_types cache)
<pre>
found in array (file.patch)
298.984k (± 5.2%) i/s - 1.499M in 5.027456s
found in gem (file.zip)
60.804k (± 1.6%) i/s - 304.803k in 5.014262s
not found in gem (file.go)
65.405k (± 2.9%) i/s - 332.575k in 5.089481s
</pre>
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Committed 29359-remove-known-types-hash-v2.patch.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

relates,Closed,29365,MailHandlerTest#test_add_issue_with_localized_attributes fails with mail gem 2.7.0
relates,Closed,29383,Update mini_mime gem (~> 1.0.1)
relates,Closed,29443,Update mail gem (~> 2.7.1)