799640 - Need a shutdown-profiling mode

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[(dormant account)]

The workflow in bug 799638 is also well-suited for diagnosing slow shutdown.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Part 1: Refactor JSObjectBuilder

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Part 2 (wip): Standalone JSON builder

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Part 2 (wip): Standalone JSON builder

Comment 4

[Benoit Girard (:BenWa)]

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

Comment 5

[Benoit Girard (:BenWa)]

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

Comment 6

[Benoit Girard (:BenWa)]

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?

Comment 7

[Benoit Girard (:BenWa)]

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

Lots of GC as expected

Comment 8

[Benoit Girard (:BenWa)]

Open saved profile + symbolication:
https://github.com/bgirard/Gecko-Profiler-Addon/commit/69d42a0c5d9d8e7d05a704beeaf98b8dc244508b

Comment 9

[(no longer active)]

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.

Comment 10

[(no longer active)]

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.

Comment 11

[Benoit Girard (:BenWa)]

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.

Comment 12

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(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!

Comment 13

[(no longer active)]

(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.

Comment 14

[Benoit Girard (:BenWa)]

(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.

Comment 15

[Benoit Girard (:BenWa)]

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;
        ^

Comment 16

[Benoit Girard (:BenWa)]

Attachment: Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN

Almost done, just need to support UTF8 encoding.

Comment 17

[Benoit Girard (:BenWa)]

Attachment: Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN

Comment 18

[(no longer active)]

I'll probably review this tomorrow, sorry for the delay!

Comment 19

[Benoit Girard (:BenWa)]

Last change to the extension:
https://github.com/bgirard/Gecko-Profiler-Addon/commit/e55e4db9c32f01b384927814ca9878190494bc8b

Comment 20

[Benoit Girard (:BenWa)]

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

Comment 21

[Benoit Girard (:BenWa)]

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

Comment 22

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 685960 [details] [diff] [review]
Part 3: Add markers

Thanks again!

Comment 23

[(no longer active)]

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?

Comment 24

[Benoit Girard (:BenWa)]

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 :(

Comment 25

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=dd0cbe6d15e1

Comment 26

[(no longer active)]

(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.

Comment 27

[Benoit Girard (:BenWa)]

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.

Comment 28

[(no longer active)]

(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?

Comment 29

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=e170973bb03c

Comment 30

[Benoit Girard (:BenWa)]

Attachment: Part 1: Refactor JSObjectBuilder

Comment 31

[Benoit Girard (:BenWa)]

Attachment: Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN

Comment 32

[Benoit Girard (:BenWa)]

Attachment: Part 3: Add markers

Comment 33

[Benoit Girard (:BenWa)]

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

Comment 34

[(no longer active)]

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 <[e-mail]> - 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 <[e-mail]> - 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 <[e-mail]> - 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 <[e-mail]> - 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 <[e-mail]> - 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 <[e-mail]> - 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
[e-mail]
https://lists.mozilla.org/listinfo/dev-tree-management

Comment 35

[Benoit Girard (:BenWa)]

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

Comment 36

[Benoit Girard (:BenWa)]

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

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b12ba63cffa9
https://hg.mozilla.org/mozilla-central/rev/b11c3a2300b1
https://hg.mozilla.org/mozilla-central/rev/0d2fab58667c

Comment 38

[Jan Beich]

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();
  ^

Comment 39

[Benoit Girard (:BenWa)]

Filed bug 818213

Comment 40

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=ff1cb49c507d

Comment 41

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=d1c62cd0cb02

Comment 42

[Benoit Girard (:BenWa)]

Forgot to request windows:
https://tbpl.mozilla.org/?tree=Try&rev=2761d34d506e

Comment 43

[[e-mail]]

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.

Comment 44

[(no longer active)]

You cannot create a JSCustomObject on the heap since operator new is private.