GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-109653: Improve the import time of `email.utils` by AlexWaygood · Pull Request #109824 · python/cpython

Conversation

@AlexWaygood

@AlexWaygood AlexWaygood commented Sep 25, 2023 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This patch reduces the import time of email.utils by around 46%.

Why do I care about email.utils? Well, email.utils is imported by email.message, and email.message is imported by lots of other stdlib modules: urllib.parse, mailbox, and importlib.metadata. Improving the import time of this module has a cascading effect through lots of the rest of the standard library.

A lot of the import cost of email.utils has to do with the make_msgid function, which requires the random and socket modules. This function is used by many third-party libraries, but isn't used at all by any of the stdlib modules that import email.utils. This patch moves the function into a separate submodule.

  • Issue: Improve import time of various stdlib modules #109653

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
AlexWaygood added 2 commits September 25, 2023 10:40
@AlexWaygood
WIP
0aca553
@AlexWaygood
news
f3c815d
@AlexWaygood AlexWaygood added type-feature A feature request or enhancement performance Performance or resource usage topic-email 3.13 only security fixes labels Sep 25, 2023
@AlexWaygood
AlexWaygood requested a review from a team as a code owner September 25, 2023 10:03
@bedevere-app bedevere-app Bot added the awaiting review label Sep 25, 2023
@bedevere-app bedevere-app Bot mentioned this pull request Sep 25, 2023
Improve import time of various stdlib modules #109653
Closed
sobolevn
sobolevn reviewed Sep 25, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-09-25-10-47-22.gh-issue-109653.TUHrId.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood @sobolevn
I can spell my own name
992274f 
Co-authored-by: Nikita Sobolev <[email protected]>
AA-Turner
AA-Turner reviewed Sep 25, 2023
View reviewed changes
Comment thread Lib/email/_msgid.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/email/_msgid.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/email/utils.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
AA-Turner
AA-Turner reviewed Sep 25, 2023
View reviewed changes
Comment thread Lib/email/utils.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood @AA-Turner
Apply suggestions from code review
493aa33 
Co-authored-by: Adam Turner <[email protected]>
bitdancer
bitdancer reviewed Sep 25, 2023
View reviewed changes
Comment thread Lib/email/utils.py Outdated
if attr == "make_msgid":
from email._msgid import make_msgid
return make_msgid
raise AttributeError(f"module {__name__!r} has no attribute {attr!r}")

@bitdancer bitdancer Sep 25, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

In general I don't like performance hacks like this, but they do have their place. I can't speak to whether or not this is a worthwhile performance hack, you should seek approval from the maintainers of the impacted modules for that.

