Dependency tree and stream prioritization #39

Closed
wants to merge 28 commits into
from

Conversation

Projects
None yet
8 participants
@brk0v

brk0v commented Apr 7, 2015

Initial dependency tree and stream prioritization implementation.

Model of the tree is the same as used in nghttp2. It makes possible to implement flow control and stream prioritization with different traversing technics (e.g depth-first search).

Each stream struct has devPrev, devNext, sibPrev and sibNext for pointing its place in the dependency tree. Also there is a roots struct that contains all root streams for connection.

Example of a tree.

                             a(1)
                       _______________
                      | depPrev = nil |
                      | depNext = b   |
                      | sibPrev = nil |
                      | sibNext = nil |
                      |_______________|
                     /        |        \
                    /         |         \
         b(3)      /     c(5) |          \       i(17)
   _______________/     ______|________   \ _______________
  | depPrev = a   |    | depPrev = nil |   | depPrev = nil |
  | depNext = d   |    | depNext = nil |   | depNext = nil |
  | sibPrev = nil |    | sibPrev = b   |   | sibPrev = c   |
  | sibNext = c   |    | sibNext = i   |   | sibNext = nil |
  |_______________|    |_______________|   |_______________|
          |
     d(7) |
   _______|________
  | depPrev = b   |
  | depNext = nil |
  | sibPrev = nil |
  | sibNext = nil |
  |_______________|

More examples you can find in tests: dependency_test.go.

Features

  • Dependency tree model;
  • reprioritization in runtime;
  • scheduling according to priorities, dependency tree and length of a DATA message;
  • support stream grouping node for flexible expressions of priority;
  • retain stream prioritization state for a period after streams become closed.

Stream grouping node allows to create dependency tree with 5 fixed dependency groups (known informally as leader, follower, unblocked, background, and speculative). Read more about it:

Todo

  • add more tests.

DanielMorsing and others added some commits Dec 6, 2014

unify Headers and Push Promise writes.
As predicted, I ended up unifying the Headers and Push Promise packing.

This is in preparation for reusing the writeResHeaders functionality
of sending continuation frames if the headers are too big.
initial implementation of server push
The API uses loopback to the top handler in order to make sure that
the resources being pushed are related to the resource being fetched.

If the header parameter is nil, we copy the headers from the initiating
request. This is mostly a shortcut for the common case where we don't
want to specify any new request headers.
Improve server push logic: add client concurrent counter, set defalt …
…client concurrent streams to 100 and make some code refactoring from DanielMorsing.
* refactoring: delete push field from stream structure;
* add default priority and dependency for push stream;
* add check in processData function for push stream.

@brk0v brk0v changed the title from add dependency tree initial logic to Dependency tree and stream prioritization Apr 28, 2015

@igrigorik

This comment has been minimized.

Show comment
Hide comment
@igrigorik

igrigorik Apr 28, 2015

Minor docs nitpick:

http2/server.go

Line 701 in 91f8030

// change when priority is implemented, so the serve goroutine knows
- still seems to indicate that prioritization is not supported.

Update: Gah, nm! Somehow, I thought this already landed.. and hope it will!

Minor docs nitpick:

http2/server.go

Line 701 in 91f8030

// change when priority is implemented, so the serve goroutine knows
- still seems to indicate that prioritization is not supported.

Update: Gah, nm! Somehow, I thought this already landed.. and hope it will!

@brk0v

This comment has been minimized.

Show comment
Hide comment
@brk0v

brk0v Apr 30, 2015

@bradfitz can you please explain how you see following:

// writeDataFromHandler writes the data described in req to stream.id.
//
// The provided ch is used to avoid allocating new channels for each
// write operation. It's expected that the caller reuses writeData and ch
// over time.
//
// The flow control currently happens in the Handler where it waits
// for 1 or more bytes to be available to then write here.  So at this
// point we know that we have flow control. But this might have to
// change when priority is implemented, so the serve goroutine knows
// the total amount of bytes waiting to be sent and can can have more
// scheduling decisions available.

