Port v8 based profiler to Mac

RESOLVED FIXED in mozilla11

Status

()

Core
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: BenWa, Assigned: jrmuizel)

Tracking

unspecified
mozilla11
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 3 obsolete attachments)

44.79 KB, patch
Details | Diff | Splinter Review
703 bytes, patch
BenWa
: review+
Details | Diff | Splinter Review
2.85 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
647 bytes, patch
BenWa
: review+
Details | Diff | Splinter Review
5.79 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
726 bytes, patch
BenWa
: review+
Details | Diff | Splinter Review
1.35 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
2.37 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
13.54 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
2.17 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
I had this working with pthread but Jeff wanted to use something else.
(Assignee)

Comment 1

6 years ago
Created attachment 570820 [details] [diff] [review]
Proof of concept
(Assignee)

Updated

6 years ago
Summary: Port SPS to Mac → Port v8 based profiler to Mac
(Assignee)

Comment 2

6 years ago
Created attachment 572037 [details] [diff] [review]
Move platform.cc to platform-linux.cc
Attachment #572037 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #572037 - Flags: review? → review?(bgirard)
(Assignee)

Comment 3

6 years ago
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.
Attachment #572038 - Flags: review?(bgirard)
(Assignee)

Comment 4

6 years ago
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>
Attachment #572039 - Flags: review?(bgirard)
(Reporter)

Updated

6 years ago
Attachment #572039 - Flags: review?(bgirard) → review+
(Reporter)

Updated

6 years ago
Attachment #572037 - Flags: review?(bgirard) → review+
(Reporter)

Comment 5

6 years ago
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.
Attachment #572038 - Flags: review?(bgirard) → review-
(Reporter)

Comment 6

6 years ago
Created attachment 572725 [details] [diff] [review]
MachoMapInfo
(Reporter)

Comment 7

6 years ago
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 :)
(Reporter)

Updated

6 years ago
Attachment #572725 - Flags: review?(jmuizelaar)
(Assignee)

Comment 8

6 years ago
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;
>+}
>+
Attachment #572725 - Flags: review?(jmuizelaar) → review+
(Reporter)

Comment 9

6 years ago
Created attachment 572977 [details] [diff] [review]
MachoMapInfo

More comments + spacing + commit message.
Attachment #572725 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
Created attachment 577797 [details] [diff] [review]
Fix include order

I was seeing errors about include order without this.
Attachment #577797 - Flags: review?(bgirard)
(Assignee)

Comment 11

6 years ago
Created attachment 577798 [details] [diff] [review]
Fixup architecture define
Attachment #577798 - Flags: review?(bgirard)
(Assignee)

Comment 12

6 years ago
Created attachment 577800 [details] [diff] [review]
Expand the v8-support header with some needed stuff
Attachment #577800 - Flags: review?(bgirard)
(Assignee)

Comment 13

6 years ago
Created attachment 577803 [details] [diff] [review]
Change active_ to an Atomic32.

This matches what happened upstream.
Attachment #577803 - Flags: review?(bgirard)
(Assignee)

Comment 14

6 years ago
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.
Attachment #577806 - Flags: review?(bgirard)
(Reporter)

Comment 15

6 years ago
Comment on attachment 577797 [details] [diff] [review]
Fix include order

That change is already in m-c. Marking the patch as obsolete.
Attachment #577797 - Attachment is obsolete: true
Attachment #577797 - Flags: review?(bgirard)
(Reporter)

Comment 16

6 years ago
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.
Attachment #577798 - Flags: review?(bgirard) → review+
(Reporter)

Comment 17

6 years ago
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.
Attachment #577806 - Flags: review?(bgirard) → review-
(Reporter)

Comment 18

6 years ago
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.
(Reporter)

Comment 19

6 years ago
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.
(Assignee)

Comment 20

6 years ago
Created attachment 578258 [details] [diff] [review]
Add mac port

Address comments
Attachment #577806 - Attachment is obsolete: true
Attachment #578258 - Flags: review?(bgirard)
(Reporter)

Comment 21

6 years ago
Comment on attachment 572038 [details] [diff] [review]
Add v8-support.h for using v8 code

r+, needed for follow up patches
Attachment #572038 - Flags: review- → review+
(Reporter)

Comment 22

6 years ago
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
Attachment #578258 - Flags: review?(bgirard) → review+
(Reporter)

Comment 23

6 years ago
Comment on attachment 572977 [details] [diff] [review]
MachoMapInfo

based on breakpad/gdb
Attachment #572977 - Flags: review?(jmuizelaar)
(Reporter)

Comment 24

6 years ago
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.
Attachment #577800 - Flags: review?(bgirard) → review+
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
Assignee: nobody → jmuizelaar
Target Milestone: --- → mozilla11
(Reporter)

Comment 26

6 years ago
Created attachment 578405 [details] [diff] [review]
Initialize pthread keys on sps startup

r=jrmuizel
Attachment #578405 - Flags: review+
(Reporter)

Comment 27

6 years ago
Testing your change with attachment 578405 [details] [diff] [review], they work fine.
https://hg.mozilla.org/mozilla-central/rev/f05700c5c031
(Assignee)

Updated

6 years ago
Attachment #572977 - Flags: review?(jmuizelaar) → review+
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
Status: NEW → ASSIGNED
(Reporter)

Updated

6 years ago
Attachment #577803 - Flags: review?(bgirard) → review+
(Reporter)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.