Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/net/http2/hpack: Write customer-header into HPACK byName Map rather than byNameValue #20574

Open
Zeymo opened this issue Jun 5, 2017 · 10 comments

Comments

@Zeymo
Copy link

@Zeymo Zeymo commented Jun 5, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.8

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Applications/workspace/code/MWCS"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/8r/y0x9w5g51k91thk9tk0xzgsc0000gn/T/go-build587864376=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

we use http2 as gateway platform protocol and extension header like mw-xxx to do some gateway sub-protocl as request and response.
as some resposne header, "mw-server-time" or "mw-rt", which has static name and dynamic value per request ,accordding to HPACK,these entry will add dynamic table like below

        t.byName[f.Name] = id
	t.byNameValue[pairNameValue{f.Name, f.Value}] = id

and which take 23% alloc_objects and much map hash grow in pprof

What did you expect to see?

we want only use byName map to index name rather than indexing name-value
and never indexing

seperate addEntry such as blew

func (t *headerFieldTable) addEntryByName(f HeaderField) {
        id := uint64(t.len()) + t.evictCount + 1
	t.byName[f.Name] = id
	t.ents = append(t.ents, f)
}

func (t *headerFieldTable) addEntryByNameVlaue(f HeaderField) {
        id := uint64(t.len()) + t.evictCount + 1
	t.byNameValue[pairNameValue{f.Name, f.Value}] = id
	t.ents = append(t.ents, f)
}

and also table.search

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 5, 2017

Sorry, I don't really understand the problem so I don't understand how your proposal to modify internal code helps.

Are you asking for new API, or is this purely an optimization request?

Do you have a self-contained minimal example program to demonstrate the issue?

/cc @tombergan

Loading

@bradfitz bradfitz changed the title Write customer-header into HPACK byName Map rather than byNameValue x/net/http2/hpack: Write customer-header into HPACK byName Map rather than byNameValue Jun 5, 2017
@gopherbot gopherbot added this to the Unreleased milestone Jun 5, 2017
@Zeymo
Copy link
Author

@Zeymo Zeymo commented Jun 5, 2017

@bradfitz Sorry for my poor description.
I mean server will always return server-time(mw-st) and current-request-rt (mw-rt)in http2 header which will write into hpack.HeaderFiled like

for k, v := range md {
   for _, entry := range v {
         // mw-st = 1496630402035
         // mw-rt = 10(ms)
        transport.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
  }
}

both name (mw-st) and value (1496630402035) will add into t. byName and t.byNameValue when sensitive is false default

name will insert into

t.byName[f.Name] = id

and value which change per response also indexing

t.byNameValue[pairNameValue{f.Name, f.Value}] = id

which make t.byNameValue grow and []HeaderField evict by time goes

Is it possiable for new API seperated t.byName and t.byNameValue insert-action apart ,to help entry which has static name and dynamic value only insert t.byName and discard pairNameValue{f.Name, f.Value} to slowdown map grow , improving hit rate for other name-value indexing?

Loading

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 5, 2017

There were changes to the hpack code recently, I think affecting the code you're talking about. Can you try Go tip? Or using golang.org/x/net/http2 directly, instead of the version bundled into std?

Loading

@Zeymo
Copy link
Author

@Zeymo Zeymo commented Jun 5, 2017

@bradfitz I use x/net/http2 directly as vendor already since I customized the gRPC and don't want to change x/net/http2 lib behavior privately : p

Loading

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 5, 2017

Please give us a complete Go program demonstrating the problem from a user's perspective, without referencing hpack internals.

Loading

@tombergan
Copy link
Contributor

@tombergan tombergan commented Jun 5, 2017

I think I mostly understand the request:

HPACK has a dynamic table that is a list of "header fields", where each header field is a name/value pair. Suppose we have the following sequence of header fields:

  1. mw-st: 1
  2. mw-st: 2
  3. mw-st: 2

In HPACK, the first field could be encoded as in Figure 7, the second field could use an index to the first field's name as in Figure 6, and the third field could use an index to the second field's name/value pair as in Figure 5 (https://tools.ietf.org/html/rfc7541#section-6.2.1). Go's HPACK library already does this.

I believe @Zeymo is asking for an additional feature: In the above case, we know that duplicate mw-st values are impossible -- specifically, case 3 is impossible -- so there's no reason to add mw-st: 1 or mw-st: 2 into the byNameValue mapping. No future name/value pair will match either of those fields. This would save memory and GC time by not allocating useless pairNameValue structs. (How much memory and GC time, I have no idea.)

In any case, this would be simple to support if we wanted to: add a boolean field IndexNameOnly to hpack.HeaderField. When true, the field will be added to byName but not byNameValue.

Loading

@tombergan
Copy link
Contributor

@tombergan tombergan commented Jun 5, 2017

A shorter-term option is to set HeaderField.Sensitive = true. This won't compress header field names, but if the names are actually short (like mw-st), you're not getting much from header name compression anyway.

Loading

@Zeymo
Copy link
Author

@Zeymo Zeymo commented Jun 5, 2017

@tombergan absolutely right. Although filed like mw-st , mw-trace-id or mw-cookie actually short which duplicate values are impossible ,but filed like these at least 5~8 are always return in HeaderFrame per response for app full chain metrics. and also for request HeaderFrame in same scenario
now , I already set senstive=true for shorter-term

Loading

@rs
Copy link
Contributor

@rs rs commented Dec 13, 2018

What about adding the ability to control which headers are set a Sensitive at the http2.Transport level? We could expose a new parameter as follow:

type Transport struct {
   ...
    SensitiveHeader func(h hpack.HeaderField) bool
}

This function would be called byClientConn.writeHeader determine if the Sensitive field should be set.

This would give the opportunity to define which header values should be indexed or not. Always returning true, would disable name/value indexing altogether as requested by @Zeymo.

Typical use-case for this is when the building a gateway that can multiplex requests from different users to the same backend connection. In this context, unique per-user headers like cookie can thrash the table to the extent that indexing becomes counter-productive.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 18, 2018

Change https://golang.org/cl/154917 mentions this issue: http2: add hpack header options control on Transport

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants