Skip to content

TS-3535: Experimental Support of Stream Priority Feature in HTTP/2#525

Closed
masaori335 wants to merge 1 commit intoapache:masterfrom
masaori335:TS-3535
Closed

TS-3535: Experimental Support of Stream Priority Feature in HTTP/2#525
masaori335 wants to merge 1 commit intoapache:masterfrom
masaori335:TS-3535

Conversation

@masaori335
Copy link
Copy Markdown
Contributor

This is an experimental support of Stream Priority Feature. ( TS-3535 )

Approach

  • Basically I followed WFQ Scheduling Algorithm invented by Kazuho and introduced by Kazu and Tatsuhiro at ATS Meetup in Tokyo
  • Using MinHeap for PriorityQueue

Issues

  • Currently overheads of Priority Feature is high, we need optimization.
  • Works fine with Chrome, FireFox, Safari (on MacOSX), but has troubles with h2spec and h2load.

@zwoop
Copy link
Copy Markdown
Contributor

zwoop commented Mar 15, 2016

Great to see this implemented, and thanks to everyone for sharing their experiences and algorithms at that Meetup, it was incredibly useful.

Seeing that there are some pretty important issues, should we leave it disabled for now, with a records.config option to enable it?

@jpeach
Copy link
Copy Markdown
Contributor

jpeach commented Mar 15, 2016

@zwoop see proxy.config.http2.stream_priority_enabled?

@zwoop
Copy link
Copy Markdown
Contributor

zwoop commented Mar 15, 2016

Perfect!

@zwoop
Copy link
Copy Markdown
Contributor

zwoop commented Mar 22, 2016

@bryancall to review, if we land this with off by default it seems harmless. We do want to avoid STL when possible, but it is what it is :).

@PSUdaemon
Copy link
Copy Markdown
Contributor

Is it possible to make the priority queue stuff more generic so it can be reusable?

@masaori335
Copy link
Copy Markdown
Contributor Author

@zwoop Should we avoid STL in test code too? I used ts/Vec.h and implemented priority queue to avoid STL:D

@masaori335
Copy link
Copy Markdown
Contributor Author

@PSUdaemon Yes, it is! It is one of reasons of implemented Http2PriorityQueue using template.
Is it better to change name and move under lib/ts/?

@PSUdaemon
Copy link
Copy Markdown
Contributor

@masaori335, yes exactly. It does look very generic already and it seems valuable to have in lib/ts. I can't think of anything offhand that would need it, but if you are already adding it for H2 seems prudent to make it available to other things.

@zwoop
Copy link
Copy Markdown
Contributor

zwoop commented Mar 23, 2016

+1 on adding it to lib/ts. As for STL, the general rule is that if something runs on the critical path (e.g. as part of the HttpSM, or normal transaction handling), where it is performance sensitive, we should make every effort to avoid STL. As a subsidiary to that, we should really think hard before using std::string on said critical path as well (there's generally little reason to use std::string there).

Now, the exception here are two things:

  1. Plugins. We don't dictate anything for plugins, so use whatever you deem necessary to make it work. If someone doesn't like it, they can either fix it, or they can not use the plugin.

  2. Anything not on the critical path. This includes configuration loading, traffic_manager/cop, periodic tasks, tests etc.

That much said, don't use STL just for the hell of it. If there is a reasonable alternative to use in lib/ts, please use it. Dragging in STL does increase binary sizes, which could affect performance in other ways (such as worse L2/3 caches etc.). It also sets bad precedence, where someone seeing it used, makes the mistake and thinks it's ok to use everywhere.

@masaori335
Copy link
Copy Markdown
Contributor Author

I spun out priority queue stuff as TS-4295.
I'm going to fix this patch to use it and fix conflicts.

@masaori335
Copy link
Copy Markdown
Contributor Author

Now, FetchSM and PluginVC is removed from HTTP/2 components by TS-3612. It look like I need more works to rebase this on latest master. I'm going to close this once and reopen new Pull-Requests for v7.0.0 to avoid blocking v6.2.0 release.

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.

4 participants