Last Comment Bug 698002 - Port v8 based profiler to Mac
: Port v8 based profiler to Mac
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla11
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
Depends on: 683229
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-28 09:08 PDT by Benoit Girard (:BenWa)
Modified: 2012-02-01 13:59 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proof of concept (44.79 KB, patch)
2011-10-31 13:20 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Move platform.cc to platform-linux.cc (703 bytes, patch)
2011-11-04 12:02 PDT, Jeff Muizelaar [:jrmuizel]
bgirard: review+
Details | Diff | Splinter Review
Add v8-support.h for using v8 code (2.85 KB, patch)
2011-11-04 12:06 PDT, Jeff Muizelaar [:jrmuizel]
bgirard: review+
Details | Diff | Splinter Review
Fix includes. (647 bytes, patch)
2011-11-04 12:07 PDT, Jeff Muizelaar [:jrmuizel]
bgirard: review+
Details | Diff | Splinter Review
MachoMapInfo (5.29 KB, patch)
2011-11-07 19:29 PST, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review
MachoMapInfo (5.79 KB, patch)
2011-11-08 13:12 PST, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review
Fix include order (532 bytes, patch)
2011-11-29 16:47 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Fixup architecture define (726 bytes, patch)
2011-11-29 16:47 PST, Jeff Muizelaar [:jrmuizel]
bgirard: review+
Details | Diff | Splinter Review
Expand the v8-support header with some needed stuff (1.35 KB, patch)
2011-11-29 16:48 PST, Jeff Muizelaar [:jrmuizel]
bgirard: review+
Details | Diff | Splinter Review
Change active_ to an Atomic32. (2.37 KB, patch)
2011-11-29 16:55 PST, Jeff Muizelaar [:jrmuizel]
bgirard: review+
Details | Diff | Splinter Review
Add mac port (21.56 KB, patch)
2011-11-29 16:59 PST, Jeff Muizelaar [:jrmuizel]
bgirard: review-
Details | Diff | Splinter Review
Add mac port (13.54 KB, patch)
2011-12-01 07:27 PST, Jeff Muizelaar [:jrmuizel]
bgirard: review+
Details | Diff | Splinter Review
Initialize pthread keys on sps startup (2.17 KB, patch)
2011-12-01 14:44 PST, Benoit Girard (:BenWa)
bgirard: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2011-10-28 09:08:38 PDT
I had this working with pthread but Jeff wanted to use something else.
Comment 1 Jeff Muizelaar [:jrmuizel] 2011-10-31 13:20:39 PDT
Created attachment 570820 [details] [diff] [review]
Proof of concept
Comment 2 Jeff Muizelaar [:jrmuizel] 2011-11-04 12:02:39 PDT
Created attachment 572037 [details] [diff] [review]
Move platform.cc to platform-linux.cc
Comment 3 Jeff Muizelaar [:jrmuizel] 2011-11-04 12:06:05 PDT
Created attachment 572038 [details] [diff] [review]
Add v8-support.h for using v8 code

As a first step this adds a dummy implementation of the Malloced class used by the linux implementation.
Comment 4 Jeff Muizelaar [:jrmuizel] 2011-11-04 12:07:40 PDT
Created attachment 572039 [details] [diff] [review]
Fix includes.

There's a time.h in chromium/base and if we add /base to our include path we pick up /base/time.h instead of the system header <time.h>
Comment 5 Benoit Girard (:BenWa) 2011-11-04 13:59:46 PDT
Comment on attachment 572038 [details] [diff] [review]
Add v8-support.h for using v8 code

Add a comment about why where adding 'Malloced' like we discussed. Maybe another comment about how we're trying to keep some sync with v8-support.h.

We should consider adding an entry to about:license since we're going to be very close to the v8 profiler.
Comment 6 Benoit Girard (:BenWa) 2011-11-07 19:29:21 PST
Created attachment 572725 [details] [diff] [review]
MachoMapInfo
Comment 7 Benoit Girard (:BenWa) 2011-11-07 21:16:20 PST
Here's a sample of the data I got using the info from backtrace + MachoMapInfo patch + atos:

