Closed Bug 1329111 Opened 8 years ago Closed 8 years ago

Clean up shared library information value

Categories

(Core :: Gecko Profiler, defect)

Product:
Core
Component:
Gecko Profiler
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact none
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

Bug 1329111 - Use JSONWriter when generating the GetSharedLibraryInfo() JSON.
(deleted), text/x-review-board-request
n.nethercote
: review+
Details
Bug 1329111 - Rename SharedLibraryInfo::mName to mModuleName, and consistently cut off the path on all platforms.
(deleted), text/x-review-board-request
marco
: review+
Details
Bug 1329111 - Supply SharedLibraryInfo with the absolute module path and debug path on all platforms.
(deleted), text/x-review-board-request
marco
: review+
Details
Bug 1329111 - Record shared library architecture on Mac.
(deleted), text/x-review-board-request
glandium
: review+
Details
Bug 1329111 - Make talos profiling symbolication compatible with the new profile format.
(deleted), text/x-review-board-request
jmaher
: review+
Details
Bug 1329111 - Change the nsIProfiler shared library information API.
(deleted), text/x-review-board-request
n.nethercote
: review+
Details
Bug 1329111 - Update devtools users of nsIProfiler.getSharedLibraryInformation.
(deleted), text/x-review-board-request
gregtatum
: review+
Details
nsIProfiler::GetSharedLibraryInformation returns a string containing JSON instead of the actual array. nsIProfiler::GetProfileDataAsync resolves with a JS profile object whose "libs" property is also such a JSON string. Users of these APIs shouldn't have to parse the JSON themselves. Moreover, the list of libraries isn't sorted by library start address. It should be. On Mac + Linux, the lib objects' "name" property contains the full path to the binary file. On Windows it only contains the filename ("something.pdb") and no (back)slashes. This should be unified: Maybe with a "pdbName" field that's just the file name, and a "path" field that contains the full path. Also, at least on Mac, the list only contains the mappings of the __TEXT sections, not of the __DATA sections, so if users have vtable pointers that they want to symbolicate, they have to guess which shared library those are from because they won't be within any start-end range.
The other piece of information that's currently missing is the architecture of the binary that's loaded. On Mac there can be multiple architectures stored in the same file, and symbolication needs to know which image to look at. For example, on 10.12, CoreFoundation contains the architectures "i386", "x86_64", and "x86_64h". Symbolication needs to know that it needs to dump symbols for "x86_64h" and not for "x86_64". Currently it doesn't know that, which causes https://github.com/devtools-html/Gecko-Profiler-Addon/issues/29 .
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Blocks: 1329888
(In reply to Markus Stange [:mstange] from comment #0) > Also, at least on Mac, the list only contains the mappings of the __TEXT > sections, not of the __DATA sections, so if users have vtable pointers that > they want to symbolicate, they have to guess which shared library those are > from because they won't be within any start-end range. This part is not fixed by the patches that I attached, and we don't need it at the moment.
Comment on attachment 8846362 [details] Bug 1329111 - Use JSONWriter when generating the GetSharedLibraryInfo() JSON. https://reviewboard.mozilla.org/r/119412/#review121300 Ha, yes, let's not hand-write the JSON. ::: tools/profiler/core/platform.cpp:1043 (Diff revision 1) > -AddSharedLibraryInfoToStream(std::ostream& aStream, const SharedLibrary& aLib) > +AddSharedLibraryInfoToStream(JSONWriter& aWriter, const SharedLibrary& aLib) > { > - aStream << "{"; > - aStream << "\"start\":" << aLib.GetStart(); > - aStream << ",\"end\":" << aLib.GetEnd(); > - aStream << ",\"offset\":" << aLib.GetOffset(); > + aWriter.StartObjectElement(); > + aWriter.DoubleProperty("start", aLib.GetStart()); > + aWriter.DoubleProperty("end", aLib.GetEnd()); > + aWriter.DoubleProperty("offset", aLib.GetOffset()); These three are all uintptr_t. Would IntProperty() be more suitable than DoubleProperty()?
Attachment #8846362 - Flags: review?(n.nethercote) → review+
Comment on attachment 8846366 [details] Bug 1329111 - Record shared library architecture on Mac. https://reviewboard.mozilla.org/r/119420/#review121302 I'm no expert on the profiler's data format but these changes all seem reasonable. Thanks for the detailed log message. ::: commit-message-60cba:26 (Diff revision 1) > + - The array of libraries is sorted by start address. > + - Each library has a "name" field that contains the binary file's basename, > + on all platforms. > + - Each library has a "path" field that contains the full path to the binary, > + on all platforms. > + - Each library has a "debugName" field that contains the library's debug Nit: two spaces before "debugName". ::: commit-message-60cba:28 (Diff revision 1) > + on all platforms. > + - Each library has a "path" field that contains the full path to the binary, > + on all platforms. > + - Each library has a "debugName" field that contains the library's debug > + name, on all platforms. > + - Each library has an "arch" field that is either an empty string (Linux + Nit: two spaces before "arch". ::: commit-message-60cba:30 (Diff revision 1) > + on all platforms. > + - Each library has a "debugName" field that contains the library's debug > + name, on all platforms. > + - Each library has an "arch" field that is either an empty string (Linux + > + Windows) or the library's architecture; it'll differentiate between the > + architectures "x86_64" and "x86_64h". What is x86_64h? (A quick google didn't show up anything helpful.) ::: tools/profiler/gecko/nsIProfiler.idl:82 (Diff revision 1) > double getElapsedTime(); > > /** > - * Returns a JSON string of an array of shared library objects. > - * Every object has three properties: start, end, and name. > + * Contains an array of shared library objects. > + * Every object has the properties: start, end, offset, name, path, debugName > + * and breakpadId. Nit: no mention of "arch". ::: tools/profiler/gecko/nsIProfiler.idl:84 (Diff revision 1) > /** > - * Returns a JSON string of an array of shared library objects. > - * Every object has three properties: start, end, and name. > + * Contains an array of shared library objects. > + * Every object has the properties: start, end, offset, name, path, debugName > + * and breakpadId. > * start and end are integers describing the address range that the library > - * occupies in memory. name is the path of the library as a string. > + * occupies in memory. path is the path of the library as a string. Nit: is it worth briefly describing all of the fields? It's not obvious to me what the difference is between "name" and "debugName", for example. (The log message for this commit has good detail about all of them that you could perhaps copy.)
Attachment #8846366 - Flags: review?(n.nethercote) → review+
Comment on attachment 8846367 [details] Bug 1329111 - Make talos profiling symbolication compatible with the new profile format. https://reviewboard.mozilla.org/r/119422/#review121308 ::: testing/talos/talos/profiler/symbolication.py:334 (Diff revision 1) > libs_with_symbols[lib["start"]] = { > "library": lib, "symbols": set()} > libs_with_symbols[lib["start"]]["symbols"].add(address) > return libs_with_symbols.values() > > def _module_from_lib(self, lib): I don't believe we call _module_from_lib() anymore can we just remove this?
Attachment #8846367 - Flags: review?(jmaher) → review+
Comment on attachment 8846363 [details] Bug 1329111 - Rename SharedLibraryInfo::mName to mModuleName, and consistently cut off the path on all platforms. LGTM, but there is another place in Telemetry.cpp that has to be changed in the same way: https://dxr.mozilla.org/mozilla-central/rev/4ceb9062ea8f4113bfd1b3536ace4a840a72faa7/toolkit/components/telemetry/Telemetry.cpp#3230.
Attachment #8846363 - Flags: review?(mcastelluccio) → review+
Comment on attachment 8846364 [details] Bug 1329111 - Supply SharedLibraryInfo with the absolute module path and debug path on all platforms. https://reviewboard.mozilla.org/r/119416/#review121310
Attachment #8846364 - Flags: review?(mcastelluccio) → review+
I reconsidered and concluded that it would be nice to have a "debugPath" field as well, to get the absolute path of the pdb file on Windows.
Attachment #8846365 - Attachment is obsolete: true
Attachment #8846365 - Flags: review?(aklotz)
Comment on attachment 8846362 [details] Bug 1329111 - Use JSONWriter when generating the GetSharedLibraryInfo() JSON. https://reviewboard.mozilla.org/r/119412/#review121300 > These three are all uintptr_t. Would IntProperty() be more suitable than DoubleProperty()? Yes it would. For some reason I didn't find IntProperty when I looked last time. I also added some code that explicitly sets values to -1 when they are too large to be expressed as a JavaScript number.
Comment on attachment 8846367 [details] Bug 1329111 - Make talos profiling symbolication compatible with the new profile format. https://reviewboard.mozilla.org/r/119422/#review121308 > I don't believe we call _module_from_lib() anymore can we just remove this? Good catch. Yes we can. I also removed the "re" import because it's no longer needed.
Comment on attachment 8846832 [details] Bug 1329111 - Change the nsIProfiler shared library information API. https://reviewboard.mozilla.org/r/119826/#review121726 ::: commit-message-3b5a6:37 (Diff revision 3) > + same. > + - Each library has an "arch" field that is either an empty string (Linux + > + Windows) or the library's architecture; it'll differentiate between the > + architectures "x86_64" and "x86_64h". (x86_64h is used for binaries that > + contain instructions that are specific to the Intel Haswell > + microarchitecture. Nit: missing ')'. ::: tools/profiler/gecko/nsIProfiler.idl:84 (Diff revision 3) > /** > - * Returns a JSON string of an array of shared library objects. > - * Every object has three properties: start, end, and name. > - * start and end are integers describing the address range that the library > - * occupies in memory. name is the path of the library as a string. > - * > + * Contains an array of shared library objects. > + * Every object has the properties: > + * - start: The start address of the memory region occupied by this library. > + * - end: The end address of the memory region occupied by this library. > + * - offset: Usually zero, except on android if the region was mapped from Nit: 'A' at the start of 'Android'. ::: tools/profiler/gecko/nsIProfiler.idl:94 (Diff revision 3) > + * - debugName: On Windows, the name of the pdb file for the binary. On other > + * platforms, the same as |name|. > + * - debugPath: On Windows, the full absolute path of the pdb file for the > + * binary. On other platforms, the same as |path|. > + * - arch: On Mac, the name of the architecture that identifies the right > + * binary image of a fat_binary. Example values are "i386", "x86_64", Nit: "fat_binary" -- is the underscore deliberate?
Attachment #8846832 - Flags: review?(n.nethercote) → review+
Comment on attachment 8846832 [details] Bug 1329111 - Change the nsIProfiler shared library information API. https://reviewboard.mozilla.org/r/119826/#review121726 > Nit: "fat_binary" -- is the underscore deliberate? I've removed it. The mach structures that I was thinking of are called fat_header and fat_arch, and I just misremembered and thought fat_binary was also among them.
Comment on attachment 8846363 [details] Bug 1329111 - Rename SharedLibraryInfo::mName to mModuleName, and consistently cut off the path on all platforms. https://reviewboard.mozilla.org/r/119414/#review121778
Attachment #8846363 - Flags: review?(mcastelluccio) → review+
Comment on attachment 8846366 [details] Bug 1329111 - Record shared library architecture on Mac. https://reviewboard.mozilla.org/r/119420/#review121874 ::: tools/profiler/public/shared-libraries.h:35 (Diff revision 5) > const nsString& aModuleName, > const nsString& aModulePath, > const nsString& aDebugName, > const nsString& aDebugPath, > - const std::string& aVersion) > + const std::string& aVersion, > + const char* aArch) Why not put a default value here (like NULL), such that you don't need to change the callers except on mac where you need to pass an explicit value?
Attachment #8846366 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8846366 [details] Bug 1329111 - Record shared library architecture on Mac. https://reviewboard.mozilla.org/r/119420/#review121874 > Why not put a default value here (like NULL), such that you don't need to change the callers except on mac where you need to pass an explicit value? I've gotten pushback every time I've used default parameter values in the past, so I decided to not use them here. This is the first time the review feedback has been in the other direction :) I think I'm going to leave it like this, for consistency with the aVersion argument.
Attachment #8847322 - Flags: review?(gtatum)
Whiteboard: [qf-]
Comment on attachment 8847322 [details] Bug 1329111 - Update devtools users of nsIProfiler.getSharedLibraryInformation. https://reviewboard.mozilla.org/r/120318/#review122916 What are your thoughts on backwards compatibility? We're changing the actor API, so that if a newer client connects to an older one, it will fail because the sharedLibraries and getSharedLibraryInformation methods don't match. I have a feeling we are going to be changing the actor and profile a decent amount right now, so this will be a pretty hard thing to remain backwards compatible. I'm r+ing this because I think it's ok to just move forward on this, especially since we have no tests in devtools for enforcing the backwards compatibility. We could have some people running into errors here with this changing, so there is a potential negative user impact. What are your thoughts?
Attachment #8847322 - Flags: review?(gtatum) → review+
Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/ff2306d8e07f Rename SharedLibraryInfo::mName to mModuleName, and consistently cut off the path on all platforms. r=marco https://hg.mozilla.org/integration/autoland/rev/2656a5fa88a2 Supply SharedLibraryInfo with the absolute module path and debug path on all platforms. r=marco https://hg.mozilla.org/integration/autoland/rev/54d438799138 Record shared library architecture on Mac. r=glandium,njn https://hg.mozilla.org/integration/autoland/rev/2f4e6242c90c Use JSONWriter when generating the GetSharedLibraryInfo() JSON. r=njn https://hg.mozilla.org/integration/autoland/rev/eaac943ff2f6 Change the nsIProfiler shared library information API. r=njn https://hg.mozilla.org/integration/autoland/rev/8addfd2d7a2f Make talos profiling symbolication compatible with the new profile format. r=jmaher https://hg.mozilla.org/integration/autoland/rev/c823541813c2 Update devtools users of nsIProfiler.getSharedLibraryInformation. r=gregtatum
Comment on attachment 8847322 [details] Bug 1329111 - Update devtools users of nsIProfiler.getSharedLibraryInformation. https://reviewboard.mozilla.org/r/120318/#review122916 I don't have many thoughts on it because I don't completely understand the scenarios in which is this going to be a problem. When does a newer client connect to an older one? Are there "same API" checks that are performed on connection, or will API mismatches only cause problems once something actually tries to invoke an API that has been removed? For getSharedLibraryInformation, my thinking was that we don't really have to worry about it yet because the only consumer of the getSharedLibraryInformation is the old add-on, which is easy to fix and should be used extremely rarely these days. For future API changes, I don't know what we should do.
Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/4d074b176f37 Rename SharedLibraryInfo::mName to mModuleName, and consistently cut off the path on all platforms. r=marco https://hg.mozilla.org/integration/autoland/rev/8a238c780c47 Supply SharedLibraryInfo with the absolute module path and debug path on all platforms. r=marco https://hg.mozilla.org/integration/autoland/rev/cdff9dc4a8ba Record shared library architecture on Mac. r=glandium,njn https://hg.mozilla.org/integration/autoland/rev/6cb351c611e3 Use JSONWriter when generating the GetSharedLibraryInfo() JSON. r=njn https://hg.mozilla.org/integration/autoland/rev/33aa51a338b1 Change the nsIProfiler shared library information API. r=njn https://hg.mozilla.org/integration/autoland/rev/fd8fc09c8229 Make talos profiling symbolication compatible with the new profile format. r=jmaher https://hg.mozilla.org/integration/autoland/rev/7e4f8900fa8c Update devtools users of nsIProfiler.getSharedLibraryInformation. r=gregtatum
Flags: needinfo?(mstange)
Depends on: 1350503
Performance Impact: --- → -
Whiteboard: [qf-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: