Skip to content

Commit

Permalink
Change HeaderTable::names_map values to a small_vector
Browse files Browse the repository at this point in the history
Summary:
std::list is generally a bad choice, and this one is particularly bad for performance. Each iteration in getIndex essentially incurs a cache miss, which under load can be going all the way to DRAM.

Change the map to always a (flat) F14ValueMap, and optimize further by having the values inline most of the time. 7 is chosen as an empirically good value, while not being outrageously large.

Reviewed By: kvtsoy

Differential Revision: D58037451

fbshipit-source-id: 7a546aa00cab65c95a2ee1c25a79a45caa71c774
  • Loading branch information
Matt Joras authored and facebook-github-bot committed Jun 3, 2024
1 parent 2023e13 commit 7f46027
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 8 deletions.
1 change: 1 addition & 0 deletions proxygen/lib/http/codec/HTTP1xCodec.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#pragma once

#include <list>
#include <proxygen/lib/http/HTTPMessage.h>
#include <proxygen/lib/http/codec/HTTPCodec.h>
#include <proxygen/lib/http/codec/TransportDirection.h>
Expand Down
2 changes: 1 addition & 1 deletion proxygen/lib/http/codec/compress/HeaderTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ uint32_t HeaderTable::removeLast() {
DCHECK(names_it != names_.end());
auto& ilist = names_it->second;
DCHECK_EQ(ilist.front(), t);
ilist.pop_front();
ilist.erase(ilist.begin());

// remove the name if there are no indices associated with it
if (ilist.empty()) {
Expand Down
8 changes: 5 additions & 3 deletions proxygen/lib/http/codec/compress/HeaderTable.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@

#pragma once

#include <list>
#include <string>
#include <vector>

#include <folly/container/F14Map.h>
#include <folly/small_vector.h>
#include <proxygen/lib/http/codec/compress/HPACKHeader.h>

namespace proxygen {
Expand All @@ -24,8 +24,10 @@ namespace proxygen {

class HeaderTable {
public:
// TODO: std::vector might be faster than std::list in the use case?
using names_map = folly::F14FastMap<HPACKHeaderName, std::list<uint32_t>>;
// 7 is chosen as an empirically good value that's not too big.
// small_vector spills to the heap beyond that.
using names_map =
folly::F14ValueMap<HPACKHeaderName, folly::small_vector<uint32_t, 7>>;

explicit HeaderTable(uint32_t capacityVal) {
init(capacityVal);
Expand Down
8 changes: 4 additions & 4 deletions proxygen/lib/http/codec/compress/test/HPACKContextTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,11 @@ TEST_F(HPACKContextTests, StaticTableHeaderNamesAreCommon) {
"max-forwards",
"if-range",
"refresh"};
for (std::pair<HPACKHeaderName, std::list<uint32_t>> entry : table.names()) {
EXPECT_TRUE(entry.first.isCommonHeader() ||
uncommonStaticEntries.find(entry.first.get()) !=
for (const auto& [header, _] : table.names()) {
EXPECT_TRUE(header.isCommonHeader() ||
uncommonStaticEntries.find(header.get()) !=
uncommonStaticEntries.end())
<< entry.first.get();
<< header.get();
}
}

Expand Down

0 comments on commit 7f46027

Please sign in to comment.