Add "trim path" option which can be used to relocate sources. #366
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,7 +63,7 @@ func printSource(w io.Writer, rpt *Report) error { | |
| } | ||
| sourcePath = wd | ||
| } | ||
| reader := newSourceReader(sourcePath, o.TrimPath) | ||
|
|
||
| fmt.Fprintf(w, "Total: %s\n", rpt.formatValue(rpt.total)) | ||
| for _, fn := range functions { | ||
|
|
@@ -146,7 +146,7 @@ func PrintWebList(w io.Writer, rpt *Report, obj plugin.ObjTool, maxFiles int) er | |
| } | ||
| sourcePath = wd | ||
| } | ||
| reader := newSourceReader(sourcePath, o.TrimPath) | ||
|
|
||
| type fileFunction struct { | ||
| fileName, functionName string | ||
|
|
@@ -263,7 +263,7 @@ func assemblyPerSourceLine(objSyms []*objSymbol, rs graph.Nodes, src string, obj | |
| // | ||
| // E.g., suppose we are printing source code for F and this | ||
| // instruction is from H where F called G called H and both | ||
| // of those calls were inlined. We want to use the line | ||
| // number from F, not from H (which is what Disasm gives us). | ||
| // | ||
| // So find the outer-most linenumber in the source file. | ||
|
|
@@ -391,16 +391,15 @@ func printFunctionSourceLine(w io.Writer, fn *graph.Node, assembly []assemblyIns | |
| continue | ||
| } | ||
| curCalls = nil | ||
| fline, ok := reader.line(c.file, c.line) | ||
| if !ok { | ||
| fline = "" | ||
| } | ||
| text := strings.Repeat(" ", srcIndent+4+4*j) + strings.TrimSpace(fline) | ||
| fmt.Fprintf(w, " %8s %10s %10s %8s <span class=inlinesrc>%s</span> <span class=unimportant>%s:%d</span>\n", | ||
| "", "", "", "", | ||
| template.HTMLEscapeString(fmt.Sprintf("%-80s", text)), | ||
| template.HTMLEscapeString(filepath.Base(c.file)), c.line) | ||
| } | ||
| curCalls = an.inlineCalls | ||
| text := strings.Repeat(" ", srcIndent+4+4*len(curCalls)) + an.instruction | ||
|
|
@@ -426,7 +425,6 @@ func printPageClosing(w io.Writer) { | |
| // file and annotates it with the samples in fns. Returns the sources | ||
| // as nodes, using the info.name field to hold the source code. | ||
| func getSourceFromFile(file string, reader *sourceReader, fns graph.Nodes, start, end int) (graph.Nodes, string, error) { | ||
| lineNodes := make(map[int]graph.Nodes) | ||
|
|
||
| // Collect source coordinates from profile. | ||
|
|
@@ -516,20 +514,26 @@ func getMissingFunctionSource(filename string, asm map[int][]assemblyInstruction | |
|
|
||
| // sourceReader provides access to source code with caching of file contents. | ||
| type sourceReader struct { | ||
| // searchPath is a filepath.ListSeparator-separated list of directories where | ||
| // source files should be searched. | ||
| searchPath string | ||
|
|
||
| // trimPath is a filepath.ListSeparator-separated list of paths to trim. | ||
| trimPath string | ||
|
|
||
| // files maps from path name to a list of lines. | ||
| // files[*][0] is unused since line numbering starts at 1. | ||
| files map[string][]string | ||
|
|
||
| // errors collects errors encountered per file. These errors are | ||
| // consulted before returning out of these module. | ||
| errors map[string]error | ||
| } | ||
|
|
||
| func newSourceReader(searchPath, trimPath string) *sourceReader { | ||
| return &sourceReader{ | ||
| searchPath, | ||
| trimPath, | ||
| make(map[string][]string), | ||
| make(map[string]error), | ||
| } | ||
|
|
@@ -544,7 +548,7 @@ func (reader *sourceReader) line(path string, lineno int) (string, bool) { | |
| if !ok { | ||
| // Read and cache file contents. | ||
| lines = []string{""} // Skip 0th line | ||
| f, err := openSourceFile(path, reader.searchPath, reader.trimPath) | ||
| if err != nil { | ||
| reader.errors[path] = err | ||
| } else { | ||
|
|
@@ -565,17 +569,20 @@ func (reader *sourceReader) line(path string, lineno int) (string, bool) { | |
| return lines[lineno], true | ||
| } | ||
|
|
||
| // openSourceFile opens a source file from a name encoded in a profile. File | ||
| // names in a profile after can be relative paths, so search them in each of | ||
| // the paths in searchPath and their parents. In case the profile contains | ||
| // absolute paths, additional paths may be configured to trim from the source | ||
| // paths in the profile. This effectively turns the path into a relative path | ||
| // searching it using searchPath as usual). | ||
| func openSourceFile(path, searchPath, trim string) (*os.File, error) { | ||
| path = trimPath(path, trim, searchPath) | ||
| // If file is still absolute, require file to exist. | ||
| if filepath.IsAbs(path) { | ||
| f, err := os.Open(path) | ||
| return f, err | ||
| } | ||
| // Scan each component of the path. | ||
| for _, dir := range filepath.SplitList(searchPath) { | ||
| // Search up for every parent of each possible path. | ||
| for { | ||
|
|
@@ -595,18 +602,34 @@ func openSourceFile(path, searchPath string) (*os.File, error) { | |
| } | ||
|
|
||
| // trimPath cleans up a path by removing prefixes that are commonly | ||
| // found on profiles plus configured prefixes. | ||
| // TODO(aalexand): Consider optimizing out the redundant work done in this | ||
| // function if it proves to matter. | ||
| func trimPath(path, trimPath, searchPath string) string { | ||
| // Keep path variable intact as it's used below to form the return value. | ||
| sPath, searchPath := filepath.ToSlash(path), filepath.ToSlash(searchPath) | ||
| if trimPath == "" { | ||
| // If the trim path is not configured, try to guess it heuristically: | ||
| // search for basename of each search path in the original path and, if | ||
| // found, strip everything up to and including the basename. So, for | ||
| // example, given original path "/some/remote/path/my-project/foo/bar.c" | ||
| // and search path "/my/local/path/my-project" the heuristic will return | ||
| // "/my/local/path/my-project/foo/bar.c". | ||
| for _, dir := range filepath.SplitList(searchPath) { | ||
| want := "/" + filepath.Base(dir) + "/" | ||
| if found := strings.Index(sPath, want); found != -1 { | ||
| return path[found+len(want):] | ||
|
There was a problem hiding this comment. Here we use There was a problem hiding this comment. I think we want path instead of sPath, as we want to return the system path. However, maybe we should use FromSlash(sPath). Not sure if there is a guarantee that in all platforms the path separator is a single character. There was a problem hiding this comment. From https://golang.org/src/path/filepath/path.go?s=4426:4458#L155 and from ToSlash documentation that says "ToSlash returns the result of replacing each separator character in path with a slash ('/') character. Multiple separators are replaced by multiple slashes." I am pretty confident that it's safe to slice the original path using the slashized one as the index. There was a problem hiding this comment. I would prefer to see There was a problem hiding this comment. I think the I agree with @aalexand that the indexes very likely do line up in every case. But I think it is undesirable to have to think so carefully about the correctness of this function. It would be preferable if it was obviously correct. This has the downside of increasing allocations inside the trimPath function. I profiled the pprof tool using such an explicit For reference my method of profiling is run
on an existing server. Then using the web-ui navigate to the There was a problem hiding this comment. I too would prefer to use FromSlash on the return, if only for symmetry. There was a problem hiding this comment. IMO the documentation for ToSlash and FromSlash clearly says that these functions only replace character for character, so I think there is nothing to worry about. Function name is There was a problem hiding this comment. Note that trimPath call is also used to trim the paths before displaying in the graph or source / assembly view, so ToSlash'ing the path here would make Unix paths display with backslash if someone opens on a Windows machine a profile collected on Linux. This is an edge case but I think the behavior as coded now would be more reasonable. There was a problem hiding this comment. That's a good point. Since running |
||
| } | ||
| } | ||
| } | ||
| // Trim configured trim prefixes. | ||
| trimPaths := append(filepath.SplitList(filepath.ToSlash(trimPath)), "/proc/self/cwd/./", "/proc/self/cwd/") | ||
| for _, trimPath := range trimPaths { | ||
| if !strings.HasSuffix(trimPath, "/") { | ||
| trimPath += "/" | ||
| } | ||
| if strings.HasPrefix(sPath, trimPath) { | ||
| return path[len(trimPath):] | ||
| } | ||
| } | ||
| return path | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a little painful doing this search for every filename,
We could remember the heuristic trimPath we've used so far and try to apply them to the next filename before trying the search. I assume in the general case we will only have one.
Up to you, though. We could leave as a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into whether it's worth it, will ping the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I collected a couple of profiles over pprof itself while generating a graph for some larger profiles passing it a number of paths in -source_path and didn't see trimPath to show up substantially (saw something like 0.3% and the number of samples was low). I am also a bit hesitant to implement simple caching just remembering last trimPath since in interactive sessions user could change source path and trim path so those need to be part of the hashing key probably. Just leaving a TODO for now.