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

Avoid excessive heap allocations in HPACK's dynamic table #20

Merged
merged 4 commits into from
Jan 28, 2020

Conversation

GallaFrancesco
Copy link
Contributor

Two improvements that can help minimizing heap usage by the HPACK module:

1: Set table size in octets instead of number of entries

We were using the struct size for the dynamic table, creating a buffer which could hold up to 4096 HTTP2HeaderTableField entries. RFC 7540 specifies that the default value for the SETTINGS_HEADER_TABLE_SIZE parameter is, instead, 4096 octets. For this reason the initial dynamic table size is now computed as 4096 / (size of an empty struct HTTP2HeaderTableField) which is 32 octets. A test case has been added to make sure that this value is respected.

2: Avoid calling FixedRingBuffer.capacity

Profiling heap allocations with buildOptions "profileGC" and sending a single HTTP/2 request to the webserver, the heaviest heap allocation is performed by the forced use of FixedRingBuffer.capacity in the constructor for the internal DynamicTable, called when the main HPACK table for the HTTP/2 server is created.

Since we are using a fixed, default value for the header table size, it seems unnecessary to call capacity when most of the time the table size is set to 4096. It makes more sense to allow for two tables which can be used behind a unique interface:

  • m_table is a statically-allocated table with size of 4096, which gets created on struct DynamicTable initialization
  • m_extraTable is a dynamically allocated table which gets initialized iff the constructor for DynamicTable is called with a size parameter > 4096. Its size is therefore set to ms - 4096 and its entries are filled only when m_table is full.

Note that most clients (especially browsers) use the default value of 4096 octets when initializing a HTTP/2 connection, in which case the extra table is not initialized and the HPACK IndexingTable should not require dynamic allocations at all.

Copy link
Member

@s-ludwig s-ludwig left a comment

Choose a reason for hiding this comment

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

Oh man, I completely missed this one. It would be extremely helpful to be able to define priority rules for pull requests or repositories to get extra notifications.

Anyway, I plan to take the next step with this repository soon and make it an optional choice instead of vibe-d:http. This includes adding any missing functionality, as well as all additions since the fork.

@s-ludwig
Copy link
Member

Okay, build failures seem to be unrelated to the changes. I'll merge manually and look into it later.

@s-ludwig s-ludwig merged commit 6e03e0a into vibe-d:master Jan 28, 2020
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