js::Parser::variables(bool) (in XUL) + 2120
js::Parser::statement() (in XUL) + 1657
js::frontend::CompileScript(JSContext*, JSObject*, js::StackFrame*, JSPrincipals*, unsigned int, unsigned short const*, unsigned long, char const*, unsigned int, JSVersion, JSString*, unsigned int) (in XUL) + 1462
CompileUCScriptForPrincipalsCommon(JSContext*, JSObject*, JSPrincipals*, unsigned short const*, unsigned long, char const*, unsigned int, JSVersion) (in XUL) + 138
JS_CompileScriptForPrincipalsVersion (in XUL) + 154
mozJSComponentLoader::GlobalForLocation(nsILocalFile*, nsIURI*, JSObject**, char**, JS::Value*) (in XUL) (mozJSComponentLoader.cpp:924)
mozJSComponentLoader::LoadModuleImpl(nsILocalFile*, nsAString_internal&, nsIURI*) (in XUL) (mozJSComponentLoader.cpp:608)
mozJSComponentLoader::LoadModule(nsILocalFile*) (in XUL) (mozJSComponentLoader.cpp:548)
nsComponentManagerImpl::KnownModule::Load() (in XUL) (nsComponentManager.cpp:951)
nsFactoryEntry::GetFactory() (in XUL) (nsComponentManager.cpp:1973)
nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) (in XUL) (nsComponentManager.cpp:1295)
nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) (in XUL) (nsComponentManager.cpp:1701)
nsGetServiceByContractIDWithError::operator()(nsID const&, void**) const (in XUL) (nsComponentManagerUtils.cpp:94)
nsCOMPtr_base::assign_from_gs_contractid_with_error(nsGetServiceByContractIDWithError const&, nsID const&) (in XUL) (nsCOMPtr.cpp:142)
nsAppStartupNotifier::Observe(nsISupports*, char const*, unsigned short const*) (in XUL) (nsAppStartupNotifier.cpp:104)
XRE_main (in XUL) (nsCOMPtr.h:522)

Looks like a pretty good trace to me :)
Comment 8 Jeff Muizelaar [:jrmuizel] 2011-11-08 12:37:34 PST
Comment on attachment 572725 [details] [diff] [review]
MachoMapInfo


>+void addMapEntry(const platform_mach_header* header, char *name, MapInfo &info) {
>+        const struct load_command *cmd =
>+                reinterpret_cast<const struct load_command *>(header + 1);
>+
>+        seg_size size;
>+        for (unsigned int i = 0; cmd && (i < header->ncmds); ++i) {
>+                if (cmd->cmd == CMD_SEGMENT) {
>+                        const mach_segment_command_type *seg =
>+                                reinterpret_cast<const mach_segment_command_type *>(cmd);
>+
>+                        if (!strcmp(seg->segname, "__TEXT")) {
>+                                size = seg->vmsize;
>+                                unsigned long long start = reinterpret_cast<unsigned long long>(header);
>+                                printf("%s start: %p end: %p size:%p\n", name, start, start+seg->vmsize, seg->vmsize);
>+                                info.AddMapEntry(MapEntry(start, start+seg->vmsize, seg->vmsize, name));
>+                                return;
>+                        }
>+                }
>+
>+                cmd = reinterpret_cast<const struct load_command *>
>+                        (reinterpret_cast<const char *>(cmd) + cmd->cmdsize);
>+        }
>+}
>+
>+MapInfo getMapInfo() {
>+        MapInfo mapInfo;
>+
>+        task_dyld_info_data_t task_dyld_info;
>+        mach_msg_type_number_t count = TASK_DYLD_INFO_COUNT;
>+        if (task_info(mach_task_self (), TASK_DYLD_INFO, (task_info_t)&task_dyld_info,
>+                                &count) != KERN_SUCCESS) {
>+                return mapInfo;
>+        }

It might be good to add some more comments about what's going on with this code and some more white space too.

>+        struct dyld_all_image_infos* aii = (struct dyld_all_image_infos*)task_dyld_info.all_image_info_addr;
>+        size_t infoCount = aii->infoArrayCount;
>+        for (size_t i = 0; i < infoCount; ++i) {
>+                const dyld_image_info *info = &aii->infoArray[i];
>+
>+                if(info->imageLoadAddress->magic != MACHO_MAGIC_NUMBER) {

missing space after the if

>+                        return mapInfo;
>+                }
>+
>+                const platform_mach_header* header =
>+                        reinterpret_cast<const platform_mach_header*>(info->imageLoadAddress);
>+
>+
>+                addMapEntry(header, (char*)info->imageFilePath, mapInfo);
>+
>+        }
>+        return mapInfo;
>+}
>+
Comment 9 Benoit Girard (:BenWa) 2011-11-08 13:12:31 PST
Created attachment 572977 [details] [diff] [review]
MachoMapInfo

More comments + spacing + commit message.
Comment 10 Jeff Muizelaar [:jrmuizel] 2011-11-29 16:47:10 PST
Created attachment 577797 [details] [diff] [review]
Fix include order

I was seeing errors about include order without this.
Comment 11 Jeff Muizelaar [:jrmuizel] 2011-11-29 16:47:51 PST
Created attachment 577798 [details] [diff] [review]
Fixup architecture define
Comment 12 Jeff Muizelaar [:jrmuizel] 2011-11-29 16:48:34 PST
Created attachment 577800 [details] [diff] [review]
Expand the v8-support header with some needed stuff
Comment 13 Jeff Muizelaar [:jrmuizel] 2011-11-29 16:55:28 PST
Created attachment 577803 [details] [diff] [review]
Change active_ to an Atomic32.

This matches what happened upstream.
Comment 14 Jeff Muizelaar [:jrmuizel] 2011-11-29 16:59:02 PST
Created attachment 577806 [details] [diff] [review]
Add mac port

This is the roughest of the patches as I haven't really looked it over myself yet. However, it seems to work.
Comment 15 Benoit Girard (:BenWa) 2011-11-29 18:02:08 PST
Comment on attachment 577797 [details] [diff] [review]
Fix include order

That change is already in m-c. Marking the patch as obsolete.
Comment 16 Benoit Girard (:BenWa) 2011-11-29 18:03:53 PST
Comment on attachment 577798 [details] [diff] [review]
Fixup architecture define

Ehsan and jlebar already have this change. I guess who ever lands this first wins. Ehsan has the version we ultimately want that supports windows as well.
Comment 17 Benoit Girard (:BenWa) 2011-11-29 18:44:59 PST
Comment on attachment 577806 [details] [diff] [review]
Add mac port

Review of attachment 577806 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/Makefile.in
@@ +79,5 @@
> +LIBXUL_LIBRARY  = 1
> +
> +CPPSRCS		= \
> +  $(NULL)
> +

These lines wont be needed with what is checked into m-c.

::: tools/profiler/sampler.h
@@ -93,4 +93,4 @@
> >  #define SAMPLE_MARKER(info)
> >  
> >  // Redefine the macros for platforms where SPS is supported.
> > -#ifdef ANDROID
> > +//#ifdef ANDROID

Why are you removing this for all platforms? We should let each port adjust this.

::: tools/profiler/sps/platform-macos.cc
@@ +121,5 @@
> +
> +  // Mac OS X does not expose the length limit of the name, so hardcode it.
> +  static const int kMaxNameLength = 63;
> +  USE(kMaxNameLength);
> +  //ASSERT(Thread::kMaxThreadNameLength <= kMaxNameLength);

We define ASSERT to MOZ_ASSERT so we can uncomment this.

