Skip to content

HeaderMap: Store pos and hash as u16#386

Merged
seanmonstar merged 3 commits intohyperium:masterfrom
mbrubeck:u16
Jan 27, 2020
Merged

HeaderMap: Store pos and hash as u16#386
seanmonstar merged 3 commits intohyperium:masterfrom
mbrubeck:u16

Conversation

@mbrubeck
Copy link
Copy Markdown
Contributor

HeaderMap currently truncates all hashes to 16 bits, and limits its
capacity to 32768 elements. The comments say this is done in order to store
positions and hashes as u16, to improve cache locality. However, they are
currently stored as usize. This branch changes the code to match the
comments.

This does not appear to cause any measurable improvement or regression in
speed in the HeaderMap benchmarks on my workstation. However, it should at
least reduce the memory footprint of every Headermap.

@seanmonstar
Copy link
Copy Markdown
Member

Oh sweet!

(I've filed #387 for the deprecation errors.)

HeaderMap currently truncates all hashes to 16 bits, and limits its
capacity to 32768 elements. The comments say this is done in order to
store positions and hashes as u16, to improve cache locality.  However,
they are currently stored as usize.  This patch changes the code to
match the comments.

This does not appear to cause any measurable improvement or regression
in speed in the HeaderMap benchmarks. However, it should at least reduce
the memory footprint of every Headermap.
The actual maximum capacity is 24578 (i.e. `32768 * 3/4`).

A higher number would cause the raw capacity (i.e. `capacity +
capacity/3` rounded to the next power of 2) to overflow `u16`.
@mbrubeck
Copy link
Copy Markdown
Contributor Author

mbrubeck commented Jan 24, 2020

Note that the Link and Links types still store indices as usize. Changing these to u16 would probably be beneficial for some use cases, though they are not as important as Size and HashValue which together make up the entire HeaderMap::indices vector.

I didn't change Link and Links because it would touch a lot more code and add a lot more u16/usize conversions. This could be error-prone if not done carefully. However, it might be a good follow-up task.

@seanmonstar seanmonstar merged commit 8b609f9 into hyperium:master Jan 27, 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