Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
382313002:
Adds tile handler to the perf server (Closed)
|
Created:
6 years, 5 months ago by kelvinly Modified:
6 years, 5 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/buildbot.git@master Project:
skiabuildbot Visibility:
Public. |
DescriptionAdds tile handler to the perf server
BUG=skia:
Committed: https://skia.googlesource.com/buildbot/+/ba46b0efb2b5657ef8e82e489a254b0978f15b86
Patch Set 1 #
Total comments: 36
Patch Set 2 : Lots of style fixes #Patch Set 3 : Fixed warning in handler #Patch Set 4 : Ignores empty trace strings #
Total comments: 6
Patch Set 5 : More style fixes #
Total comments: 19
Patch Set 6 : More style fixes #Patch Set 7 : Fixed trace coordinates #
Total comments: 2
Patch Set 8 : Removed comment #
Messages
Total messages: 13 (0 generated)
Running off my machine right now. Caching would be a really nice feature to have I think, but it seems fast enough right now
https://codereview.chromium.org/382313002/diff/1/perf/server/src/config/confi... File perf/server/src/config/config.go (right): https://codereview.chromium.org/382313002/diff/1/perf/server/src/config/confi... perf/server/src/config/config.go:68: HUMAN_READABLE_PARAM_NAMES = make(map[string]string) HUMAN_READABLE_PARAM_NAMES = map[string]string{ "builderName": "Builder name", "rotate": "Rotate", ... } Same for KEY_PARAM_ORDER https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.go File perf/server/src/server/perf.go (right): https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:51: tileHandlerPath = regexp.MustCompile(`/tiles/([a-z]*)/([0-9]*)/([-0-9]*)$`) Since there's more than one parameter here use named captures for the parts: (?P<name>re) https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:255: func tileHandler(w http.ResponseWriter, r *http.Request) { Add documentation to this function on the format of the URLs it accepts with an example or two, including what the responses would look like. https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:263: if r.Method == "GET" { Since we will only accept GET on this and to keep the indentation to a minimum do if r.Method != "GET" { http.NotFound(w, r) return } https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:267: reportError(w, r, fmt.Errorf("Error parsing tile scale: %s", err), "Failed to return traces.") reportError(w, r, err, "Failed parsing tile scale.") Since reportError already combines err and message in the logs. Here and below. https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:275: var tile *types.Tile factor out the following 11 lines of code as its own function: getTile(dataset, tileScale, tileNumber) https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:287: noCommits := len(r.FormValue("omit_commits")) > 0 keep naming consistent, and reverse the logic: includeCommits := r.FormValue("include_commits") == "true" Same for the next two lines. https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:328: glog.Infof("%d matches found for %s\n", count, keyName) Log this as a warning because it indicated a data integrity problem. https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:332: if traceVal < 1e99 { if traceVal != config.MISSING_DATA_SENTINEL { https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:352: newKey := make([]string, len(paramList)) factor out into its own function: makeKeyFromParams(paramList, trace.Params) https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:370: reportError(w, r, fmt.Errorf("%s does not exist in the readable parameter names list", paramList[i]), "Unable to send traces.") Does this have to be an error, how about falling back to using the raw param name but logging it as a warning? https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:373: result.ParamSet[i] = append(make([]string, 0), readableName) result.ParamSet[i] = []string{readableName} https://codereview.chromium.org/382313002/diff/1/perf/server/src/types/types.go File perf/server/src/types/types.go (right): https://codereview.chromium.org/382313002/diff/1/perf/server/src/types/types.... perf/server/src/types/types.go:89: type TraceGUI struct { Add doc comment on where this is used. https://codereview.chromium.org/382313002/diff/1/perf/server/src/types/types.... perf/server/src/types/types.go:90: Data [][2]float64 `json:"data_ui"` Just "data" and "key" for the json should be fine. https://codereview.chromium.org/382313002/diff/1/perf/server/src/types/types.... perf/server/src/types/types.go:91: Key string `json:"key_ui"` Run gofmt over all the code. https://codereview.chromium.org/382313002/diff/1/perf/server/src/types/types.... perf/server/src/types/types.go:94: type TileGUI struct { Add doc comment on where this is used.
https://codereview.chromium.org/382313002/diff/1/perf/server/src/config/confi... File perf/server/src/config/config.go (right): https://codereview.chromium.org/382313002/diff/1/perf/server/src/config/confi... perf/server/src/config/config.go:68: HUMAN_READABLE_PARAM_NAMES = make(map[string]string) On 2014/07/11 19:03:55, jcgregorio wrote: > HUMAN_READABLE_PARAM_NAMES = map[string]string{ > "builderName": "Builder name", > "rotate": "Rotate", > ... > } > > Same for KEY_PARAM_ORDER Done. https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.go File perf/server/src/server/perf.go (right): https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:51: tileHandlerPath = regexp.MustCompile(`/tiles/([a-z]*)/([0-9]*)/([-0-9]*)$`) On 2014/07/11 19:03:55, jcgregorio wrote: > Since there's more than one parameter here use named captures for the parts: > (?P<name>re) Would that serve any purpose other than documenting the groups? I can't seem to find a way to use the captured group names, aside from the cumbersome-looking regexp.Expand() https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:255: func tileHandler(w http.ResponseWriter, r *http.Request) { On 2014/07/11 19:03:55, jcgregorio wrote: > Add documentation to this function on the format of the URLs it accepts with an > example or two, including what the responses would look like. Done. https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:263: if r.Method == "GET" { On 2014/07/11 19:03:55, jcgregorio wrote: > Since we will only accept GET on this and to keep the indentation to a minimum > do > > if r.Method != "GET" { > http.NotFound(w, r) > return > } Done. https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:267: reportError(w, r, fmt.Errorf("Error parsing tile scale: %s", err), "Failed to return traces.") On 2014/07/11 19:03:56, jcgregorio wrote: > reportError(w, r, err, "Failed parsing tile scale.") > > Since reportError already combines err and message in the logs. > > Here and below. Done. https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:275: var tile *types.Tile On 2014/07/11 19:03:55, jcgregorio wrote: > factor out the following 11 lines of code as its own function: > > getTile(dataset, tileScale, tileNumber) Done. https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:287: noCommits := len(r.FormValue("omit_commits")) > 0 On 2014/07/11 19:03:56, jcgregorio wrote: > keep naming consistent, and reverse the logic: > > includeCommits := r.FormValue("include_commits") == "true" > > Same for the next two lines. Including seems like a good default, so I'm keeping the logic as is, but I'll rename to be more consistent https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:328: glog.Infof("%d matches found for %s\n", count, keyName) On 2014/07/11 19:03:55, jcgregorio wrote: > Log this as a warning because it indicated a data integrity problem. Done. Also changed it to trigger only when count > 1 https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:332: if traceVal < 1e99 { On 2014/07/11 19:03:56, jcgregorio wrote: > if traceVal != config.MISSING_DATA_SENTINEL { Using < instead of != https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:352: newKey := make([]string, len(paramList)) On 2014/07/11 19:03:56, jcgregorio wrote: > factor out into its own function: > > makeKeyFromParams(paramList, trace.Params) > Done. https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:370: reportError(w, r, fmt.Errorf("%s does not exist in the readable parameter names list", paramList[i]), "Unable to send traces.") On 2014/07/11 19:03:55, jcgregorio wrote: > Does this have to be an error, how about falling back to using the raw param > name but logging it as a warning? Done. https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:373: result.ParamSet[i] = append(make([]string, 0), readableName) On 2014/07/11 19:03:55, jcgregorio wrote: > result.ParamSet[i] = []string{readableName} Done. https://codereview.chromium.org/382313002/diff/1/perf/server/src/types/types.go File perf/server/src/types/types.go (right): https://codereview.chromium.org/382313002/diff/1/perf/server/src/types/types.... perf/server/src/types/types.go:89: type TraceGUI struct { On 2014/07/11 19:03:56, jcgregorio wrote: > Add doc comment on where this is used. Done. https://codereview.chromium.org/382313002/diff/1/perf/server/src/types/types.... perf/server/src/types/types.go:90: Data [][2]float64 `json:"data_ui"` On 2014/07/11 19:03:56, jcgregorio wrote: > Just "data" and "key" for the json should be fine. Done. https://codereview.chromium.org/382313002/diff/1/perf/server/src/types/types.... perf/server/src/types/types.go:91: Key string `json:"key_ui"` On 2014/07/11 19:03:56, jcgregorio wrote: > Run gofmt over all the code. Done. https://codereview.chromium.org/382313002/diff/1/perf/server/src/types/types.... perf/server/src/types/types.go:94: type TileGUI struct { On 2014/07/11 19:03:56, jcgregorio wrote: > Add doc comment on where this is used. Done.
https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.go File perf/server/src/server/perf.go (right): https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:51: tileHandlerPath = regexp.MustCompile(`/tiles/([a-z]*)/([0-9]*)/([-0-9]*)$`) Oh yeah, the regexp package doesn't handle those very well. Never mind, just add docs to this variable on what each part means. On 2014/07/11 19:42:29, kelvinly wrote: > On 2014/07/11 19:03:55, jcgregorio wrote: > > Since there's more than one parameter here use named captures for the parts: > > (?P<name>re) > > Would that serve any purpose other than documenting the groups? I can't seem to > find a way to use the captured group names, aside from the cumbersome-looking > regexp.Expand() https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:332: if traceVal < 1e99 { Use !=, because that's exactly what is stored there: https://github.com/google/skia-buildbot/blob/master/perf/server/src/server/da... On 2014/07/11 19:42:29, kelvinly wrote: > On 2014/07/11 19:03:56, jcgregorio wrote: > > if traceVal != config.MISSING_DATA_SENTINEL { > > Using < instead of != https://codereview.chromium.org/382313002/diff/60001/perf/server/src/server/p... File perf/server/src/server/perf.go (right): https://codereview.chromium.org/382313002/diff/60001/perf/server/src/server/p... perf/server/src/server/perf.go:43: index2Template *template.Template = nil looks like you're going to need to rebase https://codereview.chromium.org/382313002/diff/60001/perf/server/src/server/p... perf/server/src/server/perf.go:299: reportError(w, r, err, "Failed parsing tile scale.") needs a return https://codereview.chromium.org/382313002/diff/60001/perf/server/src/server/p... perf/server/src/server/perf.go:313: omitCommits := len(r.FormValue("omit_commits")) > 0 Either: omitCommits := r.FormValue("omit_commits") == "true" or omitCommits := r.FormValue("omit_commits") != "" Same for the next two.
https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.go File perf/server/src/server/perf.go (right): https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:51: tileHandlerPath = regexp.MustCompile(`/tiles/([a-z]*)/([0-9]*)/([-0-9]*)$`) On 2014/07/11 20:06:56, jcgregorio wrote: > Oh yeah, the regexp package doesn't handle those very well. > Never mind, just add docs to this variable on what each part means. > > > On 2014/07/11 19:42:29, kelvinly wrote: > > On 2014/07/11 19:03:55, jcgregorio wrote: > > > Since there's more than one parameter here use named captures for the parts: > > > (?P<name>re) > > > > Would that serve any purpose other than documenting the groups? I can't seem > to > > find a way to use the captured group names, aside from the cumbersome-looking > > regexp.Expand() > Done. https://codereview.chromium.org/382313002/diff/1/perf/server/src/server/perf.... perf/server/src/server/perf.go:332: if traceVal < 1e99 { On 2014/07/11 20:06:56, jcgregorio wrote: > Use !=, because that's exactly what is stored there: > > > https://github.com/google/skia-buildbot/blob/master/perf/server/src/server/da... > > On 2014/07/11 19:42:29, kelvinly wrote: > > On 2014/07/11 19:03:56, jcgregorio wrote: > > > if traceVal != config.MISSING_DATA_SENTINEL { > > > > Using < instead of != > Done. Sorry, I'm a bit wary of using any form of equality on floats in general https://codereview.chromium.org/382313002/diff/60001/perf/server/src/server/p... File perf/server/src/server/perf.go (right): https://codereview.chromium.org/382313002/diff/60001/perf/server/src/server/p... perf/server/src/server/perf.go:43: index2Template *template.Template = nil On 2014/07/11 20:06:56, jcgregorio wrote: > looks like you're going to need to rebase Huh, that probably explains why perf's been acting weird when I'm doing work on index2. Done https://codereview.chromium.org/382313002/diff/60001/perf/server/src/server/p... perf/server/src/server/perf.go:299: reportError(w, r, err, "Failed parsing tile scale.") On 2014/07/11 20:06:56, jcgregorio wrote: > needs a return Done. https://codereview.chromium.org/382313002/diff/60001/perf/server/src/server/p... perf/server/src/server/perf.go:313: omitCommits := len(r.FormValue("omit_commits")) > 0 On 2014/07/11 20:06:56, jcgregorio wrote: > Either: > > omitCommits := r.FormValue("omit_commits") == "true" > > or > > omitCommits := r.FormValue("omit_commits") != "" > > > Same for the next two. Done.
https://codereview.chromium.org/382313002/diff/80001/perf/server/src/config/c... File perf/server/src/config/config.go (right): https://codereview.chromium.org/382313002/diff/80001/perf/server/src/config/c... perf/server/src/config/config.go:68: HUMAN_READABLE_PARAM_NAMES = map[string]string{ Sort this list on the first column to make it easier to find names. https://codereview.chromium.org/382313002/diff/80001/perf/server/src/config/c... perf/server/src/config/config.go:69: "builderName": "Builder name", If these are going to be column titles then capitalize both Builder Name https://codereview.chromium.org/382313002/diff/80001/perf/server/src/config/c... perf/server/src/config/config.go:85: "skpSize": "Size of SKP", SKP Size SKP Name https://codereview.chromium.org/382313002/diff/80001/perf/server/src/server/p... File perf/server/src/server/perf.go (right): https://codereview.chromium.org/382313002/diff/80001/perf/server/src/server/p... perf/server/src/server/perf.go:54: // The three capture groups are dataset, tile scale, and tile number and tile number. https://codereview.chromium.org/382313002/diff/80001/perf/server/src/server/p... perf/server/src/server/perf.go:260: func makeKeyFromParams(paramList []string, params map[string]string) string { Need docs https://codereview.chromium.org/382313002/diff/80001/perf/server/src/server/p... perf/server/src/server/perf.go:272: func getTile(dataset string, tileScale, tileNumber int) (*types.Tile, error) { Needs docs https://codereview.chromium.org/382313002/diff/80001/perf/server/src/server/p... perf/server/src/server/perf.go:308: // TODO: Use some sort of cache Move the TODO into getTile. https://codereview.chromium.org/382313002/diff/80001/perf/server/src/server/p... perf/server/src/server/perf.go:391: // TODO: Fix this on tile generation rather than here All comments should be complete sentences, modulo the case where it might not begin with a capital letter if describing a private func or field. https://codereview.chromium.org/382313002/diff/80001/perf/server/src/types/ty... File perf/server/src/types/types.go (right): https://codereview.chromium.org/382313002/diff/80001/perf/server/src/types/ty... perf/server/src/types/types.go:89: // TraceGUI is used in the JSON returned from the tile handler. // used in TileGUI.
https://codereview.chromium.org/382313002/diff/80001/perf/server/src/config/c... File perf/server/src/config/config.go (right): https://codereview.chromium.org/382313002/diff/80001/perf/server/src/config/c... perf/server/src/config/config.go:68: HUMAN_READABLE_PARAM_NAMES = map[string]string{ On 2014/07/11 20:36:48, jcgregorio wrote: > Sort this list on the first column to make it easier to find names. Done. https://codereview.chromium.org/382313002/diff/80001/perf/server/src/config/c... perf/server/src/config/config.go:69: "builderName": "Builder name", On 2014/07/11 20:36:48, jcgregorio wrote: > If these are going to be column titles then capitalize both > > Builder Name It actually looks fairly nice lowercase, but done https://codereview.chromium.org/382313002/diff/80001/perf/server/src/config/c... perf/server/src/config/config.go:85: "skpSize": "Size of SKP", On 2014/07/11 20:36:49, jcgregorio wrote: > SKP Size > SKP Name Done. https://codereview.chromium.org/382313002/diff/80001/perf/server/src/server/p... File perf/server/src/server/perf.go (right): https://codereview.chromium.org/382313002/diff/80001/perf/server/src/server/p... perf/server/src/server/perf.go:54: // The three capture groups are dataset, tile scale, and tile number On 2014/07/11 20:36:49, jcgregorio wrote: > and tile number. Done. https://codereview.chromium.org/382313002/diff/80001/perf/server/src/server/p... perf/server/src/server/perf.go:54: // The three capture groups are dataset, tile scale, and tile number On 2014/07/11 20:36:49, jcgregorio wrote: > and tile number. Done. https://codereview.chromium.org/382313002/diff/80001/perf/server/src/server/p... perf/server/src/server/perf.go:260: func makeKeyFromParams(paramList []string, params map[string]string) string { On 2014/07/11 20:36:50, jcgregorio wrote: > Need docs Done. https://codereview.chromium.org/382313002/diff/80001/perf/server/src/server/p... perf/server/src/server/perf.go:272: func getTile(dataset string, tileScale, tileNumber int) (*types.Tile, error) { On 2014/07/11 20:36:49, jcgregorio wrote: > Needs docs Done. https://codereview.chromium.org/382313002/diff/80001/perf/server/src/server/p... perf/server/src/server/perf.go:308: // TODO: Use some sort of cache On 2014/07/11 20:36:49, jcgregorio wrote: > Move the TODO into getTile. Done. https://codereview.chromium.org/382313002/diff/80001/perf/server/src/server/p... perf/server/src/server/perf.go:391: // TODO: Fix this on tile generation rather than here On 2014/07/11 20:36:49, jcgregorio wrote: > All comments should be complete sentences, modulo the case where it might not > begin with a capital letter if describing a private func or field. Done. https://codereview.chromium.org/382313002/diff/80001/perf/server/src/types/ty... File perf/server/src/types/types.go (right): https://codereview.chromium.org/382313002/diff/80001/perf/server/src/types/ty... perf/server/src/types/types.go:89: // TraceGUI is used in the JSON returned from the tile handler. On 2014/07/11 20:36:50, jcgregorio wrote: > // used in TileGUI. Done.
Fixed how I had the x and y coordinates in the wrong order
LGTM after removing the one comment. https://codereview.chromium.org/382313002/diff/110001/perf/server/src/server/... File perf/server/src/server/perf.go (right): https://codereview.chromium.org/382313002/diff/110001/perf/server/src/server/... perf/server/src/server/perf.go:293: // So this almost doubles the size of this file. Should I move it into its own package? Drop this comment, we can leave this in this file for now.
https://codereview.chromium.org/382313002/diff/110001/perf/server/src/server/... File perf/server/src/server/perf.go (right): https://codereview.chromium.org/382313002/diff/110001/perf/server/src/server/... perf/server/src/server/perf.go:293: // So this almost doubles the size of this file. Should I move it into its own package? On 2014/07/14 15:23:13, jcgregorio wrote: > Drop this comment, we can leave this in this file for now. Done.
The CQ bit was checked by kelvinly@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/kelvinly@google.com/382313002/130001
Message was sent while issue was closed.
Change committed as ba46b0efb2b5657ef8e82e489a254b0978f15b86 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
