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

autoload parsers for load-time speed and memory usage #815

Closed
wants to merge 1 commit into from

Conversation

grosser
Copy link
Contributor

@grosser grosser commented Oct 22, 2014

#812

@bf4

Risks

  • None

@schneems
Copy link

Looks like this does the trick:

Current master:

application: 59.4805 mb
  mail: 37.0586 mb
    mail/parsers: 18.6523 mb
      mail/parsers/ragel: 18.3398 mb
        mail/parsers/ragel/ruby: 18.2852 mb
          mail/parsers/ragel/ruby/machines/address_lists_machine: 7.5859 mb
          mail/parsers/ragel/ruby/machines/received_machine: 4.8789 mb
          mail/parsers/ragel/ruby/machines/envelope_from_machine: 2.1445 mb
          mail/parsers/ragel/ruby/machines/message_ids_machine: 1.3945 mb
          mail/parsers/ragel/ruby/machines/content_type_machine: 0.6094 mb
          mail/parsers/ragel/ruby/machines/content_disposition_machine: 0.4102 mb
          mail/parsers/ragel/ruby/machines/date_time_machine: 0.375 mb
          mail/parsers/ragel/ruby/machines/content_location_machine: 0.3516 mb

With this patch:

application: 39.3945 mb
  mail: 19.8789 mb
    mime/types: 18.543 mb
    mail/field: 0.4648 mb
    mail/message: 0.3203 mb

Parsers are not loaded until they're needed. Great work!

@bf4
Copy link
Collaborator

bf4 commented Oct 22, 2014

Fantastic! Waiting for tests to pass. Perhaps I can release this as a patch release to the 2.6 series.

@grosser
Copy link
Contributor Author

grosser commented Oct 22, 2014

yeah that would be nice, afaik most things that use mail do generating
instead of parsing :)

On Wed, Oct 22, 2014 at 10:34 AM, Benjamin Fleischer <
notifications@github.com> wrote:

Fantastic! Waiting for tests to pass. Perhaps I can release this as a
patch release to the 2.6 series.


Reply to this email directly or view it on GitHub
#815 (comment).

@bf4
Copy link
Collaborator

bf4 commented Oct 22, 2014

@grosser @schneems I created a 2-6-stable branch that has only the fixes in it. Waiting for tests to pass. #817

@bf4
Copy link
Collaborator

bf4 commented Oct 23, 2014

Closed by #817 via 8be5488

@bf4 bf4 closed this Oct 23, 2014
@tenderlove
Copy link
Contributor

If this doesn't load parsers until runtime, and my app needs the parser, does that mean that the lazy loading will ruin CoW advantages of eager loading? I mean, should people who use the parser and forking severs know that they should eagerly load? (Otherwise this could possibly increase memory usage)

@SamSaffron
Copy link
Contributor

aren't we usually doing mail parsing in sidekiqs anyway?

@grosser
Copy link
Contributor Author

grosser commented Feb 12, 2015

Yes balancing dev vs production needs is always problematic,
in rails you should use eager_load in production and maybe something
similar in other frameworks.

We do mail parsing in resque with forks.

On Wed, Feb 11, 2015 at 5:26 PM, Aaron Patterson notifications@github.com
wrote:

If this doesn't load parsers until runtime, and my app needs the parser,
does that mean that the lazy loading will ruin CoW advantages of eager
loading? I mean, should people who use the parser and forking severs know
that they should eagerly load? (Otherwise this could possibly increase
memory usage)


Reply to this email directly or view it on GitHub
#815 (comment).

@tenderlove
Copy link
Contributor

in rails you should use eager_load in production and maybe something
similar in other frameworks.

eager_load doesn't guarantee that the mail parser constant will be touched (e.g. the first time the constant is referenced could be in a method that isn't executed until runtime). :-/

@grosser
Copy link
Contributor Author

grosser commented Feb 12, 2015

but adding it to eager_load_paths would do the trick ?

On Wed, Feb 11, 2015 at 5:32 PM, Aaron Patterson notifications@github.com
wrote:

in rails you should use eager_load in production and maybe something
similar in other frameworks.

eager_load doesn't guarantee that the mail parser constant will be
touched (e.g. the first time the constant is referenced could be in a
method that isn't executed until runtime). :-/


Reply to this email directly or view it on GitHub
#815 (comment).

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.

5 participants