Last Comment Bug 799640 - Need a shutdown-profiling mode
: Need a shutdown-profiling mode
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla20
Assigned To: Benoit Girard (:BenWa)
:
:
Mentors:
Depends on: 799638
Blocks: 818213 818381 835915
  Show dependency treegraph
 
Reported: 2012-10-09 12:33 PDT by (dormant account)
Modified: 2013-01-29 10:25 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Refactor JSObjectBuilder (8.88 KB, patch)
2012-11-22 13:06 PST, Benoit Girard (:BenWa)
ehsan: review+
Details | Diff | Splinter Review
Part 2 (wip): Standalone JSON builder (13.73 KB, patch)
2012-11-23 15:31 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 2 (wip): Standalone JSON builder (30.77 KB, patch)
2012-11-23 15:34 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN (34.74 KB, patch)
2012-11-24 10:26 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
art 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN (31.67 KB, patch)
2012-11-24 11:50 PST, Benoit Girard (:BenWa)
ehsan: review-
Details | Diff | Splinter Review
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN (35.49 KB, patch)
2012-11-26 15:59 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN (23.64 KB, patch)
2012-11-27 12:43 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN (35.42 KB, patch)
2012-11-27 20:14 PST, Benoit Girard (:BenWa)
ehsan: review+
Details | Diff | Splinter Review
Part 3: Add markers (2.70 KB, patch)
2012-11-27 20:32 PST, Benoit Girard (:BenWa)
respindola: review+
Details | Diff | Splinter Review
Part 1: Refactor JSObjectBuilder (8.93 KB, patch)
2012-11-30 09:50 PST, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN (35.01 KB, patch)
2012-11-30 09:51 PST, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review
Part 3: Add markers (2.73 KB, patch)
2012-11-30 09:51 PST, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review

Description (dormant account) 2012-10-09 12:33:09 PDT
The workflow in bug 799638 is also well-suited for diagnosing slow shutdown.
Comment 1 Benoit Girard (:BenWa) 2012-11-22 13:06:20 PST
Created attachment 684505 [details] [diff] [review]
Part 1: Refactor JSObjectBuilder
Comment 2 Benoit Girard (:BenWa) 2012-11-23 15:31:38 PST
Created attachment 684813 [details] [diff] [review]
Part 2 (wip): Standalone JSON builder
Comment 3 Benoit Girard (:BenWa) 2012-11-23 15:34:23 PST
Created attachment 684815 [details] [diff] [review]
Part 2 (wip): Standalone JSON builder
Comment 4 Benoit Girard (:BenWa) 2012-11-24 10:26:38 PST
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 :)
Comment 5 Benoit Girard (:BenWa) 2012-11-24 11:50:20 PST
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.
Comment 6 Benoit Girard (:BenWa) 2012-11-24 11:55:29 PST
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) 2012-11-24 17:58:44 PST
Shutdown profile:
http://people.mozilla.com/~bgirard/cleopatra/#report=0b979508bb527fdc4675518d8a678cef89d12eb8

Lots of GC as expected
Comment 8 Benoit Girard (:BenWa) 2012-11-24 18:00:42 PST
Open saved profile + symbolication:
https://github.com/bgirard/Gecko-Profiler-Addon/commit/69d42a0c5d9d8e7d05a704beeaf98b8dc244508b
Comment 9 :Ehsan Akhgari 2012-11-25 14:17:41 PST
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 :Ehsan Akhgari 2012-11-25 14:54:20 PST
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) 2012-11-25 18:21:11 PST
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) 2012-11-26 05:25:59 PST
(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 :Ehsan Akhgari 2012-11-26 09:34:03 PST
(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) 2012-11-26 11:27:56 PST
(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) 2012-11-26 15:27:30 PST
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) 2012-11-26 15:59:00 PST
Created attachment 685378 [details] [diff] [review]
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN

Almost done, just need to support UTF8 encoding.
Comment 17 Benoit Girard (:BenWa) 2012-11-27 12:43:56 PST
Created attachment 685783 [details] [diff] [review]
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN
Comment 18 :Ehsan Akhgari 2012-11-27 16:21:16 PST
I'll probably review this tomorrow, sorry for the delay!
Comment 19 Benoit Girard (:BenWa) 2012-11-27 20:11:38 PST
Last change to the extension:
https://github.com/bgirard/Gecko-Profiler-Addon/commit/e55e4db9c32f01b384927814ca9878190494bc8b
Comment 20 Benoit Girard (:BenWa) 2012-11-27 20:14:14 PST
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.
Comment 21 Benoit Girard (:BenWa) 2012-11-27 20:32:37 PST
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.
Comment 22 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-11-28 08:24:45 PST
Comment on attachment 685960 [details] [diff] [review]
Part 3: Add markers

Thanks again!
Comment 23 :Ehsan Akhgari 2012-11-28 10:42:28 PST
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) 2012-11-28 12:24:16 PST
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) 2012-11-28 12:26:41 PST
https://tbpl.mozilla.org/?tree=Try&rev=dd0cbe6d15e1
Comment 26 :Ehsan Akhgari 2012-11-28 13:33:25 PST
(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) 2012-11-28 13:35:49 PST
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 :Ehsan Akhgari 2012-11-28 15:29:31 PST
(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) 2012-11-29 15:34:16 PST
https://tbpl.mozilla.org/?tree=Try&rev=e170973bb03c
Comment 30 Benoit Girard (:BenWa) 2012-11-30 09:50:48 PST
Created attachment 687155 [details] [diff] [review]
Part 1: Refactor JSObjectBuilder
Comment 31 Benoit Girard (:BenWa) 2012-11-30 09:51:10 PST
Created attachment 687156 [details] [diff] [review]
Part 2: Standalone JS + save to MOZ_PROFILER_SHUTDOWN
Comment 32 Benoit Girard (:BenWa) 2012-11-30 09:51:33 PST
Created attachment 687157 [details] [diff] [review]
Part 3: Add markers
Comment 34 :Ehsan Akhgari 2012-11-30 13:07:56 PST
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
Comment 35 Benoit Girard (:BenWa) 2012-12-03 10:24:45 PST
Pushed to try tp5 but I doubt this patch is to blame:
https://tbpl.mozilla.org/?tree=Try&rev=432f3b63d2f7
Comment 38 Jan Beich 2012-12-04 11:44:04 PST
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) 2012-12-04 11:47:44 PST
Filed bug 818213
Comment 40 Benoit Girard (:BenWa) 2012-12-06 10:54:33 PST
https://tbpl.mozilla.org/?tree=Try&rev=ff1cb49c507d
Comment 41 Benoit Girard (:BenWa) 2012-12-07 10:58:57 PST
https://tbpl.mozilla.org/?tree=Try&rev=d1c62cd0cb02
Comment 42 Benoit Girard (:BenWa) 2012-12-07 12:10:41 PST
Forgot to request windows:
https://tbpl.mozilla.org/?tree=Try&rev=2761d34d506e
Comment 43 neil@parkwaycc.co.uk 2012-12-11 11:25:15 PST
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 :Ehsan Akhgari 2012-12-11 11:28:54 PST
You cannot create a JSCustomObject on the heap since operator new is private.

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