Patch #29359

Switch to mini_mime from mime-types

Added by Pavel Rosický 3 months ago. Updated 3 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Go MAEDA% Done:

0%

Category:Performance
Target version:4.0.0

Description

Redmine uses unbound cache for mime types which is eliminated now. Also mime-types is too heavy and Redmine uses only a tiny peace of it, we just need a simple ext -> mime lookup.

https://github.com/mikel/mail/pull/1059 (mail 2.6.x still uses mime-types)
https://github.com/teamcapybara/capybara/pull/1884

require 'benchmark/ips'

Benchmark.ips do |x|
  x.report('known')     { Redmine::MimeType.of("file.patch") }
  x.report('not known') { Redmine::MimeType.of("file.zip")   }
end
trunk
               known    284.431k (± 1.9%) i/s -      1.422M in   5.002378s
           not known    285.699k (± 3.0%) i/s -      1.446M in   5.065113s
patch
               known    657.114k (± 1.8%) i/s -      3.327M in   5.064494s
           not known    392.574k (± 2.2%) i/s -      1.965M in   5.006827s
                boot allocations boot retained
mime-types                109796         31165
mini_mime                    398            62

what do you think?

Gemfile.patch Magnifier (381 Bytes) Pavel Rosický, 2018-08-10 19:48

mime_type.rb.patch Magnifier (1.43 KB) Pavel Rosický, 2018-08-10 21:42

replace-mime-types-with-mini_mime.diff Magnifier (1.38 KB) Go MAEDA, 2018-08-11 08:01

replace-mime-types-with-mini_mime-v2.diff Magnifier (1.17 KB) Go MAEDA, 2018-08-11 09:26

replace-mime-types-with-mini_mime-v3.diff Magnifier (1.14 KB) Go MAEDA, 2018-08-12 11:43

29359-remove-known-types-hash.patch Magnifier (1.13 KB) Go MAEDA, 2018-08-14 12:04

29359-remove-known-types-hash-v2.patch Magnifier (1.14 KB) Go MAEDA, 2018-08-14 17:16


Related issues

Related to Redmine - Defect #29365: MailHandlerTest#test_add_issue_with_localized_attributes ... Closed
Related to Redmine - Patch #29383: Update mini_mime gem (~> 1.0.1) Closed
Related to Redmine - Feature #29443: Update mail gem (~> 2.7.1) Closed

Associated revisions

Revision 17468
Added by Go MAEDA 3 months ago

Replace mime-types gem with more efficient mini_mime gem (#29359).

Contributed by Pavel Rosický.

Revision 17470
Added by Go MAEDA 3 months ago

Don't require mime-types (#29359).

Revision 17471
Added by Go MAEDA 3 months ago

Test that Redmine::MimeType.of is case-insensitive (#29359).

Revision 17472
Added by Go MAEDA 3 months ago

Test that Redmine::MimeType.css_class_of, main_mimetype_of and is_type? are case-insensitive (#29359).

Revision 17473
Added by Go MAEDA 3 months ago

Refactor Redmine::MimeType.of. Uses MiniMime.lookup_by_extension instead of lookup_by_filename (#29359).

History

#1 Updated by Go MAEDA 3 months ago

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:

               known    226.301k (± 5.0%) i/s -      1.131M in   5.010006s
             unknown    225.491k (± 4.6%) i/s -      1.131M in   5.024550s

replace-mime-types-with-mini_mime.diff: (faster):

               known    382.020k (± 6.2%) i/s -      1.923M in   5.052268s
             unknown    382.602k (± 6.6%) i/s -      1.908M in   5.008955s

mime_type.rb.patch by Pavel Rosický (fastest):

               known    431.349k (± 5.4%) i/s -      2.167M in   5.038918s
             unknown    432.307k (± 5.5%) i/s -      2.172M in   5.040046s

#2 Updated by Go MAEDA 3 months ago

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:

               known    170.009k (± 7.2%) i/s -    859.560k in   5.083952s
             unknown    169.572k (± 6.2%) i/s -    844.816k in   5.000900s

replace-mime-types-with-mini_mime.diff:

               known    244.827k (± 5.3%) i/s -      1.228M in   5.031343s
             unknown     79.055k (± 5.5%) i/s -    394.128k in   5.000338s

mime_type.rb.patch by Pavel Rosický:

               known    256.660k (± 5.7%) i/s -      1.291M in   5.047902s
             unknown    150.151k (± 5.5%) i/s -    753.483k in   5.033494s

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

#3 Updated by Go MAEDA 3 months ago

I updated my patch to preserve existing cache mechanism. Now it is fast also for unknown extensions.

               known    232.028k (± 4.9%) i/s -      1.170M in   5.052806s
             unknown    236.938k (± 4.6%) i/s -      1.199M in   5.071712s

#4 Updated by Pavel Rosický 3 months ago

Lets ask sam saffron if he could add it to the public api.

#5 Updated by Pavel Rosický 3 months ago

@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.

#6 Updated by Go MAEDA 3 months ago

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?

#7 Updated by Pavel Rosický 3 months ago

It's ok, thanks

#8 Updated by Go MAEDA 3 months ago

  • Subject changed from Switch to mini_mime to Switch to mini_mime from mime-types
  • Target version set to 4.1.0

#10 Updated by Go MAEDA 3 months ago

  • Status changed from New to Closed
  • Assignee set to Go MAEDA

Committed. Thank you for your contribution.

#11 Updated by Go MAEDA 3 months ago

  • Target version changed from 4.1.0 to 4.0.0

#12 Updated by Go MAEDA 3 months ago

  • Related to Defect #29365: MailHandlerTest#test_add_issue_with_localized_attributes fails with mail gem 2.7.0 added

#13 Updated by Pavel Rosický 3 months ago

please also remove

 require 'mime/types'

#14 Updated by Marius BALTEANU 3 months ago

  • Status changed from Closed to Reopened

#15 Updated by Go MAEDA 3 months ago

  • Status changed from Reopened to Closed

Pavel Rosický wrote:

please also remove
[...]

Committed. Thanks.

#16 Updated by Go MAEDA 3 months ago

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):

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

with the patch 29359-remove-known-types-hash.patch:

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

#17 Updated by Pavel Rosický 3 months ago

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

#18 Updated by Go MAEDA 3 months ago

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):

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

with the patch:

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

benchmark script:

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

#19 Updated by Pavel Rosický 3 months ago

my results (ruby 2.5.0):

with the patch (mini_mime)

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

r17467 (mime_types)

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

r17467 (mime_types without @known_types cache)

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

#20 Updated by Go MAEDA 3 months ago

  • Related to Patch #29383: Update mini_mime gem (~> 1.0.1) added

#21 Updated by Go MAEDA 3 months ago

  • Status changed from Reopened to Closed

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

#22 Updated by Go MAEDA 3 months ago

Also available in: Atom PDF