698002 - Port v8 based profiler to Mac

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Benoit Girard (:BenWa)]

I had this working with pthread but Jeff wanted to use something else.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Proof of concept

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Move platform.cc to platform-linux.cc

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: 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]]

Attachment: 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)]

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)]

Attachment: MachoMapInfo

Comment 7

[Benoit Girard (:BenWa)]

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]]

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)]

Attachment: MachoMapInfo

More comments + spacing + commit message.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Attachment: Fix include order

I was seeing errors about include order without this.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Attachment: Fixup architecture define

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Attachment: Expand the v8-support header with some needed stuff

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Attachment: Change active_ to an Atomic32.

This matches what happened upstream.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Attachment: 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)]

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)]

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)]

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)]

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)]

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]]

Attachment: Add mac port

Address comments

Comment 21

[Benoit Girard (:BenWa)]

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)]

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)]

Comment on attachment 572977 [details] [diff] [review]
MachoMapInfo

based on breakpad/gdb

Comment 24

[Benoit Girard (:BenWa)]

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)]

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)]

Attachment: Initialize pthread keys on sps startup

r=jrmuizel

Comment 27

[Benoit Girard (:BenWa)]

Testing your change with attachment 578405 [details] [diff] [review], they work fine.

Comment 28

[Marco Bonardo [:mak]]

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

Comment 29

[Ed Morley [:emorley]]

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