That said, the stdlib itself makes no use of make_msgid, and email.utils is not itself considered part of the new email API. Moving it into a separate module and making that part of the non-legacy public API of the email module would actually make some sense. I guess we'd just call it 'msgid'? Then this code should issue a deprecation warning pointing to the new way to import make_msgid. It feels kind of weird to have a module with just one function, but there isn't really anything else related to it that I can think of. (I wonder...maybe make_msgid actually belongs in the UUID module? Probably not. Wrong RFC.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@hauntsaninja

hauntsaninja commented Oct 3, 2023

Copy link
Copy Markdown
Contributor

This feels a little strange to me. Should we instead just defer the imports? Might even be faster to do so
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood

AlexWaygood commented Oct 3, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

This feels a little strange to me. Should we instead just defer the imports? Might even be faster to do so

Yeah, that might be a better shout. Have been meaning to give it a go — just haven't got round to it yet :)

I'm not really enthused about the idea of a painful deprecation period in order to change the place where people are "meant" to import it from. This function is pretty widely used by third-party packages, and the improvement in import time doesn't seem worth the disruption to me :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood and others added 5 commits October 3, 2023 15:45
@AlexWaygood
Merge branch 'main' into email-message-import
a697d9d
@AlexWaygood
Abandon the separate submodule idea
03233f1
@AlexWaygood
stats are slightly less impressive now because importing random is ch…
f11bc98 
…eaper now
@AlexWaygood
Merge branch 'email-message-import' of https://github.com/AlexWaygood…
c5e7a61 
…/cpython into email-message-import
@AlexWaygood
Merge branch 'main' into email-message-import
bcaeb4c
@AlexWaygood

AlexWaygood commented Oct 3, 2023

Copy link
Copy Markdown
Member Author

I've updated the PR. I abandoned the idea of adding a new submodule; now I just defer the problematic random, socket and os imports to inside make_msgid(). The reason I didn't do that originally was because I was concerned it would slow down the make_msgid() function, but local experimentation doesn't show that. (I ran python -m timeit -s "from email.utils import make_msgid" "make_msgid()" both with and without the change to test.) Shows the importance of benchmarking!
AA-Turner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AA-Turner
AA-Turner reviewed Oct 3, 2023
View reviewed changes
Comment thread Lib/email/utils.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood @AA-Turner
Update Lib/email/utils.py
b387dcd 
Co-authored-by: Adam Turner <[email protected]>
hauntsaninja
hauntsaninja approved these changes Oct 4, 2023
View reviewed changes

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks good

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/email/utils.py
except UnicodeEncodeError:
if isinstance(charset, str):
# lazy import to improve module import time
from email.charset import Charset

@hauntsaninja hauntsaninja Oct 4, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Not sure I like this one, only fine with it since formataddr doesn't look too widely used (a lot of the time it's nice to pay these costs upfront, predictable performance is important, e.g. don't want the first request your webserver serves to be randomly slow)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@AlexWaygood AlexWaygood Oct 4, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

(a lot of the time it's nice to pay these costs upfront, predictable performance is important

agreed. On the other hand, though, the email package goes in quite heavily for lazy imports in some other places, so this does seem in keeping with that general philosophy:

cpython/Lib/email/__init__.py

Lines 28 to 61 in f786029

# Some convenience routines. Don't import Parser and Message as side-effects
# of importing email since those cascadingly import most of the rest of the
# email package.
def message_from_string(s, *args, **kws):
"""Parse a string into a Message object model.
Optional _class and strict are passed to the Parser constructor.
"""
from email.parser import Parser
return Parser(*args, **kws).parsestr(s)
def message_from_bytes(s, *args, **kws):
"""Parse a bytes string into a Message object model.
Optional _class and strict are passed to the Parser constructor.
"""
from email.parser import BytesParser
return BytesParser(*args, **kws).parsebytes(s)
def message_from_file(fp, *args, **kws):
"""Read a file and parse its contents into a Message object model.
Optional _class and strict are passed to the Parser constructor.
"""
from email.parser import Parser
return Parser(*args, **kws).parse(fp)
def message_from_binary_file(fp, *args, **kws):
"""Read a binary file and parse its contents into a Message object model.
Optional _class and strict are passed to the Parser constructor.
"""
from email.parser import BytesParser
return BytesParser(*args, **kws).parse(fp)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@AlexWaygood AlexWaygood Oct 4, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'd be happy to change it so it's imported at the top of the function if you think that'd be better?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@hauntsaninja hauntsaninja Oct 4, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nah that'd be worse, module level or here. I'm fine with this as is!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/email/utils.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Oct 4, 2023
AlexWaygood added 2 commits October 4, 2023 22:08
@AlexWaygood
Merge branch 'main' of https://github.com/python/cpython into email-m…
c72ee5c 
…essage-import
@AlexWaygood
eagerly import os, it is free
e201028
@hauntsaninja
hauntsaninja merged commit aa3f419 into python:main Oct 12, 2023
@bedevere-app bedevere-app Bot removed the awaiting core review label Oct 12, 2023
@AlexWaygood
AlexWaygood deleted the email-message-import branch October 12, 2023 22:26
@AlexWaygood
AlexWaygood restored the email-message-import branch February 22, 2024 16:37
@AlexWaygood
AlexWaygood deleted the email-message-import branch March 7, 2024 14:35
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@AlexWaygood @Glyphack
pythongh-109653: Improve the import time of email.utils (python#109824
7867907 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bitdancer bitdancer bitdancer left review comments
@sobolevn sobolevn sobolevn left review comments
@AA-Turner AA-Turner AA-Turner left review comments
@hauntsaninja hauntsaninja hauntsaninja approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

3.13 only security fixes performance Performance or resource usage topic-email type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@AlexWaygood @hauntsaninja @bitdancer @sobolevn @AA-Turner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.