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

gh-118761: Improve import time of tomllib #128907

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hukkin
Copy link
Contributor

@hukkin hukkin commented Jan 16, 2025

This is a port of two commits in Tomli repository (the diff of these commits being hukkin/tomli@42a570d...1da01ef).

This removes runtime overhead of string, typing, and tomllib._types imports from import tomllib.

main branch

$ hyperfine --warmup 8 "./python -c 'import tomllib'" "./python -c 'pass'"
Benchmark 1: ./python -c 'import tomllib'
  Time (mean ± σ):      70.3 ms ±   1.8 ms    [User: 66.0 ms, System: 4.3 ms]
  Range (min … max):    68.6 ms …  76.8 ms    41 runs
 
Benchmark 2: ./python -c 'pass'
  Time (mean ± σ):      26.2 ms ±   0.7 ms    [User: 23.3 ms, System: 3.0 ms]
  Range (min … max):    25.1 ms …  29.7 ms    110 runs
 
Summary
  './python -c 'pass'' ran
    2.69 ± 0.10 times faster than './python -c 'import tomllib''

PR branch

$ hyperfine --warmup 8 "./python -c 'import tomllib'" "./python -c 'pass'"
Benchmark 1: ./python -c 'import tomllib'
  Time (mean ± σ):      48.0 ms ±   0.7 ms    [User: 43.4 ms, System: 4.7 ms]
  Range (min … max):    46.8 ms …  50.2 ms    60 runs
 
Benchmark 2: ./python -c 'pass'
  Time (mean ± σ):      26.1 ms ±   0.4 ms    [User: 23.5 ms, System: 2.7 ms]
  Range (min … max):    25.3 ms …  27.4 ms    110 runs
 
Summary
  './python -c 'pass'' ran
    1.84 ± 0.04 times faster than './python -c 'import tomllib''

Lib/tomllib/_parser.py Show resolved Hide resolved
Comment on lines -118 to +124
def load(fp: BinaryIO, /, *, parse_float: ParseFloat = float) -> dict[str, Any]:
def load(fp: IO[bytes], /, *, parse_float: ParseFloat = float) -> dict[str, Any]:
Copy link
Member

Choose a reason for hiding this comment

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

I think that this should technically be a protocol similar to typeshed's SupportsRead. But for a slightly more radical proposal, would you support removing all the type annotations from tomllib? The ones in typeshed are used by all of the type checkers, and there's a risk of going out of sync, or unhelpful 'improvements' to adjust the (unused) type hints in Lib/tomllib.

Copy link
Contributor Author

@hukkin hukkin Jan 16, 2025

Choose a reason for hiding this comment

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

I think that this should technically be a protocol similar to typeshed's SupportsRead.

This is true. I changed this annotation because the the diff I was porting included an updated from typing import line that uses IO not BinaryIO. A trade-off was made choosing this annotation, and so far nobody complained about it.

But for a slightly more radical proposal, would you support removing all the type annotations from tomllib?

I wouldn't support it, at least not before Python 3.10 EOL date. Type annotations were inherited from Tomli, and having them here makes diffs smaller and easier to work with -> syncing changes such as this PR is a bit easier for me.

That said, no big deal for me. If folks want to remove the annotations, I'd be fine with it.

Copy link
Member

@AA-Turner AA-Turner Jan 16, 2025

Choose a reason for hiding this comment

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

Fair enough -- I'd remove them personally, but I'm not the maintainer of tomllib and being able to easily synchronise updates is a good reason -- we might re-visit in a couple of years!

IO[bytes] is probably fine for now the given this.

@vstinner
Copy link
Member

cc @encukou who added the module in Python 3.11 (PEP 680).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants