Closed Bug 733792 Opened 9 years ago Closed 9 years ago

Leave library finding up to the user of nsIProfile; don't assign addresses to libraries in nsProfiler.

Categories

(Core :: Gecko Profiler, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(2 files)

Profiles with real stack traces are very big because we put the library name after each symbol. If instead we reported only the original symbol addresses, the user of nsIProfiler (e.g. the addon) could do their own library finding and profiles would be smaller.
Doing things that way would also solve bug 731551 more elegantly.
Using a JSON string is a bit of a hack, but in the addon we're going to send the result of this to a worker thread right away anyway, so creating a new SharedLibrary IDL interface is probably not worth the effort.
Attachment #603746 - Flags: review?(bgirard)
This is an incompatible change to the profile format. Are there any other consumers apart from the extension?
Attachment #603748 - Flags: review?(bgirard)
Here are the necessary changes to the profiler addon:
https://github.com/mstange/Gecko-Profiler-Addon/commit/6e06c1609ddf8c6c82905309fa8ed4d1850ae8bc

I haven't done the work necessary to keep the addon compatible with the old format; those changes will make it break when used on a build without the patches from this bug.
(In reply to Markus Stange from comment #0)
> Profiles with real stack traces are very big because we put the library name
> after each symbol. If instead we reported only the original symbol
> addresses, the user of nsIProfiler (e.g. the addon) could do their own
> library finding and profiles would be smaller.
> Doing things that way would also solve bug 731551 more elegantly.

I've discussed using an optimized format for profile but Jeff gave a convincing argument that we should just use compression which will likely give us better size reduction without the complexity. Although I'm not opposed to leaving the library translation to the consumer (addon) since it give additional information to the consumer.

About compatibility, this is the primary reason we haven't announced the feature yet on blogs and dev.platform, we want to keep a window where we don't have keep backwards compatibility to iterate and improve the APIs easily without keeping backwards compatibility crud. Once we're satisfied we will announce this tool. As long as we land the addon+platform changes for the same nightly I'm happy.
(In reply to Benoit Girard (:BenWa) from comment #4)
> I've discussed using an optimized format for profile but Jeff gave a
> convincing argument that we should just use compression which will likely
> give us better size reduction without the complexity.

Sure. But we can only use compression after we've constructed the initial profile string. For large profiles I've seen a lot of time spent in memory allocation during GetProfile() while building the string; that would be avoided by making the profile smaller to start with.

> About compatibility, this is the primary reason we haven't announced the
> feature yet on blogs and dev.platform, we want to keep a window where we
> don't have keep backwards compatibility to iterate and improve the APIs
> easily without keeping backwards compatibility crud. Once we're satisfied we
> will announce this tool. As long as we land the addon+platform changes for
> the same nightly I'm happy.

Great.
Attachment #603748 - Flags: review?(bgirard) → review+
Comment on attachment 603746 [details] [diff] [review]
part 1: add nsIProfiler::getSharedLibraryInformation

We currently have a bug that we don't account that a library can be loaded into multiple segments using the offset field from proc map. I think this is the ideal time to fix it since we're going to be breaking compatibility here.

To fix it we need to also return the offset. As I understand it is, it is possible to get something like this:

libxul start: 0x1000 end 0x1050 offset 0x0
libxul start: 0x2000 end 0x3000 offset 0x50

We also need to add a library UUID for at least window since well we using a symbolication server but we can address that in a follow up bug since we will just be modifying a JSON string.
Attachment #603746 - Flags: review?(bgirard) → review-
Actually, if you rely on external symbols, all you need is the base address at which libraries are loaded. By the way, as written on irc, /proc/pid/maps should be a last resort way to get libraries info. DT_DEBUG information is the way to go, and will give you accurate information about libraries loaded by our linker on android.

However, I wonder if it wouldn't make sense to get that info from the crashreporter api. (and bug 689178 is going to make breakpad use DT_DEBUG)
Okay, I have no idea what I'm doing here. I'll leave the discussion to you two :)
Comment on attachment 603746 [details] [diff] [review]
part 1: add nsIProfiler::getSharedLibraryInformation

Alright I discussed things with glandium on IRC and my solution isn't much better. Let's take this patch since it doesn't make anything worse and we will switch over to using the information from breakpad later.
Attachment #603746 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/d674eb1d1aa4
https://hg.mozilla.org/mozilla-central/rev/34050c0da844
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla13
This seems to have broken builds on 'other' platforms, such as OpenBSD :

../../dist/bin/libxul.so.1.0: undefined reference to `SharedLibraryInfo::GetInfoForSelf()'

http://buildbot.rhaalovely.net/builders/mozilla-central-amd64/builds/321/steps/build/logs/stdio

I don't know what's the actual usecase for the codepaths under tools/profiler, but can it be made optional, or provide an empty/dumb fallback for other platforms ?
(In reply to Landry Breuil from comment #12)
> This seems to have broken builds on 'other' platforms, such as OpenBSD

Bug 734335 will fix this.
Duplicate of this bug: 731551
You need to log in before you can comment on or make changes to this bug.