@@ +140,5 @@
> +}
> +
> +
> +void Thread::set_name(const char* name) {
> +  strncpy(name_, name, sizeof(name_));

UMR. Could also raise SIGSEGV.

@@ +159,5 @@
> +}
> +
> +
> +void Thread::Join() {
> +  pthread_join(data_->thread_, NULL);

Isn't this initialized to pthread_self()? Looks like your joining with yourself.

@@ +282,5 @@
> +SamplerThread* SamplerThread::instance_ = NULL;
> +
> +
> +Sampler::Sampler(int interval, bool profiling)
> +    : // isolate_(isolate),

Let's remove this

@@ +284,5 @@
> +
> +Sampler::Sampler(int interval, bool profiling)
> +    : // isolate_(isolate),
> +      interval_(interval),
> +      profiling_(profiling),

We should consider removing 'profiling_'. I don't see any plans to use this for anything other then profiling in the short term. Doesn't have to happen here.

@@ +299,5 @@
> +
> +
> +void Sampler::Start() {
> +  ASSERT(!IsActive());
> +  //SetActive(true);

You're asserting this on DTOR/Stop but you're not calling it. We should be using this since we plan on letting JS toggle the profiler.

::: tools/profiler/sps/platform.h
@@ -83,0 +84,55 @@
> > +// ----------------------------------------------------------------------------
> > +// Mutex
> > +//
> > +// Mutexes are used for serializing access to non-reentrant sections of code.
NaN more ...

This is including a lot of code we don't need, most of which doesn't have an implementation.
Comment 18 Benoit Girard (:BenWa) 2011-11-29 18:46:49 PST
Comment on attachment 577800 [details] [diff] [review]
Expand the v8-support header with some needed stuff

Review of attachment 577800 [details] [diff] [review]:
-----------------------------------------------------------------

I feel like we're already including, and using in other ports, analogous version of these. I'm not convinced we want these.
Comment 19 Benoit Girard (:BenWa) 2011-11-29 18:53:58 PST
Comment on attachment 577803 [details] [diff] [review]
Change active_ to an Atomic32.

Review of attachment 577803 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/sps/platform.h
@@ +149,4 @@
>    const int interval_;
>    const bool profiling_;
>    const bool synchronous_;
> +  Atomic32 active_;

Shouldn't we be using 'volatile sig_atomic_t' like linux instead?

::: tools/profiler/sps/v8-support.h
@@ -54,0 +54,8 @@
> > +typedef int32_t Atomic32;
> > +#ifdef V8_HOST_ARCH_64_BIT
> > +// We need to be able to go between Atomic64 and AtomicWord implicitly.  This
> > +// means Atomic64 and AtomicWord should be the same type on 64-bit.
NaN more ...

We're not using this.
Comment 20 Jeff Muizelaar [:jrmuizel] 2011-12-01 07:27:52 PST
Created attachment 578258 [details] [diff] [review]
Add mac port

Address comments
Comment 21 Benoit Girard (:BenWa) 2011-12-01 07:44:47 PST
Comment on attachment 572038 [details] [diff] [review]
Add v8-support.h for using v8 code

r+, needed for follow up patches
Comment 22 Benoit Girard (:BenWa) 2011-12-01 08:21:29 PST
Comment on attachment 578258 [details] [diff] [review]
Add mac port

r+ but still a few things that would be nice to fix:
- Some useless code commented
- Not a fan of using both pthread and mac threads
Comment 23 Benoit Girard (:BenWa) 2011-12-01 08:30:31 PST
Comment on attachment 572977 [details] [diff] [review]
MachoMapInfo

based on breakpad/gdb
Comment 24 Benoit Girard (:BenWa) 2011-12-01 11:00:17 PST
Comment on attachment 577800 [details] [diff] [review]
Expand the v8-support header with some needed stuff

We've decided that we want to let people rip out SPS out of mozilla with relative ease. Thus this v8-support header helps us do that. Although some work will be needed for other ports. Later if we feel this is important we can file a follow up bug and clean up the usage.
Comment 25 Matt Brubeck (:mbrubeck) 2011-12-01 11:40:34 PST
Three patches checked in for mozilla11.  Leaving open for remaining patches.
https://hg.mozilla.org/mozilla-central/rev/169cad8b9e52
https://hg.mozilla.org/mozilla-central/rev/24b7e2efa35c
https://hg.mozilla.org/mozilla-central/rev/c7763db2a4da
Comment 26 Benoit Girard (:BenWa) 2011-12-01 14:44:34 PST
Created attachment 578405 [details] [diff] [review]
Initialize pthread keys on sps startup

r=jrmuizel
Comment 27 Benoit Girard (:BenWa) 2011-12-01 21:09:06 PST
Testing your change with attachment 578405 [details] [diff] [review], they work fine.
Comment 28 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-12-02 03:21:44 PST
https://hg.mozilla.org/mozilla-central/rev/f05700c5c031
Comment 29 Ed Morley [:emorley] 2011-12-04 07:19:47 PST
Presuming this wants to stay open post-merge? (For clarity in the future, can you annotate the whiteboard please: https://wiki.mozilla.org/Tree_Rules/Inbound#Please_do_the_following_after_pushing_to_inbound)

https://hg.mozilla.org/mozilla-central/rev/efdfd71b342f

Note You need to log in before you can comment on or make changes to this bug.