Need a shutdown-profiling mode

RESOLVED FIXED in mozilla20

Status

()

Core
Gecko Profiler
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: (dormant account), Assigned: BenWa)

Tracking

unspecified
mozilla20
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 9 obsolete attachments)

8.93 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
35.01 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
2.73 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
The workflow in bug 799638 is also well-suited for diagnosing slow shutdown.
(Assignee)

Comment 1

5 years ago
Created attachment 684505 [details] [diff] [review]
Part 1: Refactor JSObjectBuilder
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 684813 [details] [diff] [review]
Part 2 (wip): Standalone JSON builder
(Assignee)

Comment 3

5 years ago
Created attachment 684815 [details] [diff] [review]
Part 2 (wip): Standalone JSON builder
Attachment #684813 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 684868 [details] [diff] [review]
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN

This is all the platforms changes required to profile shutdown. After this we're just going to need a small change to the add-on :)
Attachment #684815 - Attachment is obsolete: true
Attachment #684868 - Flags: review?(ehsan)
(Assignee)

Comment 5

5 years ago
Created attachment 684873 [details] [diff] [review]
art 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN

Fixed a silly JSON syntax error.

I've got the profile loading but I'm just missing symbolication. I can't get OS.File.read to work because it complains that FileReader is missing :(. I'm going to rewrite the code to us nsIFile instead.
Attachment #684868 - Attachment is obsolete: true
Attachment #684868 - Flags: review?(ehsan)
Attachment #684873 - Flags: review?(ehsan)
(Assignee)

Comment 6

5 years ago
Rafael, can you tell me the best points in the code where the shutdown is initiated and the point where we plan on doing exit(0) so I can label them in the shutdown profile? Are there any other points of interest we should label?
Flags: needinfo?(respindola)
(Assignee)

Updated

5 years ago
Attachment #684505 - Flags: review?(ehsan)
(Assignee)

Comment 7

5 years ago
Shutdown profile:
http://people.mozilla.com/~bgirard/cleopatra/#report=0b979508bb527fdc4675518d8a678cef89d12eb8

Lots of GC as expected
(Assignee)

Comment 8

5 years ago
Open saved profile + symbolication:
https://github.com/bgirard/Gecko-Profiler-Addon/commit/69d42a0c5d9d8e7d05a704beeaf98b8dc244508b
Comment on attachment 684505 [details] [diff] [review]
Part 1: Refactor JSObjectBuilder

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

::: tools/profiler/JSObjectBuilder.h
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +class JSObject;

Nit: use struct here in order to avoid clang warnings please.
Attachment #684505 - Flags: review?(ehsan) → review+
Comment on attachment 684873 [details] [diff] [review]
art 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN

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

