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

Response.ContentEncoding(): store as field and avoid using Header.SetCanonical() #1311

Merged
merged 2 commits into from
Jun 5, 2022

Conversation

stokito
Copy link
Contributor

@stokito stokito commented Jun 4, 2022

The CE is not so often used for plain APIs responses and even not so often used for static files and on the fly compression.
But still it should be checked each time so instead of peek headers map it will be faster to check a field.
Also having a dedicated field getter and setter simplifies code.

header.go Outdated Show resolved Hide resolved
The CE is not so often used for plain APIs responses and even not so often used for static files and on the fly compression.
But still it should be checked each time.
Also having a dedicated field getter and setter simplifies code
…ial() methods instead of SetCanonical()

The change should improve performance because the setSpecialHeader() call is omitted.
As a downside on adding a new basic header field all putHeader() must be replaced with a direct getter and setter.
@erikdubbelboer
Copy link
Collaborator

Thanks!

@stokito
Copy link
Contributor Author

stokito commented Jun 5, 2022

Thank you, Eric. You may noticed that for the Request I didn't added the ConentEncoding().
The reason why I not did it is because the compressed requests aren't so often used.
So adding a new field here may be questionable.
In the OpenRTB spec they are mentioned

To enable compression on the bid request, it must first be agreed upon between the exchange and the
bidder that this is supported. This is similar to when a custom data format is used since the exchange
has to know both format and encoding before sending the bid request. If the bidder supports it, the
exchange should indicate it is sending a gzip compressed bid request by setting the HTTP 1.1 ContentEncoding header. The encoding value used should be “gzip”.
Content-Encoding: gzip
If this header is not set then it is assumed that the request content isn’t encoded. In HTTP 1.1, the
Content-Encoding header is usually only used for response content. However by using this header for
the request content as well we are able to indicate a request is compressed regardless of the data
format used. This is useful since even binary data formats can benefit from being compressed.

And some providers supports this e.g. https://docs.openx.com/demand-partners/compression-traffic-gzip/ or https://protocol.bidswitch.com/rtb/data-format.html

Even while not so many such http clients but under a heavy load this may have a difference.
Adding a new field has a downside because makes Peek() or VisitAll() slower.
So I don't know if it worth to add a separate field.
Maybe just to keep Resp/Req method similar we can add the ContentEncoding() method that internally will just peek it from the headers map. Similarly to as Referer() works.

@erikdubbelboer
Copy link
Collaborator

Lets leave it for now until someone complains about it.

@stokito stokito deleted the ce branch June 5, 2022 19:13
bbenzikry pushed a commit to bbenzikry/fasthttp that referenced this pull request Sep 11, 2022
…Canonical() (valyala#1311)

* Response.ContentEncoding(): store as field

The CE is not so often used for plain APIs responses and even not so often used for static files and on the fly compression.
But still it should be checked each time.
Also having a dedicated field getter and setter simplifies code

* header.go Use shorter Response.setNonSpecial() and Request.setNonSpecial() methods instead of SetCanonical()

The change should improve performance because the setSpecialHeader() call is omitted.
As a downside on adding a new basic header field all putHeader() must be replaced with a direct getter and setter.
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