Maybe some examples of how we should use this info? Now for scheduling decisions I use data from writeSched only. Is it not optimal solution? How I can improve it?

Thank you.

brk0v commented Apr 30, 2015

@bradfitz can you please explain how you see following:

// writeDataFromHandler writes the data described in req to stream.id.
//
// The provided ch is used to avoid allocating new channels for each
// write operation. It's expected that the caller reuses writeData and ch
// over time.
//
// The flow control currently happens in the Handler where it waits
// for 1 or more bytes to be available to then write here.  So at this
// point we know that we have flow control. But this might have to
// change when priority is implemented, so the serve goroutine knows
// the total amount of bytes waiting to be sent and can can have more
// scheduling decisions available.

Maybe some examples of how we should use this info? Now for scheduling decisions I use data from writeSched only. Is it not optimal solution? How I can improve it?

Thank you.

stream.go
+}
+
+func (ds streamDepState) String() string {
+ return depStateName[ds]

This comment has been minimized.

@nightlyone

nightlyone Apr 30, 2015

maybe a range check like this:

    if 0 <= ds && ds < streamDepState(len(depStateName)) {
        return depStateName[ds]
    }
    return fmt.Sprintf("http2: unknown streamDepState %d", ds)

here in order to handle it like this: http://play.golang.org/p/EIhDuHDq9n instead of panic might ease debugging?

@nightlyone

nightlyone Apr 30, 2015

maybe a range check like this:

    if 0 <= ds && ds < streamDepState(len(depStateName)) {
        return depStateName[ds]
    }
    return fmt.Sprintf("http2: unknown streamDepState %d", ds)

here in order to handle it like this: http://play.golang.org/p/EIhDuHDq9n instead of panic might ease debugging?

This comment has been minimized.

@brk0v

brk0v May 2, 2015

Thank you. I'll add this.

@brk0v

brk0v May 2, 2015

Thank you. I'll add this.

+// handler, this struct intentionally has no pointer to the
+// *responseWriter{,State} itself, as the Handler ending nils out the
+// responseWriter's state field.
+type stream struct {

This comment has been minimized.

@nightlyone

nightlyone Apr 30, 2015

Some of this data could be an embedded struct using a copy(from) method instead. Since they are not exported, this seems safe and can be changed later easily. But it certainly makes it easier to understand ownership and lifetime.

@nightlyone

nightlyone Apr 30, 2015

Some of this data could be an embedded struct using a copy(from) method instead. Since they are not exported, this seems safe and can be changed later easily. But it certainly makes it easier to understand ownership and lifetime.

dependency_test.go
+ },
+ streamTest{
+ st: g,
+ depPrev

This comment has been minimized.

@nightlyone

nightlyone Apr 30, 2015

maybe build the tree more explicitly with somthing like StreamDep : a.id, here? That might help when inserting/removing lines in the tests.

@nightlyone

nightlyone Apr 30, 2015

maybe build the tree more explicitly with somthing like StreamDep : a.id, here? That might help when inserting/removing lines in the tests.

@mholt mholt referenced this pull request in mholt/caddy Jun 15, 2015

Closed

needs better explanation of http2 support #144

@NervosaX NervosaX referenced this pull request in jspm/jspm-cli Aug 6, 2015

Open

Performance with many files #872

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Sep 24, 2015

Owner

This repo just moved into the official Go repo.

See https://github.com/bradfitz/http2/blob/master/README for the move details.

File new bugs at: https://github.com/golang/go/issues/new?title=x/net/http2:+

Closing this PR as we're now using Gerrit for code review. Please file a bug or send the review to Gerrit if this is still relevant. Sorry for how long this languished here.

Owner

bradfitz commented Sep 24, 2015

This repo just moved into the official Go repo.

See https://github.com/bradfitz/http2/blob/master/README for the move details.

File new bugs at: https://github.com/golang/go/issues/new?title=x/net/http2:+

Closing this PR as we're now using Gerrit for code review. Please file a bug or send the review to Gerrit if this is still relevant. Sorry for how long this languished here.

@bradfitz bradfitz closed this Sep 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment