{"action":"created","issue":{"url":"https://api.github.com/repos/apache/trafficserver/issues/4212","repository_url":"https://api.github.com/repos/apache/trafficserver","labels_url":"https://api.github.com/repos/apache/trafficserver/issues/4212/labels{/name}","comments_url":"https://api.github.com/repos/apache/trafficserver/issues/4212/comments","events_url":"https://api.github.com/repos/apache/trafficserver/issues/4212/events","html_url":"https://github.com/apache/trafficserver/pull/4212","id":357812964,"node_id":"MDExOlB1bGxSZXF1ZXN0MjEzNzM5MzQ1","number":4212,"title":"HTTP/2 priority fixes to match common browser patterns","user":{"login":"shinrich","id":334488,"node_id":"MDQ6VXNlcjMzNDQ4OA==","avatar_url":"https://avatars0.githubusercontent.com/u/334488?v=4","gravatar_id":"","url":"https://api.github.com/users/shinrich","html_url":"https://github.com/shinrich","followers_url":"https://api.github.com/users/shinrich/followers","following_url":"https://api.github.com/users/shinrich/following{/other_user}","gists_url":"https://api.github.com/users/shinrich/gists{/gist_id}","starred_url":"https://api.github.com/users/shinrich/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/shinrich/subscriptions","organizations_url":"https://api.github.com/users/shinrich/orgs","repos_url":"https://api.github.com/users/shinrich/repos","events_url":"https://api.github.com/users/shinrich/events{/privacy}","received_events_url":"https://api.github.com/users/shinrich/received_events","type":"User","site_admin":false},"labels":[{"id":374860791,"node_id":"MDU6TGFiZWwzNzQ4NjA3OTE=","url":"https://api.github.com/repos/apache/trafficserver/labels/HTTP/2","name":"HTTP/2","color":"5319e7","default":false},{"id":551674950,"node_id":"MDU6TGFiZWw1NTE2NzQ5NTA=","url":"https://api.github.com/repos/apache/trafficserver/labels/Performance","name":"Performance","color":"b60205","default":false}],"state":"closed","locked":false,"assignee":{"login":"shinrich","id":334488,"node_id":"MDQ6VXNlcjMzNDQ4OA==","avatar_url":"https://avatars0.githubusercontent.com/u/334488?v=4","gravatar_id":"","url":"https://api.github.com/users/shinrich","html_url":"https://github.com/shinrich","followers_url":"https://api.github.com/users/shinrich/followers","following_url":"https://api.github.com/users/shinrich/following{/other_user}","gists_url":"https://api.github.com/users/shinrich/gists{/gist_id}","starred_url":"https://api.github.com/users/shinrich/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/shinrich/subscriptions","organizations_url":"https://api.github.com/users/shinrich/orgs","repos_url":"https://api.github.com/users/shinrich/repos","events_url":"https://api.github.com/users/shinrich/events{/privacy}","received_events_url":"https://api.github.com/users/shinrich/received_events","type":"User","site_admin":false},"assignees":[{"login":"shinrich","id":334488,"node_id":"MDQ6VXNlcjMzNDQ4OA==","avatar_url":"https://avatars0.githubusercontent.com/u/334488?v=4","gravatar_id":"","url":"https://api.github.com/users/shinrich","html_url":"https://github.com/shinrich","followers_url":"https://api.github.com/users/shinrich/followers","following_url":"https://api.github.com/users/shinrich/following{/other_user}","gists_url":"https://api.github.com/users/shinrich/gists{/gist_id}","starred_url":"https://api.github.com/users/shinrich/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/shinrich/subscriptions","organizations_url":"https://api.github.com/users/shinrich/orgs","repos_url":"https://api.github.com/users/shinrich/repos","events_url":"https://api.github.com/users/shinrich/events{/privacy}","received_events_url":"https://api.github.com/users/shinrich/received_events","type":"User","site_admin":false}],"milestone":{"url":"https://api.github.com/repos/apache/trafficserver/milestones/19","html_url":"https://github.com/apache/trafficserver/milestone/19","labels_url":"https://api.github.com/repos/apache/trafficserver/milestones/19/labels","id":3358032,"node_id":"MDk6TWlsZXN0b25lMzM1ODAzMg==","number":19,"title":"9.0.0","description":"","creator":{"login":"bryancall","id":216749,"node_id":"MDQ6VXNlcjIxNjc0OQ==","avatar_url":"https://avatars3.githubusercontent.com/u/216749?v=4","gravatar_id":"","url":"https://api.github.com/users/bryancall","html_url":"https://github.com/bryancall","followers_url":"https://api.github.com/users/bryancall/followers","following_url":"https://api.github.com/users/bryancall/following{/other_user}","gists_url":"https://api.github.com/users/bryancall/gists{/gist_id}","starred_url":"https://api.github.com/users/bryancall/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bryancall/subscriptions","organizations_url":"https://api.github.com/users/bryancall/orgs","repos_url":"https://api.github.com/users/bryancall/repos","events_url":"https://api.github.com/users/bryancall/events{/privacy}","received_events_url":"https://api.github.com/users/bryancall/received_events","type":"User","site_admin":false},"open_issues":53,"closed_issues":147,"state":"open","created_at":"2018-05-18T19:12:59Z","updated_at":"2018-09-25T22:00:12Z","due_on":"2019-06-30T07:00:00Z","closed_at":null},"comments":4,"created_at":"2018-09-06T20:24:18Z","updated_at":"2018-09-25T22:00:41Z","closed_at":"2018-09-20T15:22:27Z","author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/apache/trafficserver/pulls/4212","html_url":"https://github.com/apache/trafficserver/pull/4212","diff_url":"https://github.com/apache/trafficserver/pull/4212.diff","patch_url":"https://github.com/apache/trafficserver/pull/4212.patch"},"body":"<img width=\"292\" alt=\"screen shot 2018-08-31 at 5 12 44 pm\" src=\"https://user-images.githubusercontent.com/334488/45183953-21f5fc00-b1eb-11e8-931a-f5112c02e813.png\">\r\nNow that HTTP/2 priorities are no longer crashing, we did some short experiments activating in production.  However, that resulted in some customer-side latency problems which I have been digging into through the past week.  I have been concentrating on exercising ATS via a continuous streaming site.  I have been taking advantage for browser developer hooks to simulate slower network speeds.\r\n\r\nI found a paper which gave a nice overview of browser priority strategies https://www.researchgate.net/publication/324514529_HTTP2_Prioritization_and_its_Impact_on_Web_Performance . Chrome does a chain of priorities (first come, first served).  The session window limit seems to be hit first.  Firefox uses priority holder nodes to set up 4 or 5 buckets.  Safari seems to do a naive round robin approach where all priority nodes are direct children of the root.  My testing has concentrated on Firefox and Chrome.\r\n\r\nThe PR adds debug messages to dump the state of the priority tree as JSON output which has been very useful for tracking down these issues.\r\n\r\nThis PR has fixes for two problems.\r\n\r\n* For Chrome, I would occasionally see the priority chain be broken.  Originally, the new direct descendent would be a shadow node.  Then I took out the shadow node addition unless the parent_id was in the future and it would just be that new stream as the direct root descendent.  Since the point value of the new node was lower than the others, it would unfairly get a higher priority of the bandwidth (based on the weighted-fair queuing logic) starving out the older nodes and violating Chrome's first come first served policy.  The problem is that the parent of the new node got finished and removed from the priority tree before the stream id request appeared.  I saw this mostly with a high bandwidth client.  I addressed this issue by adding a small circular buffer of ancestors, so if the parent is missing, we can look to see where that node originally plugged in.  It isn't perfect since we are not keeping a complete history, but once I made this change, I no longer saw the problem.  I think in practice you will only have one or two parents missing before the next stream request comes.  \r\n\r\n* For Firefox, we are inserting nodes in the priority tree for priority frames, but we were treating them as shadow nodes.  So once their last direct descendent was deleted, the priority node was also deleted losing the buckets for future requests on the session.  This is not desirable for the Firefox strategy since we are relying on the priority direct root descendants as weight buckets.  So rather than using the fact that there is no associated Http2Stream node to identify a priority node as a shadow (true for shadows and priority frames), I added another flag to explicitly mark a node as a shadow.\r\n\r\nI am still a bit unclear on the use of shadow nodes.  I have commented out a couple of the tests until we reach a conclusion on what the shadows should be.  I did some searching based on our original behavior of creating shadow nodes for unknown parent and found  this discussion https://github.com/http2/http2-spec/issues/764 where Lukasa declares that the right way to do this is to make the new stream for the missing parent and do default priority processing. But in their example, they have stream 6 depending on stream 7 (presumably a future stream). I looked into how nghttp2 does it (which is declared to be the right way) and indeed they create a new node for the missing parent with default weight and root it at the root node.  Based on that discussion, I am assuming that shadow nodes are only to represent nodes that will come in the future (e.g. parent id > new stream id).\r\n\r\nWe are still in the process of testing these changes, but I wanted to get more eyes on this.  Particularly since I was not involved in the original priority code development.\r\n\r\nI also need to augment the unit tests to model the chrome and firefox scenarios.  I hope to test some android apps.  It would also be good to test some iOS apps, but I don't have access to an iOS device."},"comment":{"url":"https://api.github.com/repos/apache/trafficserver/issues/comments/424516405","html_url":"https://github.com/apache/trafficserver/pull/4212#issuecomment-424516405","issue_url":"https://api.github.com/repos/apache/trafficserver/issues/4212","id":424516405,"node_id":"MDEyOklzc3VlQ29tbWVudDQyNDUxNjQwNQ==","user":{"login":"zwoop","id":202131,"node_id":"MDQ6VXNlcjIwMjEzMQ==","avatar_url":"https://avatars1.githubusercontent.com/u/202131?v=4","gravatar_id":"","url":"https://api.github.com/users/zwoop","html_url":"https://github.com/zwoop","followers_url":"https://api.github.com/users/zwoop/followers","following_url":"https://api.github.com/users/zwoop/following{/other_user}","gists_url":"https://api.github.com/users/zwoop/gists{/gist_id}","starred_url":"https://api.github.com/users/zwoop/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/zwoop/subscriptions","organizations_url":"https://api.github.com/users/zwoop/orgs","repos_url":"https://api.github.com/users/zwoop/repos","events_url":"https://api.github.com/users/zwoop/events{/privacy}","received_events_url":"https://api.github.com/users/zwoop/received_events","type":"User","site_admin":false},"created_at":"2018-09-25T22:00:40Z","updated_at":"2018-09-25T22:00:40Z","author_association":"CONTRIBUTOR","body":"@shinrich This will need a new PR against the 7.1.x branch please"}}
{"actor":{"display_login":"zwoop"}}