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

Use quoted_printable_encode() if available for performance reasons #217

Merged
merged 2 commits into from Jul 1, 2012

Conversation

Projects
None yet
4 participants
@lstrojny
Contributor

lstrojny commented Jun 25, 2012

If we have a PHP version that offers quoted_printable_encode() we should it as it is much faster.

(Pull request courtesy of @maxbeutel and myself)

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 25, 2012

Member

It breaks a bunch of tests.

Member

fabpot commented Jun 25, 2012

It breaks a bunch of tests.

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Jun 25, 2012

Contributor

Would it maybe be better to have NativeQpEncoder and leave the userland implementation untouched?

Contributor

lstrojny commented Jun 25, 2012

Would it maybe be better to have NativeQpEncoder and leave the userland implementation untouched?

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 26, 2012

Member

This is probably a better idea.

Member

fabpot commented Jun 26, 2012

This is probably a better idea.

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Jun 26, 2012

Contributor

OK, here is a standalone implementation. We measured 30x (down from 100 mails in 30 seconds to 100 mails in 1 second) more throughput with the native function compared to the userland implementation. And here is how to use it (swift_init.php):

<?php
...
Swift_DependencyContainer::getInstance()
    ->register('mime.qpcontentencoder')
    ->asAliasOf('mime.nativeqpcontentencoder');
Contributor

lstrojny commented Jun 26, 2012

OK, here is a standalone implementation. We measured 30x (down from 100 mails in 30 seconds to 100 mails in 1 second) more throughput with the native function compared to the userland implementation. And here is how to use it (swift_init.php):

<?php
...
Swift_DependencyContainer::getInstance()
    ->register('mime.qpcontentencoder')
    ->asAliasOf('mime.nativeqpcontentencoder');
@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Jun 30, 2012

Contributor

From my POV this pull request is ready to be merged. Comments?

Contributor

lstrojny commented Jun 30, 2012

From my POV this pull request is ready to be merged. Comments?

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 30, 2012

Member

Can you squash you commits? Thanks.

Member

fabpot commented Jun 30, 2012

Can you squash you commits? Thanks.

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Jun 30, 2012

Contributor

Done

Contributor

lstrojny commented Jun 30, 2012

Done

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 30, 2012

Member

Can you also add a note somewhere in the documentation (probably in including-the-files.rst) that when using PHP >= 5.3.0, this is the recommended class to use (with a snippet of code on how to configure it)?

Member

fabpot commented Jun 30, 2012

Can you also add a note somewhere in the documentation (probably in including-the-files.rst) that when using PHP >= 5.3.0, this is the recommended class to use (with a snippet of code on how to configure it)?

@lukaswoj

This comment has been minimized.

Show comment
Hide comment
@lukaswoj

lukaswoj Jul 1, 2012

Hi there
I'm so happy to find out this PR. Looks like really fresh topic.
Thank you Lars

I was just struggling with the same problem of high CPU usage when sending many emails in bulk.
In my case the problem came from the fact that emails needs to be dispatched at given moment in time and the amount of them makes whole process taking too long.
Since I knew in advance what will be the content of each email - I even had the idea to asynchronously preencode body of the messages and later pass already encoded string.
But now this does not matter anymore.

Great job!

Thanks again
Cheers

lukaswoj commented Jul 1, 2012

Hi there
I'm so happy to find out this PR. Looks like really fresh topic.
Thank you Lars

I was just struggling with the same problem of high CPU usage when sending many emails in bulk.
In my case the problem came from the fact that emails needs to be dispatched at given moment in time and the amount of them makes whole process taking too long.
Since I knew in advance what will be the content of each email - I even had the idea to asynchronously preencode body of the messages and later pass already encoded string.
But now this does not matter anymore.

Great job!

Thanks again
Cheers

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Jul 1, 2012

Contributor

Documentation added. @lukaswoj glad we could help you.

Contributor

lstrojny commented Jul 1, 2012

Documentation added. @lukaswoj glad we could help you.

fabpot added a commit that referenced this pull request Jul 1, 2012