Minusing because I think the patch is not complete and has some bugs in it (see below.)  But the big question that I have is whether you considered to use a JSON library instead of rolling your own JSON encoder?  (I'm not necessarily opposed to doing that, I just want to know more about why you did it.)

::: tools/profiler/JSAObjectBuilder.h
@@ +14,5 @@
> +class JSAObjectBuilder
> +{
> +public:
> +  JSAObjectBuilder();
> +  virtual ~JSAObjectBuilder();

Nit: please drop the constructor, and define the destructor as pure virtual as well.

::: tools/profiler/JSCustomObjectBuilder.cpp
@@ +10,5 @@
> +
> +struct PropertyValue {
> +  virtual ~PropertyValue() {}
> +  virtual void SendToStream(std::ostream& stream) = 0;
> +};

Please document who owns PropertyValue's and when they get deleted.

@@ +24,5 @@
> +struct is_pointer<T*>
> +{
> +  static const bool value = true;
> +};
> +*/

What do you want to use this for?  This belongs to mozilla/TypeTraits.h if you need it.  Otherwise, please remove it.  :-)

@@ +30,5 @@
> +#if _MSC_VER
> + #define snprintf _snprintf
> +#endif
> +
> +void EscapeToStream(std::ostream& stream, const char* str) {

What is the encoding of the incoming buffer here?

@@ +42,5 @@
> +      stream << "\\\\";
> +    } else if (c < 20) {
> +      char escChar[5];
> +      snprintf(escChar, 5, "%04i", c);
> +      stream << "\\u" << escChar;

Hmm, I don't know what the JSON spec says about this, but I think you might have to do this for more than just the first 20 characters.

@@ +90,5 @@
> +    free(p);
> +  }
> +};
> +
> +template <class T> class TemplatePropertyValue: public PropertyValue {

Nit: add a newline before the second class.

@@ +236,5 @@
> +{
> +  if (!mOk)
> +    return;
> +
> +  aObject->mProperties.Put(nsDependentCString(name), new TemplatePropertyValue<JSCustomObject*>(aValue));

Nit: instead of this pattern of specifying the exact type of the property value you want to use, define a function like this:

template<class T>
TemplatePropertyValue<T>* CreatePropertyValue(T value)
{
  return new TemplatePropertyValue<T>(value);
}

And then use it like this:

aObject->mProperties.Put(nsDependentCString(name), CreatePropertyValue(value));

You could even go one step further and add this method to JSCustomObject:

template<class T>
void AddProperty(const char* name, T value);

And then just call it here and let the compiler do the type deduction.  Doing that would heavily simplify this code.

@@ +275,5 @@
> +
> +void
> +JSCustomObjectBuilder::ArrayPush(JSCustomArray *aArray, int value)
> +{
> +  aArray->mValues.AppendElement(new TemplatePropertyValue<int>(value));

Please do the same as above for this pattern.

::: tools/profiler/JSCustomObjectBuilder.h
@@ +6,5 @@
> +#ifndef JSCUSTOMOBJECTBUILDER_H
> +#define JSCUSTOMOBJECTBUILDER_H
> +
> +#include "JSAObjectBuilder.h"
> +#include <ostream>

Nit: please forward-delcare std::ostream.

@@ +18,5 @@
> +{
> +public:
> +
> +  // We need to ensure that this object lives on the stack so that GC sees it properly
> +  JSCustomObjectBuilder();

Hmm, if you really want to ensure this, you should declare the following private operators: new, new[], delete, delete[].

@@ +35,5 @@
> +  void ArrayPush(JSCustomArray *aArray, JSCustomObject *aObject);
> +  JSCustomArray  *CreateArray();
> +  JSCustomObject *CreateObject();
> +
> +  // Delete this object and all of it's descendant

Nit: its :P

@@ +39,5 @@
> +  // Delete this object and all of it's descendant
> +  void DeleteObject(JSCustomObject* aObject);
> +
> +private:
> +  JSCustomObjectBuilder(JSCustomObjectBuilder&);

Nit: Make this accept a const reference, and also declare a private operator=, with a comment explaining why you're doing this.

::: tools/profiler/JSObjectBuilder.cpp
@@ +12,5 @@
>  
>  void
> +JSObjectBuilder::DefineProperty(JSCustomObject *aObject, const char *name, JSCustomArray *aValue)
> +{
> +  DefineProperty(aObject, name, (JSCustomObject*)aValue);

I mostly didn't review this part of the patch because I didn't know what JSCustomObject is...

::: tools/profiler/JSObjectBuilder.h
@@ +7,5 @@
> +#define JSOBJECTBUILDER_H
> +
> +#include "JSAObjectBuilder.h"
> +
> +class JSCustomObject;

I think there might be some stuff missing from this patch...  I can't find where JSCustomObject is defined, for example.

@@ +39,3 @@
>  
>  private:
> +  JSObjectBuilder(JSCustomObjectBuilder&);

This seems wrong.  Also, you need a private assignment op as noted before.

::: tools/profiler/TableTicker.cpp
@@ +305,2 @@
>    {
> +    JSCustomObjectBuilder* b = new JSCustomObjectBuilder();

So you have a comment saying that this needs to live on the stack, and you allocate it on the heap.  I think that's the wrong thing to do.

@@ +313,4 @@
>  
> +  JSCustomObject *ToJSObject(JSContext *aCx)
> +  {
> +    JSAObjectBuilder* b = new JSObjectBuilder(aCx);

This should live on the stack too.

@@ +326,5 @@
>      b.DefineProperty(profile, "samples", samples);
>  
> +    JSCustomObject *sample = NULL;
> +    JSCustomArray *frames = NULL;
> +    JSCustomArray *marker = NULL;

Nit: nullptr.

@@ -512,5 @@
> -    // This will prevent us from recording sampling
> -    // regarding profile saving. This will also
> -    // prevent bugs caused by the circular buffer not
> -    // being thread safe. Bug 750989.
> -    t->SetPaused(true);

Hmm, why are you removing this code?

@@ +635,5 @@
>    }
>  
>    nsCOMPtr<nsIXULRuntime> runtime = do_GetService("@mozilla.org/xre/runtime;1");
>    if (runtime) {
> +    nsCString string;

Why is this needed?

@@ +660,5 @@
>  }
>  
> +void TableTicker::ToStreamAsJSON(std::ostream& stream)
> +{
> +  JSCustomObjectBuilder* b = new JSCustomObjectBuilder();

Seriously, use the stack!!!

@@ +672,3 @@
>  JSObject* TableTicker::ToJSObject(JSContext *aCx)
>  {
> +  JSObjectBuilder* b = new JSObjectBuilder(aCx);

Shall I say more?  ;-)

::: tools/profiler/sps_sampler.h
@@ +172,5 @@
>  char* mozilla_sampler_get_profile();
>  JSObject *mozilla_sampler_get_profile_data(JSContext *aCx);
>  const char** mozilla_sampler_get_features();
>  void mozilla_sampler_init();
> +void mozilla_sampler_deinit();

Nit: call this mozilla_sampler_shutdown, please.
Attachment #684873 - Flags: review?(ehsan) → review-
(Assignee)

Comment 11

5 years ago
Thanks for the detailed review!

(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> Comment on attachment 684873 [details] [diff] [review]
> art 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN
> 
> Review of attachment 684873 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Minusing because I think the patch is not complete and has some bugs in it
> (see below.)  But the big question that I have is whether you considered to
> use a JSON library instead of rolling your own JSON encoder?  (I'm not
> necessarily opposed to doing that, I just want to know more about why you
> did it.)
> 

I needed something that would have the same interface as JSObjectBuilder so that we could use one implementation if we we're returning a JSObject and another if we are writing it to a file. The actual JSON encoding is only about 20 lines or so. I didn't look into pulling an external library for something so simple. I did consider pulling in js' implementation but it likely would be too complex to get it working standalone.

> 
> @@ +30,5 @@
> > +#if _MSC_VER
> > + #define snprintf _snprintf
> > +#endif
> > +
> > +void EscapeToStream(std::ostream& stream, const char* str) {
> 
> What is the encoding of the incoming buffer here?
> 

I've been assuming ASCII for all inputs.

> @@ +42,5 @@
> > +      stream << "\\\\";
> > +    } else if (c < 20) {
> > +      char escChar[5];
> > +      snprintf(escChar, 5, "%04i", c);
> > +      stream << "\\u" << escChar;
> 
> Hmm, I don't know what the JSON spec says about this, but I think you might
> have to do this for more than just the first 20 characters.

Looks like I missed a few:

http://www.ietf.org/rfc/rfc4627.txt?number=4627
characters that must be escaped:
   quotation mark, reverse solidus, and the control characters (U+0000
   through U+001F).

> 
> I think there might be some stuff missing from this patch...  I can't find
> where JSCustomObject is defined, for example.
> 

It's in the patch. Under JSObjectBuilder JSCustomObject is a js JSObject but you're not allowed to use it as such directly and under JSCustomObjectBuilder it's a custom struct that holds the properties. Perhaps I should rename this to JSAbstractObject and put a comment. The idea is that we have 2 small method that either create a JSObectBuilder or a JSCustomObjectBuilder and we can either get back a JSObject to give to the JS engine or a write it out to a stream and the code that creates the profile just deals with an abstract JSAObjectBuilder without having to worry how the profile is going to be saved.

> 
> ::: tools/profiler/TableTicker.cpp
> @@ +305,2 @@
> >    {
> > +    JSCustomObjectBuilder* b = new JSCustomObjectBuilder();
> 
> So you have a comment saying that this needs to live on the stack, and you
> allocate it on the heap.  I think that's the wrong thing to do.

Good catch, I meant to fix this up, this is a remanence (not in our dictionary) of a previous iteration.

> @@ -512,5 @@
> > -    // This will prevent us from recording sampling
> > -    // regarding profile saving. This will also
> > -    // prevent bugs caused by the circular buffer not
> > -    // being thread safe. Bug 750989.
> > -    t->SetPaused(true);
> 
> Hmm, why are you removing this code?
> 

The pausing is handled later before saving, no point in pausing the profiler here since we do it later.
(In reply to Benoit Girard (:BenWa) from comment #6)
> Rafael, can you tell me the best points in the code where the shutdown is
> initiated and the point where we plan on doing exit(0) so I can label them
> in the shutdown profile? Are there any other points of interest we should
> label?

It is started at nsAppStartup::Quit. At least that is where we record the time to write to Telemetry.ShutdownTime.txt.

We intend to first call exit(0) at the point we currently poison writes in ShutdownXPCOM.  We will do it once we know no writes happen after that point. We should then move it back bit by bit until we get to be just after the call to nsXREDirProvider::DoShutdown in ~ScopedXPCOMStartup, so after the profile-* messages but before the xpcom-ones.

Can you mark all 3 points?
Thanks!
Flags: needinfo?(respindola)
(In reply to Benoit Girard (:BenWa) from comment #11)
> Thanks for the detailed review!
> 
> (In reply to Ehsan Akhgari [:ehsan] from comment #10)
> > Comment on attachment 684873 [details] [diff] [review]
> > art 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN
> > 
> > Review of attachment 684873 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Minusing because I think the patch is not complete and has some bugs in it
> > (see below.)  But the big question that I have is whether you considered to
> > use a JSON library instead of rolling your own JSON encoder?  (I'm not
> > necessarily opposed to doing that, I just want to know more about why you
> > did it.)
> > 
> 
> I needed something that would have the same interface as JSObjectBuilder so
> that we could use one implementation if we we're returning a JSObject and
> another if we are writing it to a file. The actual JSON encoding is only
> about 20 lines or so. I didn't look into pulling an external library for
> something so simple. I did consider pulling in js' implementation but it
> likely would be too complex to get it working standalone.

OK.

> > @@ +30,5 @@
> > > +#if _MSC_VER
> > > + #define snprintf _snprintf
> > > +#endif
> > > +
> > > +void EscapeToStream(std::ostream& stream, const char* str) {
> > 
> > What is the encoding of the incoming buffer here?
> > 
> 
> I've been assuming ASCII for all inputs.

Is that the correct assumption?  For example, what happens if you have a UTF-8 character somewhere in the input stream.  Surely we just don't drop those characters, right?

> > I think there might be some stuff missing from this patch...  I can't find
> > where JSCustomObject is defined, for example.
> > 
> 
> It's in the patch. Under JSObjectBuilder JSCustomObject is a js JSObject but
> you're not allowed to use it as such directly and under
> JSCustomObjectBuilder it's a custom struct that holds the properties.
> Perhaps I should rename this to JSAbstractObject and put a comment. The idea
> is that we have 2 small method that either create a JSObectBuilder or a
> JSCustomObjectBuilder and we can either get back a JSObject to give to the
> JS engine or a write it out to a stream and the code that creates the
> profile just deals with an abstract JSAObjectBuilder without having to worry
> how the profile is going to be saved.

Right, sorry I didn't spot it for some reason.

> > ::: tools/profiler/TableTicker.cpp
> > @@ +305,2 @@
> > >    {
> > > +    JSCustomObjectBuilder* b = new JSCustomObjectBuilder();
> > 
> > So you have a comment saying that this needs to live on the stack, and you
> > allocate it on the heap.  I think that's the wrong thing to do.
> 
> Good catch, I meant to fix this up, this is a remanence (not in our
> dictionary) of a previous iteration.

Heh, ok.  File a bug for remanence though.  ;-)

> > @@ -512,5 @@
> > > -    // This will prevent us from recording sampling
> > > -    // regarding profile saving. This will also
> > > -    // prevent bugs caused by the circular buffer not
> > > -    // being thread safe. Bug 750989.
> > > -    t->SetPaused(true);
> > 
> > Hmm, why are you removing this code?
> > 
> 
> The pausing is handled later before saving, no point in pausing the profiler
> here since we do it later.

My question is, did your patch change something that makes this unnecessary, or was it unnecessary in the first place?  In the latter case, you should put this in a separate bug/patch.
(Assignee)

Comment 14

5 years ago
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #12)
> (In reply to Benoit Girard (:BenWa) from comment #6)
> > Rafael, can you tell me the best points in the code where the shutdown is
> > initiated and the point where we plan on doing exit(0) so I can label them
> > in the shutdown profile? Are there any other points of interest we should
> > label?
> 
> It is started at nsAppStartup::Quit. At least that is where we record the
> time to write to Telemetry.ShutdownTime.txt.
> 
> We intend to first call exit(0) at the point we currently poison writes in
> ShutdownXPCOM.  We will do it once we know no writes happen after that
> point. We should then move it back bit by bit until we get to be just after
> the call to nsXREDirProvider::DoShutdown in ~ScopedXPCOMStartup, so after
> the profile-* messages but before the xpcom-ones.
> 
> Can you mark all 3 points?
> Thanks!

Thanks for the response. I'll post a part 3 with these and land the marker with these patches.
(Assignee)

Comment 15

5 years ago
I recall now. I had tried to forward declare ostream but it's a typedef so I have the assume the underlying type is basic_ostream<char>.

See the error. My quick search showed that forward declaring typedef is painful and risks not being portable. 

/usr/include/c++/4.2.1/iosfwd:137:33: error: typedef redefinition with different types ('basic_ostream<char>' vs 'std::ostream')
  typedef basic_ostream<char>           ostream;        ///< @isiosfwd
                                        ^
/Volumes/SSD-Mac1/Users/benoitgirard/ssd-mozilla/mozilla-central/tree/tools/profiler/JSCustomObjectBuilder.h:12:9: note: previous
      definition is here
  class ostream;
        ^
(Assignee)

Comment 16

5 years ago
Created attachment 685378 [details] [diff] [review]
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN

Almost done, just need to support UTF8 encoding.
(Assignee)

Comment 17

5 years ago
Created attachment 685783 [details] [diff] [review]
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN
Attachment #684873 - Attachment is obsolete: true
Attachment #685378 - Attachment is obsolete: true
Attachment #685783 - Flags: review?(ehsan)
I'll probably review this tomorrow, sorry for the delay!
(Assignee)

Comment 19

5 years ago
Last change to the extension:
https://github.com/bgirard/Gecko-Profiler-Addon/commit/e55e4db9c32f01b384927814ca9878190494bc8b
(Assignee)

Comment 20

5 years ago
Created attachment 685952 [details] [diff] [review]
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN

I was missing a call to profiler shutdown on restart which annoying goes through a slightly different code path. This patch is good to go.
Attachment #685783 - Attachment is obsolete: true
Attachment #685783 - Flags: review?(ehsan)
Attachment #685952 - Flags: review?(ehsan)
(Assignee)

Comment 21

5 years ago
Created attachment 685960 [details] [diff] [review]
Part 3: Add markers

Here's a profile collected with part 1,2,3 + Gecko profiler changes:
http://people.mozilla.com/~bgirard/cleopatra/?report=8113135303dc4c1dea7c5749497c6cfae720d210

The UI for showing the markers will need more work but the markers will be sufficient for scripts to parse and analyze.

Let me know if you can suggest better names.
Attachment #685960 - Flags: review?(respindola)
Comment on attachment 685960 [details] [diff] [review]
Part 3: Add markers

Thanks again!
Attachment #685960 - Flags: review?(respindola) → review+
Comment on attachment 685952 [details] [diff] [review]
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN

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

::: tools/profiler/JSCustomObjectBuilder.cpp
@@ +14,5 @@
> +  virtual void SendToStream(std::ostream& stream) = 0;
> +};
> +
> +template <typename T>
> +struct dtor_impl

Hmm, on the second thought, please call this finalizer_impl or something.  dtor has a very clear meaning in C++ and only one of the implementations of this function does what dtors do.  :-)

@@ +36,5 @@
> +  }
> +};
> +
> +template <class T> class TemplatePropertyValue
> +  : public PropertyValue {

Your formatting looks weird.  I'd prefer the following style much better, and I think it's used in most other places in Gecko as well:

template<class T>
class Foo : public Bar
{
...
};

@@ +53,5 @@
> +};
> +
> +#if _MSC_VER
> + #define snprintf _snprintf
> +#endif

Nit: please move this up to after the #includes.

@@ +63,5 @@
> +  stream << "\"";
> +
> +  // Test unicode encoding
> +  //unsigned char myStr[] = {0xF0,0x9D,0x84,0x9E,0x00};
> +  //str = (const char*)myStr;

Please remove the testing code.  ;-)

@@ +69,5 @@
> +  size_t len = strlen(str);
> +  const char* end = &str[len];
> +  while (str < end) {
> +    bool err;
> +    const char* utf8CharStart = &str[0];

Nit: str.

@@ +91,5 @@
> +      PRUnichar chr[2];
> +      ConvertUTF8toUTF16 encoder(chr);
> +      encoder.write(utf8CharStart, str-utf8CharStart);
> +      char escChar[13];
> +      snprintf(escChar, 13, "\\u%04X\\u%04X", chr[0], chr[1]);

Nit: please use ArrayLength instead of harcoding the array length everywhere.

@@ +104,5 @@
> +  }
> +  stream << "\"";
> +  return;
> +
> +  // See http://www.ietf.org/rfc/rfc4627.txt?number=4627

I believe everything after this line is now dead code.  Please remove it, and also the return statement above.

@@ +300,5 @@
> +
> +void
> +JSCustomObjectBuilder::DefineProperty(JSCustomObject *aObject, const char *name, const char *aValue)
> +{
> +  aObject->AddProperty(name, strdup(aValue));

Nit: please add a comment saying who will free this.

@@ +312,5 @@
> +
> +void
> +JSCustomObjectBuilder::ArrayPush(JSCustomArray *aArray, const char *value)
> +{
> +  aArray->AppendElement(strdup(value));

Same here.

::: tools/profiler/JSCustomObjectBuilder.h
@@ +17,5 @@
> +{
> +public:
> +
> +  // We need to ensure that this object lives on the stack so that GC sees it properly
> +  JSCustomObjectBuilder();

My previous comment still stands.

::: tools/profiler/JSObjectBuilder.cpp
@@ +21,5 @@
> +{
> +  if (!mOk)
> +    return;
> +
> +  mOk = JS_DefineProperty(mCx, (JSObject*)aObject, name, OBJECT_TO_JSVAL((JSObject*)aValue), NULL, NULL, JSPROP_ENUMERATE);

Nit: please use nullptr everywhere in the patch.

@@ +30,5 @@
> +{
> +  if (!mOk)
> +    return;
> +
> +  mOk = JS_DefineProperty(mCx, (JSObject*)aObject, name, INT_TO_JSVAL(value), NULL, NULL, JSPROP_ENUMERATE);

Hmm, these casts are very unfortunate.  I'm inclined to suggest that you should typedef JSCustomObject to be a void* and then cast it everywhere to the correct types, and document the hell out of all this.  But I trust your judgement on whether you do this or not.

::: tools/profiler/JSObjectBuilder.h
@@ +19,4 @@
>  {
> +public:
> +  // We need to ensure that this object lives on the stack so that GC sees it properly
> +  JSObjectBuilder(JSContext *aCx);

Please make this explicit.

::: tools/profiler/TableTicker.cpp
@@ +520,5 @@
>    SaveProfileTask() {}
>  
>    NS_IMETHOD Run() {
>      TableTicker *t = tlsTicker.get();
> +    // Pause the profiler during SAMPLER_INITving.

Nit: drop 'v'.

@@ +525,2 @@
>      // This will prevent us from recording sampling
> +    // regarding fromprofile saving. This will also

fromprofile?
Attachment #685952 - Flags: review?(ehsan) → review+
(Assignee)

Comment 24

5 years ago
I forgot about the operator new/delete bit. Adding this:
  static void* operator new(size_t) CPP_THROW_NEW;
  static void* operator new[](size_t) CPP_THROW_NEW;
  static void operator delete(void*);
  static void operator delete[](void*);

yields:
Undefined symbols for architecture x86_64:
  "JSObjectBuilder::operator delete(void*)", referenced from:
      JSObjectBuilder::~JSObjectBuilder() in JSObjectBuilder.o

I can't find an example of this in MXR either :(
(Assignee)

Comment 25

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=dd0cbe6d15e1
(In reply to comment #24)
> I forgot about the operator new/delete bit. Adding this:
>   static void* operator new(size_t) CPP_THROW_NEW;
>   static void* operator new[](size_t) CPP_THROW_NEW;
>   static void operator delete(void*);
>   static void operator delete[](void*);
> 
> yields:
> Undefined symbols for architecture x86_64:
>   "JSObjectBuilder::operator delete(void*)", referenced from:
>       JSObjectBuilder::~JSObjectBuilder() in JSObjectBuilder.o
> 
> I can't find an example of this in MXR either :(

You should be able to provide a dummy implementation for them which does NS_NOTREACHED() etc.
(Assignee)

Comment 27

5 years ago
The fact that's it's trying to call operator delete from the destructor didn't sound right so putting a NOTREACHED in there is a bit troubling. I'll give it a shot.
(In reply to comment #27)
> The fact that's it's trying to call operator delete from the destructor didn't
> sound right so putting a NOTREACHED in there is a bit troubling. I'll give it a
> shot.

Whut?! You shouldn't see the dtor calling operator delete.  What does the generated code look like?
(Assignee)

Comment 29

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=e170973bb03c
(Assignee)

Comment 30

5 years ago
Created attachment 687155 [details] [diff] [review]
Part 1: Refactor JSObjectBuilder
Attachment #684505 - Attachment is obsolete: true
Attachment #687155 - Flags: review+
(Assignee)

Comment 31

5 years ago
Created attachment 687156 [details] [diff] [review]
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN
Attachment #685952 - Attachment is obsolete: true
Attachment #687156 - Flags: review+
(Assignee)

Comment 32

5 years ago
Created attachment 687157 [details] [diff] [review]
Part 3: Add markers
Attachment #685960 - Attachment is obsolete: true
Attachment #687157 - Flags: review+
(Assignee)

Comment 33

5 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e47b059493cf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b130bb991d84
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/133c704dbcc6
Backed out on the suspicion of regressing Tp5 by 50%!!!

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd22236ffb15

Please reland once you make sure that this regression was not caused by this patch.




Regression  Tp5 No Network Row Major Shutdown MozAfterPaint increase 45.1% on MacOSX 10.8 Mozilla-Inbound
-----------------------------------------------------------------------------------------------------------
    Previous: avg 398.667 stddev 17.175 of 30 runs up to revision ce1e1e13511b
    New     : avg 578.600 stddev 27.043 of 5 runs since revision 944c97dabc33
    Change  : +179.933 (45.1% / z=10.476)
    Graph   : http://mzl.la/QTeoJd

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ce1e1e13511b&tochange=944c97dabc33

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/5ae27ec3d9ec
    : Gregor Wagner <anygregor@gmail.com> - Bug 815974 - [System] Remember geolocation permission for an app is broken. r=sicking
    : http://bugzilla.mozilla.org/show_bug.cgi?id=815974

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/b88467155adc
    : Axel Hecht <axel@pike.org> - bug 796051, add chrome-% target to package b2g localized files, r=fabrice
    : http://bugzilla.mozilla.org/show_bug.cgi?id=796051

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/e47b059493cf
    : Benoit Girard <b56girard@gmail.com> - Bug 799640 - Part 1: Refactor JSObjectBuilder. r=ehsan
    : http://bugzilla.mozilla.org/show_bug.cgi?id=799640

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/b130bb991d84
    : Benoit Girard <b56girard@gmail.com> - Bug 799640 - Part 2: Save profiles on shutdown using custom JSON encoder. r=ehsan
    : http://bugzilla.mozilla.org/show_bug.cgi?id=799640

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/133c704dbcc6
    : Benoit Girard <b56girard@gmail.com> - Bug 799640 - Part 3: Add shutdown labels. r=espindola
    : http://bugzilla.mozilla.org/show_bug.cgi?id=799640

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/944c97dabc33
    : Michal Novotny <michal.novotny@gmail.com> - Bug 816642 - Avoid fragmenting cache files, r=jduell
    : http://bugzilla.mozilla.org/show_bug.cgi?id=816642

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=799640 - Need a shutdown-profiling mode
  * http://bugzilla.mozilla.org/show_bug.cgi?id=816642 - Avoid fragmenting cache files
  * http://bugzilla.mozilla.org/show_bug.cgi?id=796051 - Generate make target to package needed l10n files in gecko
  * http://bugzilla.mozilla.org/show_bug.cgi?id=815974 - [System] Remember geolocation permission for an app is broken
_______________________________________________
dev-tree-management mailing list
dev-tree-management@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-tree-management
(Assignee)

Comment 35

5 years ago
Pushed to try tp5 but I doubt this patch is to blame:
https://tbpl.mozilla.org/?tree=Try&rev=432f3b63d2f7
(Assignee)

Comment 36

5 years ago
Reland after a chat with ehsan
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b12ba63cffa9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b11c3a2300b1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0d2fab58667c
https://hg.mozilla.org/mozilla-central/rev/b12ba63cffa9
https://hg.mozilla.org/mozilla-central/rev/b11c3a2300b1
https://hg.mozilla.org/mozilla-central/rev/0d2fab58667c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
It doesn't build without SPS. I haven't tested --enable-profiling but placing SAMPLER_SHUTDOWN() stub in tools/profiler/sampler.h seems to help.

toolkit/xre/nsAppRunner.cpp:3941:5: error:
      use of undeclared identifier 'SAMPLER_SHUTDOWN'
    SAMPLER_SHUTDOWN();
    ^
toolkit/xre/nsAppRunner.cpp:3964:3: error:
      use of undeclared identifier 'SAMPLER_SHUTDOWN'
  SAMPLER_SHUTDOWN();
  ^
(Assignee)

Updated

5 years ago
Blocks: 818213
(Assignee)

Comment 39

5 years ago
Filed bug 818213

Updated

5 years ago
Blocks: 818381
(Assignee)

Comment 40

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=ff1cb49c507d
(Assignee)

Comment 41

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=d1c62cd0cb02
(Assignee)

Comment 42

5 years ago
Forgot to request windows:
https://tbpl.mozilla.org/?tree=Try&rev=2761d34d506e
Comment on attachment 687156 [details] [diff] [review]
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN

>+class JSCustomObjectBuilder : public JSAObjectBuilder
>+{
...
>+private:
>+  // This class can't be copied
>+  JSCustomObjectBuilder(const JSCustomObjectBuilder&);
>+  JSCustomObjectBuilder& operator=(const JSCustomObjectBuilder&);
>+
>+  void* operator new(size_t);
>+  void* operator new[](size_t);
>+  void operator delete(void*) {
>+    // Since JSCustomObjectBuilder has a virtual destructor the compiler
>+    // has to provide a destructor in the object file that will call
>+    // operate delete in case there is a derived class since its
>+    // destructor wont know how to free this instance.
>+    abort();
>+  }
>+  void operator delete[](void*);
So, you want to be able to create other subclasses of JSAObjectBuilder on the heap and delete them using JSAObjectBuilder's operator delete? (Otherwise why would JSAObjectBuilder's destructor to be virtual?) This rather defeats the point of making operator delete private in JSCustomObjectBuilder, as if you could create a JSCustomObjectBuilder in the heap you would be able to cast the pointer to JSAObjectBuilder* to delete it.

A quick web search suggests that nobody deletes operator delete as deleting operator new suffices.
You cannot create a JSCustomObject on the heap since operator new is private.
Blocks: 835915
You need to log in before you can comment on or make changes to this bug.