Skip to content

Conversation

@masaori335
Copy link
Contributor

The std::deque is an appropriate container for HPACK dynamic table. The use cases are below.
All of them are O(1) with std::deque.

  • random access
  • insertion at its beginning
  • deletion at its end

@masaori335 masaori335 added this to the 10.0.0 milestone Oct 30, 2019
@masaori335 masaori335 requested a review from vmamidi October 30, 2019 02:55
@masaori335 masaori335 self-assigned this Oct 30, 2019
@maskit
Copy link
Member

maskit commented Oct 30, 2019

Use of deque sounds reasonable to me. If I remember correctly the reason we didn't use it is that our data container policy only allowed use of std::vector from std data containers. If we allow it now, there's no reason to not use it.

It should be faster than current but did you check the performance?

@vmamidi
Copy link
Contributor

vmamidi commented Oct 30, 2019 via email

@maskit
Copy link
Member

maskit commented Oct 30, 2019

What is the data container policy ?

Undocumented (or hard to find) policy. I'm not sure if there is a list of containers we can use, but the basic rule is this. https://issues.apache.org/jira/browse/TS-3770
As far as I know, vector and unordered_map are the only STL data containers we can use.

@masaori335
Copy link
Contributor Author

As far as I know, ATS doesn't have "double-ended queue" nor any data structures which satisfy our needs here. IMO, we should add std::deque on the white list if it doesn't have. Also, we should write the policy on the cwiki or somewhere.

As for the benchmark, the throughput is almost same. However, from the dtrace, memmove on the HpackDynamicTable::_evict_overflowed_entries() like below is disappeared.

              libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell+0x110
              traffic_server`HpackDynamicTable::_evict_overflowed_entries()+0xc9
              traffic_server`HpackDynamicTable::add_header_field(MIMEField const*)+0x6e

@vmamidi
Copy link
Contributor

vmamidi commented Oct 30, 2019 via email

@bryancall bryancall changed the title Replace container of HAPCK dynamic table from std::vector to std::deque Replace container of HPACK dynamic table from std::vector to std::deque Oct 30, 2019
@bryancall bryancall merged commit 323d5b5 into apache:master Oct 30, 2019
@zwoop
Copy link
Contributor

zwoop commented Dec 10, 2019

Cherry-picked to v9.0.x branch.

@zwoop zwoop modified the milestones: 10.0.0, 9.0.0 Dec 10, 2019
@zwoop zwoop modified the milestones: 9.0.0, 8.1.0 Mar 30, 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.

5 participants