merged branch InterNations/native-quoted-printable-encode (PR #217)
Commits
-------

ed0312b Adding doc hint
449117d Native quoted printable content encoder

Discussion
----------

Use quoted_printable_encode() if available for performance reasons

If we have a PHP version that offers quoted_printable_encode() we should it as it is much faster.

(Pull request courtesy of @maxbeutel and myself)

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

by fabpot at 2012-06-25T19:37:32Z

It breaks a bunch of tests.

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

by lstrojny at 2012-06-25T19:49:29Z

Would it maybe be better to have NativeQpEncoder and leave the userland implementation untouched?

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

by fabpot at 2012-06-26T05:06:13Z

This is probably a better idea.

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

by lstrojny at 2012-06-26T09:36:20Z

OK, here is a standalone implementation. We measured 30x (down from 100 mails in 30 seconds to 100 mails in 1 second) more throughput with the native function compared to the userland implementation. And here is how to use it (`swift_init.php`):

```php
<?php
...
Swift_DependencyContainer::getInstance()
    ->register('mime.qpcontentencoder')
    ->asAliasOf('mime.nativeqpcontentencoder');

```

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

by lstrojny at 2012-06-30T20:10:59Z

From my POV this pull request is ready to be merged. Comments?

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

by fabpot at 2012-06-30T20:17:48Z

Can you squash you commits? Thanks.

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

by lstrojny at 2012-06-30T20:25:32Z

Done

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

by fabpot at 2012-06-30T20:25:42Z

Can you also add a note somewhere in the documentation (probably in `including-the-files.rst`) that when using PHP >= 5.3.0, this is the recommended class to use (with a snippet of code on how to configure it)?

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

by lukaswoj at 2012-07-01T03:50:08Z

Hi there
I'm so happy to find out this PR. Looks like really fresh topic.
Thank you Lars

I was just struggling with the same problem of high CPU usage when sending many emails in bulk.
In my case the problem came from the fact that emails needs to be dispatched at given moment in time and the amount of them makes whole process taking too long.
Since I knew in advance what will be the content of each email - I even had the idea to asynchronously preencode body of the messages and later pass already encoded string.
But now this does not matter anymore.

Great job!

Thanks again
Cheers

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

by lstrojny at 2012-07-01T13:37:38Z

Documentation added. @lukaswoj glad we could help you.

@fabpot fabpot merged commit ed0312b into swiftmailer:master Jul 1, 2012

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jul 1, 2012

Member

merged, thanks!

Member

fabpot commented Jul 1, 2012

merged, thanks!

@c2h5oh

This comment has been minimized.

Show comment
Hide comment
@c2h5oh

c2h5oh Jul 2, 2012

Sweet, email batch that used to take over 4 hours got sent in just under 20 minutes. Why don't we make it default for PHP 5.3.0+ ?

<?php
...

if (version_compare(PHP_VERSION, '5.3.0') >= 0) {
    Swift_DependencyContainer::getInstance()
        ->register('mime.qpcontentencoder')
        ->asAliasOf('mime.nativeqpcontentencoder');
}

c2h5oh commented Jul 2, 2012

Sweet, email batch that used to take over 4 hours got sent in just under 20 minutes. Why don't we make it default for PHP 5.3.0+ ?

<?php
...

if (version_compare(PHP_VERSION, '5.3.0') >= 0) {
    Swift_DependencyContainer::getInstance()
        ->register('mime.qpcontentencoder')
        ->asAliasOf('mime.nativeqpcontentencoder');
}
@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jul 2, 2012

Member

@c2h5oh That's indeed a good idea. We might want to get some more feedback from people like you before switching it to be the default. I've opened a ticket for the change (see #220).

Member

fabpot commented Jul 2, 2012

@c2h5oh That's indeed a good idea. We might want to get some more feedback from people like you before switching it to be the default. I've opened a ticket for the change (see #220).

makasim pushed a commit to formapro-forks/swiftmailer that referenced this pull request Jul 26, 2017

makasim pushed a commit to formapro-forks/swiftmailer that referenced this pull request Jul 26, 2017

Merge pull request #222 from jmontoyaa/master
Adding a new maxColumnWidth property see #217
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment