Skip to content

Conversation

@masaori335
Copy link
Contributor

@masaori335 masaori335 commented Apr 9, 2020

http2_generate_h2_header_from_1_1() is one of bottlenecks of HTTP/2 subsystem because of extra HdrHeap allocation and all header copies.

  • master
    http2_generate_h2_header_from_1_1() is 32% of Http2ConnectionState::send_headers_frame()

master

  • 6635
    http2_convert_header_from_1_1_to_2() is 3% of Http2ConnectionState::send_headers_frame()

6635

Avoid extra HdrHeap allocation and copy all headers.
@masaori335 masaori335 added this to the 10.0.0 milestone Apr 9, 2020
@masaori335 masaori335 self-assigned this Apr 9, 2020
@masaori335 masaori335 linked an issue Apr 9, 2020 that may be closed by this pull request
12 tasks
@maskit
Copy link
Member

maskit commented Apr 9, 2020

I'm not sure if this is still true, but I think I had to generate a new HTTPHdr when I add/modify the function. The reason was probably that the original HTTPHdr was read from somewhere else later and the code expected HTTP/1.1 style.

Copy link
Member

@shinrich shinrich left a comment

Choose a reason for hiding this comment

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

Looks good. Updating in place makes a lot of sense.

@masaori335
Copy link
Contributor Author

@maskit Was it FetchSM::check_chunk()? We got rid of it. Also, AuTests covers chunked contents cases well.

TS-4321 - Masakazu Kitajo added a comment - 06/Apr/16 02:19
I confirmed that it is certainly caused by the last HPACK change, and is triggered by TE: chunked.
The last HPACK change removes TE header from a header list which is contained in a FetchSM, so check_chunk() in FetchSM doesn't return true.

@maskit
Copy link
Member

maskit commented Apr 10, 2020

@masaori335 Yes, you found it! Then it should be fine, but let me review this a bit more.

Copy link
Member

@maskit maskit left a comment

Choose a reason for hiding this comment

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

I'm not a big fun of the additional initialization call for pseudo headers, but I don't have a better idea. Ideally, HTTPHdr should probably have a concept of pseudo header.

@masaori335 masaori335 merged commit 3b0716d into apache:master Apr 12, 2020
@masaori335 masaori335 mentioned this pull request Apr 14, 2020
12 tasks
@masaori335 masaori335 removed a link to an issue Jul 2, 2020
12 tasks
@zwoop
Copy link
Contributor

zwoop commented Sep 3, 2020

Cherry-picked to v9.0.x branch.

@zwoop zwoop modified the milestones: 10.0.0, 9.0.0 Sep 3, 2020
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.

4 participants