Skip to content

Cache the expensive only_us_ascii? check - #1119

Closed
dalibor wants to merge 1 commit into
mikel:masterfrom
dalibor:only_us_ascii
Closed

dalibor wants to merge 1 commit into
mikel:masterfrom
dalibor:only_us_ascii

Conversation

@dalibor

@dalibor dalibor commented Jun 1, 2017

Copy link
Copy Markdown
Contributor

Is this safe to do?

In my benchmarks I'm seeing from 15-20% speed improvement when building emails with a 40KB html part and 30-40% speed improvement when building emails with large attachments.

In my benchmarks I'm seeing from 15-20% speed improvement when building
emails with a 40KB html part and 30-40% speed improvement when building
emails with large attachments.
@jeremy jeremy added this to the 2.7.0 milestone Jun 1, 2017
@jeremy jeremy closed this in 679c477 Jun 1, 2017

@jeremy jeremy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Safe since raw_source isn't changed after it's set. Thanks @dalibor!

@dalibor

dalibor commented Jun 1, 2017

Copy link
Copy Markdown
Contributor Author

@jeremy Great using String#ascii_only?! The only difference is that the original regex excludes "\x00" while ascii_only? includes it, does it matter? With this change the test suite time goes from 2.3 seconds to 1.6 seconds. And also, I'm seeing significant performance improvements in benchmarks locally building real emails.

30 attachments of ~ 10MB each:   17.801965 sec -> 5.149144 sec (~ 3x faster)
10000 html parts of ~ 40KB each: 16.148903 sec -> 9.189533 sec (~ 1x faster)

@jeremy

jeremy commented Jun 1, 2017

Copy link
Copy Markdown
Collaborator

IMO that was a bug, since we don't want NUL bytes in our US-ASCII text either :)

Very nice improvements!

@dalibor

dalibor commented Jun 1, 2017

Copy link
Copy Markdown
Contributor Author

It's the other way around, the new ascii_only? check will allow NUL bytes in emails I'm afraid. :) If we want to prevent that, we could do raw_source.ascii_only? && raw_source !~ /\x00/ :/, but that would be slower than ascii_only? and faster than the original regex.

@jeremy

jeremy commented Jun 11, 2017

Copy link
Copy Markdown
Collaborator

Hmm yes. This seems like an implementation wart, really. NUL is legit for US-ASCII. It's not legal in emails, but that's not what we're validating here, so we can get away with the ascii_only?check.

@dalibor

dalibor commented Jun 11, 2017

Copy link
Copy Markdown
Contributor Author

Great, thanks for checking that, Jeremy.

@dalibor
dalibor deleted the only_us_ascii branch November 2, 2017 06:47
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.

2 participants