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

Preserve header casing. Take two. 🎬 #104

Merged
merged 10 commits into from
Oct 4, 2020

Conversation

tomchristie
Copy link
Contributor

@tomchristie tomchristie commented Sep 8, 2020

Closes #31

Based on #103, focusing on minimising benchmark regressions.

  • Preserves HTTP header casing.
  • event.headers is a sequence-like instance, containing two-tuples of bytes.
  • event.headers.raw_items() is a list of two-tuples of (raw_name, value).

Three benchmark runs here against each case.

Running against current master...

7330.1 requests/sec
7357.9 requests/sec
7363.4 requests/sec
7310.5 requests/sec
7363.2 requests/sec
7354.3 requests/sec
7363.3 requests/sec

7321.4 requests/sec
7355.2 requests/sec
7353.7 requests/sec
7375.7 requests/sec
7352.6 requests/sec
7381.0 requests/sec
7410.0 requests/sec

7234.4 requests/sec
7274.0 requests/sec
7231.8 requests/sec
7272.5 requests/sec
7263.7 requests/sec
7268.9 requests/sec
7273.7 requests/sec

Running against this PR...

7022.7 requests/sec
7041.2 requests/sec
7052.2 requests/sec
7038.3 requests/sec
7038.5 requests/sec
7042.8 requests/sec
7053.2 requests/sec

7129.9 requests/sec
7145.6 requests/sec
7152.4 requests/sec
7158.3 requests/sec
7154.1 requests/sec
7116.6 requests/sec
7146.2 requests/sec

7080.6 requests/sec
7077.9 requests/sec
7100.3 requests/sec
7051.9 requests/sec
7091.7 requests/sec
7072.6 requests/sec
7056.3 requests/sec

@tomchristie tomchristie changed the title Preserve header casing, take 2 Preserve header casing. Take two. 🎬 Sep 8, 2020
@codecov
Copy link

codecov bot commented Sep 14, 2020

Codecov Report

Merging #104 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
+ Coverage   99.14%   99.15%   +0.01%     
==========================================
  Files          21       21              
  Lines         937      952      +15     
  Branches      173      175       +2     
==========================================
+ Hits          929      944      +15     
  Misses          7        7              
  Partials        1        1              
Impacted Files Coverage Δ
h11/tests/test_connection.py 100.00% <ø> (ø)
h11/tests/test_events.py 100.00% <ø> (ø)
h11/tests/test_headers.py 100.00% <ø> (ø)
h11/_connection.py 100.00% <100.00%> (ø)
h11/_headers.py 100.00% <100.00%> (ø)
h11/_readers.py 100.00% <100.00%> (ø)
h11/_writers.py 88.73% <100.00%> (+0.16%) ⬆️
h11/tests/test_io.py 100.00% <100.00%> (ø)
h11/tests/helpers.py 100.00% <0.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1540843...5f77670. Read the comment docs.

@tomchristie
Copy link
Contributor Author

tomchristie commented Sep 14, 2020

Okay, I think this is about the best we can do here.
I've updated the docs to include the one extra point of API that's now exposed.

Unavoidably it does benchmark marginally slower that the current master branch.

Before...

7211.1 requests/sec
7300.6 requests/sec
7298.3 requests/sec
7271.7 requests/sec
7342.2 requests/sec
7310.1 requests/sec
7334.3 requests/sec

After...

7025.6 requests/sec
7094.9 requests/sec
7095.7 requests/sec
7094.9 requests/sec
7115.5 requests/sec
7053.5 requests/sec
7104.2 requests/sec

Averaging to: 7295 requests/sec -> 7083 requests/sec

What do we think about this folks? @njsmith, @pgjones?

(From my POV I'd really like to get this resolved - we're currently treating it as one of our HTTPX 1.0 blockers.)

🙏💚😇

docs/source/api.rst Outdated Show resolved Hide resolved

original_headers = [("Host", "example.com")]
req = h11.Request(method="GET", target="/", headers=original_headers)
req.headers.raw_items()
Copy link
Contributor

Choose a reason for hiding this comment

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

Will there be output demonstrating the result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, see https://h11.readthedocs.io/en/latest/api.html - but I wasn't able to figure out the docs build locally just yet, so I've not seen the rendered docs myself.

Choose a reason for hiding this comment

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

You can see the docs rendered for this pull request at https://h11--104.org.readthedocs.build/en/104/

Copy link
Contributor Author

@tomchristie tomchristie Sep 15, 2020

Choose a reason for hiding this comment

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

Ah neat, thanks @pquentin.

Not 100% obvious what repr we'd want for the Headers there:

  • Use <Headers([(b'host', b'example.org')])> to make it clear it's not (quite) a plain list.
  • Use [(b'host', b'example.org')] to keep it looking simple.
  • Use ((b'host', b'example.org'),) since it's a non-mutable sequence.
  • Something else?

Some examples as currently rendered...

Screen Shot 2020-09-15 at 10 37 06

Screen Shot 2020-09-15 at 10 37 39

Co-authored-by: Stephen Brown II <Stephen.Brown2@gmail.com>
@tomchristie tomchristie mentioned this pull request Sep 22, 2020
4 tasks
@pquentin
Copy link

@StephenBrown2 @pgjones Would one of you be able to review this fine work? I'm not qualified (and not part of python-hyper anyways) and @njsmith isn't currently available for OSS reviews (even on Trio)

@pgjones
Copy link
Member

pgjones commented Oct 1, 2020

Minded to merge - happy to be corrected - will merge and release over the weekend if not.

@tomchristie
Copy link
Contributor Author

@pgjones Fantastic - pretty excited to get this one resolved. 🤗
Presumably we'll bump to 0.11 for this release?

@pgjones pgjones merged commit 0aac921 into python-hyper:master Oct 4, 2020
This was referenced Oct 6, 2020
@tomchristie tomchristie deleted the preserve-header-casing-2 branch October 13, 2020 08:32
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.

Discussion: Capitalize the header names of outgoing headers.
5 participants