893222 - Modify strings memory reporter so that it aggregates strings by their value before deciding what does and doesn't count as a "huge" string

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Justin Lebar (not reading bugmail)]

Currently you're a "huge" string if you're really long.

But we've seen bugs where we believe we many copies of a small string.  I'm having trouble finding the bug right now, but there was one which, in comment 2, I identified that we had ~150mb of strings, none of which was huge.

I have a patch to fix this.

Comment 1

[Justin Lebar (not reading bugmail)]

Here's a simple script (paste into the JS console) to generate a lot of
duplicate small strings.

> function makestr()
> {
>     var s = "";
>     var chars = "AB";
> 
>     for (var i = 0; i < 5; i++) {
>         s += chars.charAt(Math.floor(Math.random() * chars.length));
>     }
> 
>     return s;
> }
> 
> var arr = [];
> 
> for (var i = 0; i < 100000; i++) {
>   arr.push(makestr());
> }

And here's what it looks like in about:memory.

> 22.73 MB (12.08%) -- js-zone(0x120026000)
> ├──22.50 MB (11.96%) -- gc-heap
> │  ├──15.65 MB (08.32%) ── unused-gc-things
> │  ├───6.12 MB (03.25%) -- strings
> │  │   ├──6.10 MB (03.24%) -- huge
> │  │   │  ├──0.20 MB (00.11%) ── string(length=5, copies=3241, "AABAB")/gc-things
> │  │   │  ├──0.20 MB (00.10%) ── string(length=5, copies=3229, "ABBAB")/gc-things
> │  │   │  ├──0.20 MB (00.10%) ── string(length=5, copies=3206, "ABBBA")/gc-things
> │  │   │  ├──0.20 MB (00.10%) ── string(length=5, copies=3204, "BAABA")/gc-things
> │  │   └──0.02 MB (00.01%) ── normal-gc-things

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Add a new PutEscapedString implementation, which takes a raw JSString*.

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Modify the JS memory reporter to consider a string as "notable" if we have many small copies of it.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v2: Modify the JS memory reporter to consider a string as "notable" if we have many small copies of it.

Forgot to qref.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v3: Modify the JS memory reporter to consider a string as "notable" if we have many small copies of it.

Rebased atop tip.

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v3: Modify the JS memory reporter to consider a string as "notable" if we have many small copies of it.

Rebased atop tip.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 775002 [details] [diff] [review]
Part 1: Add a new PutEscapedString implementation, which takes a raw JSString*.

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

> Part 1: Add a new PutEscapedString implementation, which takes a raw JSString*.

Should be "a raw char* buffer".

::: js/src/jsstr.cpp
@@ +4647,5 @@
>      return ucs4Char;
>  }
>  
>  size_t
> +js::PutEscapedStringImpl(char *buffer, size_t bufferSize, FILE *fp, JSLinearString* str,

JS style puts '*' on the right.  Please move it back.

::: js/src/jsstr.h
@@ +343,5 @@
>  extern size_t
>  PutEscapedStringImpl(char *buffer, size_t size, FILE *fp, JSLinearString *str, uint32_t quote);
>  
> +extern size_t
> +PutEscapedStringImpl(char *buffer, size_t bufferSize, FILE *fp, const jschar* chars,

'*' on the right for |chars|.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 775016 [details] [diff] [review]
Part 2, v3: Modify the JS memory reporter to consider a string as "notable" if we have many small copies of it.

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

I'm giving f+ because the overall idea is excellent but I'm unhappy with malloc'd memory going under the gc-heap sub-tree.  AFAICT you should be able to keep the sub-tree structure much as it is now (replacing 'huge' with 'notable').  Once you've done that, can you please post sample output along with the updated patch?  That helps a lot with reviewing.  THnaks.

And apologies in advance for all the style nits;  writing SpiderMonkey code when you're used to Gecko is quite a style shift :/

::: js/public/MemoryMetrics.h
@@ +36,3 @@
>  JS_FRIEND_API(size_t) MemoryReportingSundriesThreshold();
>  
> +struct JSStringHashPolicy

This is in |namespace js| so it doesn't need the |JS| prefix.

@@ +36,5 @@
>  JS_FRIEND_API(size_t) MemoryReportingSundriesThreshold();
>  
> +struct JSStringHashPolicy
> +{
> +    typedef JSString* Lookup;

'*' on the right.

@@ +38,5 @@
> +struct JSStringHashPolicy
> +{
> +    typedef JSString* Lookup;
> +    static HashNumber hash(const Lookup &l);
> +    static bool match(const JSString* const &k, const Lookup &l);

Ditto.

@@ +41,5 @@
> +    static HashNumber hash(const Lookup &l);
> +    static bool match(const JSString* const &k, const Lookup &l);
> +};
> +
> +struct ZoneStatsPod

Separating the POD part is a nice idea.

@@ +44,5 @@
> +
> +struct ZoneStatsPod
> +{
> +    ZoneStatsPod()
> +    {

SpiderMonkey style is to put the opening brace of a function defined within its class on the previous line.

@@ +49,5 @@
> +        mozilla::PodZero(this);
> +    }
> +
> +    void add(const ZoneStatsPod &other)
> +    {

Ditto.

@@ +158,5 @@
>  
>      CodeSizes() { memset(this, 0, sizeof(CodeSizes)); }
>  };
>  
> +struct StringInfo

A brief comment on this class would be helpful.

@@ +176,5 @@
> +    {}
> +
> +    void add(size_t shortStringGCThingSize, size_t normalStringGCThingSize,
> +             size_t stringCharsSize, size_t copies = 1)
> +    {

Opening brace on previous line, here and in various cases below.

@@ +188,5 @@
> +    {
> +        if (length == 0) {
> +          length = info.length;
> +        } else {
> +           MOZ_ASSERT(length == info.length);

SM style is to not brace single line indented blocks :/

@@ +194,5 @@
> +
> +        totalShortStringGCThingSize += info.totalShortStringGCThingSize;
> +        totalNormalStringGCThingSize += info.totalNormalStringGCThingSize;
> +        totalStringCharsSize += info.totalStringCharsSize;
> +        numCopies += info.numCopies;

If |info.numCopies| ever not equal to 1 here?

@@ +209,5 @@
> +    }
> +
> +    // A string's size in memory is not necessarily equal to twice its length
> +    // because the allocator and the JS engine both may round up.
> +    size_t length;

I assume |length| is holding the numbers of chars in the string, right?  In which case I just found this comment confusing at this point, because it made me wonder if it was the malloc_size_of instead.

@@ +212,5 @@
> +    // because the allocator and the JS engine both may round up.
> +    size_t length;
> +
> +    size_t totalShortStringGCThingSize;
> +    size_t totalNormalStringGCThingSize;

IIUC, for any instance of this class, one of these values will be zero, because a string of length N will always be short or always be normal.  Assuming that's right, it's worth a comment, and possibly some assertions, e.g. in add().

@@ +213,5 @@
> +    size_t length;
> +
> +    size_t totalShortStringGCThingSize;
> +    size_t totalNormalStringGCThingSize;
> +    size_t totalStringCharsSize;

"sizeOf" is used for most names in memory reporters.  So I'd prefer |sizeOfAllStringChars|, and likewise for similar cases.

@@ +222,2 @@
>  // bytes of memory), so we can report it individually.
> +struct NotableStringInfo : public StringInfo

If this is the only subclass of StringInfo, is there any reason not to just merge them?

(Looking lower, I see that StringsHashMap uses StringInfo.  So maybe a comment would make this less surprising.)

@@ +226,5 @@
> +    NotableStringInfo(JSString *str, const StringInfo &info);
> +    NotableStringInfo(const NotableStringInfo& info);
> +    NotableStringInfo(mozilla::MoveRef<NotableStringInfo> info);
> +    NotableStringInfo &operator=(mozilla::MoveRef<NotableStringInfo> info);
> +    ~NotableStringInfo();

That's a lot of constructors and similar operations.

@@ +234,1 @@
>      static size_t MinSize() {

Maybe call it notableSize() now?

@@ +234,5 @@
>      static size_t MinSize() {
>          return js::MemoryReportingSundriesThreshold();
>      }
>  
> +    size_t bufferSize;

Is this the malloc_size_of the buffer?  (Subsequent code indicates not.)  A comment would help.

@@ +284,5 @@
> +            if (p) {
> +                p->value.add(r.front().value);
> +            } else {
> +                strings.add(p, r.front().key, r.front().value);
> +            }

The overloading of |add| here is enough that comments would help.  E.g. "We've seen this string before;  record this as a duplicate." and "We haven't seen this string before;  add to the hash table."  Or something like that?

::: js/src/jsmemorymetrics.cpp
@@ +95,5 @@
> +NotableStringInfo::NotableStringInfo(JSString *str, const StringInfo &info)
> +    : StringInfo(info)
> +{
> +    bufferSize = Min(str->length() + 1, size_t(4096));
> +    buffer = js_pod_malloc<char>(bufferSize);

At first I thought there was a problem because str->length() is in jschars but bufferSize is in chars.  But I guess if the string is ASCII it'll fit (assuming it's less than 4096 chars), and it'll only be truncated if there are non-ASCII chars which need escaping.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1485,5 @@
> +                   "Memory allocated to hold characters for strings which are too long "
> +                   "to fit entirely within their string headers.\n\n"
> +                   "Although this reporter is under 'gc-heap', the memory it "
> +                   "counts is allocated on the malloc heap, not the JS "
> +                   "garbage-collected heap.\n\n"

I'm not happy with this.  It renders the gc-heap total incorrect, and I can't see a good reason for it.

@@ +1506,5 @@
> +        nsCString path = pathPrefix +
> +            nsPrintfCString("gc-heap/strings/notable/string"
> +                            "(length=%d, copies=%d, \"%s\"%s)",
> +                            info.length, info.numCopies, escapedString.get(),
> +                            truncated ? " (truncated)" : "");

Again, this shouldn't be under gc-heap.

@@ +1517,5 @@
> +            "together use more than %d bytes total of JS GC heap and malloc heap "
> +            "memory.\n\n"
> +            "These headers may contain the string data itself, if the string "
> +            "is short enough.  If so, the string won't have any memory reported "
> +            "under string-chars.",

Put single quotes around paths and sub-paths in these descriptions.

Comment 9

[Justin Lebar (not reading bugmail)]

I agree that this patch needs more comments; I'm sorry I sent it for review without them.

> IIUC, for any instance of this class, one of these values will be zero, because a string 
> of length N will always be short or always be normal.

I don't know if that's actually correct.  For example, what if I have a rope "A" + "B" + "C"?  Even if it's correct now I didn't want to assume that it would be correct in the future.

> I'm giving f+ because the overall idea is excellent but I'm unhappy with malloc'd memory 
> going under the gc-heap sub-tree.  AFAICT you should be able to keep the sub-tree 
> structure much as it is now (replacing 'huge' with 'notable').

Sorry, I should have explained why I made this change before I asked for review.

The current structure doesn't let you ask "how much memory is due to strings"?  You have to ask separately "how much memory is due to string js-things?" and "how much memory is due to string chars?"

This doesn't seem optimal to me.  It's particularly bad if you have a bunch of strings of the magic length such that a string's chars use about as much memory as the string's gc-thing.  In that case, in order to figure out how much memory notable string X uses, you have to add two different values from different parts of the tree.

I agree that putting something under gc-heap that doesn't actually live in the gc heap is non-ideal; I just didn't have a better idea.  I've never found myself caring about the size of the js garbage-collected heap in such a way that if it included string chars, I'd need to subtract them out.  The distinction seems pretty inside-baseball.

Maybe we should rename "gc-heap" to something else?

Comment 10

[Justin Lebar (not reading bugmail)]

> Should be "a raw char* buffer".

"a raw JSString*" is obviously wrong, but it should be "a raw jschar*", right?

>+extern size_t
>+PutEscapedStringImpl(char *buffer, size_t bufferSize, FILE *fp, const jschar* chars,
>+                     size_t length, uint32_t quote);

Comment 11

[Nicholas Nethercote [inactive]]

Having to add two numbers doesn't seem that onerous;  no slicing of the data will suit every need.  And I still insist that our presentation of the data contains no lies.

Maybe there's a way to restructure the tree so that the string values all end up in one place and there are no lies, e.g. by getting rid of gc-heap somehow.  I feel like this might be possible but I'm suffering through my fifth day of influenza and my brain is too mushy to work it out right now.  Sorry.

Comment 12

[Justin Lebar (not reading bugmail)]

> No slicing of the data will suit every need.

Indeed; my claim is that the current slicing of data, which lets you easily tell how many bytes we've allocated on the JS GC heap, isn't as useful as a different slicing of data, which lets you easily tell how many bytes we've allocated for a given string, or how many bytes we've allocated to strings in general.

I could be wrong, though; maybe you think the gc-heap number is particularly useful?

If not, what if we removed gc-heap and then moved everything inside it up one level?

Comment 13

[Nicholas Nethercote [inactive]]

> If not, what if we removed gc-heap and then moved everything inside it up
> one level?

It's a reasonable suggestion.  There are several GC things (objects, strings, shapes, scripts) where we have GC thing memory and malloc'd memory.  It would make sense to group them, e.g.:

objects
- gc-things
  - ordinary
  - dense-array
  - function
  - cross-compartment-wrapper
- extra
  - slots
  - elements
  - regexp-statics

However, it's a sizeable change -- and the gc-heap trees under 'js-main-runtime' and 'js-main-runtime/zones' might need rearranging as well.
And it doesn't seem fundamental to this bug.  Can we think about it as a follow-up?

Comment 14

[Justin Lebar (not reading bugmail)]

>     static bool match(const JSString* const &k, const Lookup &l);

I made this

>     static bool match(const JSString * const &k, const Lookup &l);

(i.e., a space before "const") because of

$ git grep '* const' -- js/src | wc -l
221
$ git grep '*const' -- js/src | wc -l
126

But let me know if that's wrong.

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Part 0: Make JSRope::getCharsNonDestructiveInternal work with a null tcx.

Comment 16

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v2: Add a new PutEscapedString implementation, which takes a raw jschar*.

Comment 17

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v2: Modify the JS memory reporter to consider a string as "notable" if we have many small copies of it.

I left the paths as they were in this patch because I'm about to change them in the next patch.

Comment 18

[Justin Lebar (not reading bugmail)]

Attachment: Part 3, v1: Rework JS memory reporters so gc-heap is no longer a top-level node.

Comment 19

[Justin Lebar (not reading bugmail)]

An example compartment reporter

> 0.37 MB (00.72%) -- compartment([System Principal], resource://gre/modules/XPIProvider.jsm)
> ├──0.11 MB (00.22%) -- shapes
> │  ├──0.08 MB (00.16%) -- js-gc-heap
> │  │  ├──0.05 MB (00.10%) -- tree
> │  │  │  ├──0.04 MB (00.08%) ── global-parented
> │  │  │  └──0.01 MB (00.02%) ── non-global-parented
> │  │  ├──0.02 MB (00.04%) ── base
> │  │  └──0.01 MB (00.02%) ── dict
> │  └──0.03 MB (00.07%) -- malloc-heap
> │     ├──0.02 MB (00.03%) ── compartment-tables
> │     ├──0.01 MB (00.02%) ── tree-tables
> │     └──0.01 MB (00.02%) ── tree-shape-kids
> ├──0.11 MB (00.22%) -- objects
> │  ├──0.10 MB (00.19%) -- js-gc-heap
> │  │  ├──0.07 MB (00.13%) ── function
> │  │  └──0.03 MB (00.06%) ── ordinary
> │  └──0.02 MB (00.03%) ── malloc-heap/slots
> ├──0.10 MB (00.19%) -- scripts
> │  ├──0.07 MB (00.14%) ── js-gc-things
> │  └──0.02 MB (00.04%) ── malloc-heap/data
> ├──0.02 MB (00.05%) -- baseline/stubs
> │  ├──0.02 MB (00.03%) ── fallback
> │  └──0.01 MB (00.02%) ── optimized
> └──0.02 MB (00.04%) -- sundries
>    ├──0.01 MB (00.03%) ── malloc-heap
>    └──0.01 MB (00.01%) ── js-gc-heap

The js-main-runtime aggregated reporter.

> 27.70 MB (100.0%) -- js-main-runtime
> ├──14.16 MB (51.12%) -- compartments
> │  ├───5.86 MB (21.15%) -- shapes
> │  │   ├──3.81 MB (13.77%) -- js-gc-heap
> │  │   │  ├──2.09 MB (07.53%) -- tree
> │  │   │  │  ├──1.84 MB (06.63%) ── global-parented
> │  │   │  │  └──0.25 MB (00.90%) ── non-global-parented
> │  │   │  ├──1.32 MB (04.75%) ── base
> │  │   │  └──0.41 MB (01.48%) ── dict
> │  │   └──2.04 MB (07.38%) -- malloc-heap
> │  │      ├──1.37 MB (04.94%) ── compartment-tables
> │  │      ├──0.39 MB (01.41%) ++ (2 tiny)
> │  │      └──0.28 MB (01.03%) ── tree-tables
> │  ├───5.20 MB (18.77%) -- objects
> │  │   ├──4.28 MB (15.43%) -- js-gc-heap
> │  │   │  ├──3.04 MB (10.97%) ── function
> │  │   │  ├──0.99 MB (03.56%) ── ordinary
> │  │   │  └──0.25 MB (00.91%) -- (2 tiny)
> │  │   │     ├──0.15 MB (00.54%) ── dense-array
> │  │   │     └──0.10 MB (00.36%) ── cross-compartment-wrapper
> │  │   └──0.92 MB (03.34%) -- malloc-heap
> │  │      ├──0.88 MB (03.18%) ── slots
> │  │      └──0.05 MB (00.16%) -- (2 tiny)
> │  │         ├──0.03 MB (00.12%) ── regexp-statics
> │  │         └──0.01 MB (00.04%) ── elements/non-asm.js
> │  ├───2.52 MB (09.08%) -- scripts
> │  │   ├──2.05 MB (07.39%) ── js-gc-things
> │  │   └──0.47 MB (01.69%) ── malloc-heap/data
> │  └───0.59 MB (02.12%) -- (7 tiny)
> │      ├──0.18 MB (00.64%) ── compartment-object
> │      ├──0.13 MB (00.49%) ── cross-compartment-wrapper-table
> │      ├──0.11 MB (00.39%) -- baseline
> │      │  ├──0.09 MB (00.34%) -- stubs
> │      │  │  ├──0.06 MB (00.21%) ── fallback
> │      │  │  └──0.04 MB (00.13%) ── optimized
> │      │  └──0.01 MB (00.05%) ── data
> │      ├──0.10 MB (00.38%) -- type-inference
> │      │  ├──0.09 MB (00.34%) ── analysis-pool
> │      │  └──0.01 MB (00.04%) ── type-scripts
> │      ├──0.04 MB (00.13%) ── regexp-compartment
> │      ├──0.01 MB (00.05%) ── sundries/malloc-heap
> │      └──0.01 MB (00.04%) ── debuggees-set
> ├───6.08 MB (21.95%) -- zones
> │   ├──3.01 MB (10.85%) -- strings
> │   │  ├──1.28 MB (04.62%) -- normal
> │   │  │  ├──0.72 MB (02.61%) ── malloc-heap/string-chars
> │   │  │  └──0.56 MB (02.01%) ── js-gc-things
> │   │  ├──1.18 MB (04.26%) ── short/js-gc-things
> │   │  └──0.54 MB (01.96%) -- notable
> │   │     ├──0.25 MB (00.90%) -- string(length=107192, copies=1, "{"windows":[{"tabs":[{"ent
> │   │     │  ├──0.25 MB (00.90%) ── malloc-heap/string-chars
> │   │     │  └──0.00 MB (00.00%) ── js-gc-things
> │   │     ├──0.18 MB (00.64%) -- string(length=93018, copies=1, "{"cc":["appbar","bfoot","bo
> │   │     │  ├──0.18 MB (00.64%) ── malloc-heap/string-chars
> │   │     │  └──0.00 MB (00.00%) ── js-gc-things
> │   │     ├──0.01 MB (00.05%) ── string(length=8, copies=214, "explicit")/js-gc-things
> │   │     ├──0.01 MB (00.04%) ++ string(length=5398, copies=1, "{"c":{},"sb":{"agen":false,"
> │   │     ├──0.01 MB (00.04%) ++ string(length=5322, copies=1, "{"c":{},"sb":{"agen":false,"
> │   │     ├──0.01 MB (00.03%) ++ string(length=4437, copies=1, "explicit//workers//workers()
> │   │     ├──0.01 MB (00.03%) ++ string(length=4425, copies=1, "explicit//workers//workers()
> │   │     ├──0.01 MB (00.03%) ++ string(length=4341, copies=1, "explicit//workers//workers()
> │   │     ├──0.01 MB (00.03%) ++ string(length=4344, copies=1, "explicit//workers//workers()
> │   │     ├──0.01 MB (00.03%) ++ string(length=4332, copies=1, "explicit//workers//workers()
> │   │     ├──0.01 MB (00.03%) ++ string(length=4329, copies=1, "explicit//workers//workers()
> │   │     ├──0.01 MB (00.03%) ++ string(length=4312, copies=1, "explicit//workers//workers()
> │   │     ├──0.01 MB (00.03%) ++ string(length=4314, copies=1, "explicit//workers//workers()
> │   │     ├──0.01 MB (00.03%) ++ string(length=4300, copies=1, "explicit//workers//workers()
> │   │     └──0.01 MB (00.03%) ++ string(length=4302, copies=1, "explicit//workers//workers()
> │   ├──2.47 MB (08.93%) ── unused-js-gc-things
> │   ├──0.32 MB (01.15%) -- (4 tiny)
> │   │  ├──0.24 MB (00.86%) ── arena-admin
> │   │  ├──0.04 MB (00.15%) ── ion-codes
> │   │  ├──0.04 MB (00.14%) ── type-pool
> │   │  └──0.00 MB (00.00%) ── sundries/malloc-heap
> │   └──0.28 MB (01.02%) ── type-objects
> ├───4.39 MB (15.83%) ── runtime
> ├───1.81 MB (06.53%) ── unused-arenas
> ├───1.00 MB (03.61%) ── unused-chunks
> └───0.27 MB (00.96%) ── chunk-admin

You'll notice that I tried to be explicit about what's in the malloc heap versus what's in the JS heap.  I didn't like using "js-gc-heap" both as a leaf and a non-leaf, so I decided that "js-gc-things" would be for leaves, while "js-gc-heap" would be for non-leaves.

I think it might be interesting to add a separate memory reporter kind for JS_GC_HEAP.  Then we could, e.g., color JS_GC_HEAP entries differently in about:memory.  But that's all orthogonal to this patch.

Comment 20

[Justin Lebar (not reading bugmail)]

I suspect you'll want to try out these patches.  You can check out my git branch:

  $ git remote add jlebar https://github.com/jlebar/mozilla-central.git
  $ git fetch jlebar
  $ git checkout jlebar/short-strings-about-memory

If you want to apply the patches from the bug, they apply atop

  http://hg.mozilla.org/mozilla-central/rev/3433a021847b587aa

Comment 21

[Nicholas Nethercote [inactive]]

Ugh, I was *really* hoping you'd do the minimal notable string changes in this bug and the gc-heap rearrangement (i.e. part 3) in a follow-up.  How hard would it be to do that at this point?


> >     static bool match(const JSString* const &k, const Lookup &l);
> >     static bool match(const JSString * const &k, const Lookup &l);

Ignore the existing code, the former is proper!

Comment 22

[Justin Lebar (not reading bugmail)]

> the former is proper!

Do you mean, |const JSString *const| is proper?  Because the former is what you asked me to change.

> Ugh, I was *really* hoping you'd do the minimal notable string changes in this bug and 
> the gc-heap rearrangement (i.e. part 3) in a follow-up.

Sorry.  :(

Let me see how hard it would be to do what you're asking.  It's probably not too bad...

Comment 23

[Nicholas Nethercote [inactive]]

> Do you mean, |const JSString *const| is proper?

Yes, that's the proper form.  Sorry for misreading and if I said otherwise earlier.

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 781249 [details] [diff] [review]
Part 0: Make JSRope::getCharsNonDestructiveInternal work with a null tcx.

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

What's the calling context where you have a null tcx?  This seems pretty harmless, but I'll defer to bhackett.

Comment 25

[Justin Lebar (not reading bugmail)]

> It's probably not too bad...

Okay, it's worse than I expected.  I might be able to do it tomorrow.

That's what I get for going with the path of least resistance, I guess...

Comment 26

[Justin Lebar (not reading bugmail)]

> What's the calling context where you have a null tcx?

Part 2 in this bug.  :)

Comment 27

[Nicholas Nethercote [inactive]]

> Okay, it's worse than I expected.  I might be able to do it tomorrow.

In case it's not clear:  I'm currently assuming you can do this and so am waiting for the updated patches before reviewing.

Comment 28

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v3: Modify the JS memory reporter to consider a string as "notable" if we have many small copies of it.

This isn't ideal, because we have to manually add and subtract from the zStats measurements in order to get the right thing.  But I'm not wild about changing this even more if we're just going to change it back.

Comment 29

[Justin Lebar (not reading bugmail)]

Attachment: Part 3, v2: Rework JS memory reporters so gc-heap is no longer a top-level node.

We can do this in a separate bug, if you like.

Comment 30

[Justin Lebar (not reading bugmail)]

I updated the git branch, in case you want to look.

Comment 31

[Nicholas Nethercote [inactive]]

Comment on attachment 782023 [details] [diff] [review]
Part 2, v3: Modify the JS memory reporter to consider a string as "notable" if we have many small copies of it.

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

Lots of nits (sorry).  Beyond that, there's a deeper issue about ZoneStats::notableStrings that I'd like to discuss.  See below.

::: js/public/MemoryMetrics.h
@@ +38,5 @@
> +struct StringHashPolicy
> +{
> +    typedef JSString *Lookup;
> +    static HashNumber hash(const Lookup &l);
> +    static bool match(const JSString * const &k, const Lookup &l);

|JSString *const|

@@ +156,5 @@
>  
>      CodeSizes() { memset(this, 0, sizeof(CodeSizes)); }
>  };
>  
> +/**

No need for javadoc-style /** comments in SpiderMonkey. And feel free to use // in block comments (I personally prefer it).

@@ +159,5 @@
>  
> +/**
> + * This class holds information about the memory taken up by a particular
> + * string.  Multiple JSStrings may have their sizes aggregated together into
> + * one StringInfo object.

The meaning of "particular string" is unclear.  I think you mean something like "all identical instances of a particular platonic string".

Maybe "This class holds information about the memory taken up by all identical copies of a unique string." ?

@@ +169,5 @@
> +        sizeOfNormalStringGCThings(0), sizeOfAllStringChars(0)
> +    {}
> +
> +    StringInfo(size_t length, size_t shortStringGCThingSize,
> +               size_t normalStringGCThingSize, size_t stringCharsSize)

You could just use |shorts|, |normals| and |chars| as the argument names, if you like.

@@ +178,5 @@
> +        sizeOfAllStringChars(stringCharsSize)
> +    {}
> +
> +    void add(size_t shortStringGCThingSize, size_t normalStringGCThingSize,
> +             size_t stringCharsSize) {

Ditto.

@@ +186,5 @@
> +        numCopies++;
> +    }
> +
> +    void add(const StringInfo& info) {
> +        if (length == 0)

A comment about how the |length == 0| case would be helpful... I guess it's because you have a hashtable of these, which are constructed with the zero-arg constructor, and that's how it's possible to add() to a zeroed StringInfo?  (Actually, now I'm not even sure if that's what happens... all the more reason for a comment :)

@@ +197,5 @@
> +        sizeOfAllStringChars += info.sizeOfAllStringChars;
> +        numCopies += info.numCopies;
> +    }
> +
> +    size_t totalSize() const {

totalSizeOf()?

@@ +201,5 @@
> +    size_t totalSize() const {
> +        return sizeOfShortStringGCThings + sizeOfNormalStringGCThings + sizeOfAllStringChars;
> +    }
> +
> +    size_t totalGCThingSize() const {

totalSizeOfGCThings()?

@@ +205,5 @@
> +    size_t totalGCThingSize() const {
> +        return sizeOfShortStringGCThings + sizeOfNormalStringGCThings;
> +    }
> +
> +    // The string's length.

Excluding any terminating null char?

@@ +217,5 @@
> +    size_t sizeOfNormalStringGCThings;
> +    size_t sizeOfAllStringChars;
> +};
> +
> +// Holds data about a notable string (one which uses more

s/more/more than/

@@ +239,5 @@
>          return js::MemoryReportingSundriesThreshold();
>      }
>  
> +    // The amount of memory we allocated for |buffer| (not
> +    // malloc_usable_size(buffer)).

This comment is confusing.  'malloc_usable_size(buffer)' is, by definition, exactly the amount of memory we allocated for |buffer|.  At this point I'm not sure what would be a better thing to say here.

(After reading below, I think you want to replace the parentheses with something like "(this may be larger than needed to hold the string, or it may be short enough that it's truncated when converted to ASCII with escapes)".  Or something.)

@@ +282,5 @@
>      // Add other's numbers to this object's numbers.
> +    void add(const ZoneStats &other) {
> +        ZoneStatsPod::add(other);
> +
> +        notableStrings.append(other.notableStrings);

At this point I'm unsure exactly what |notableStrings| is holding.  When you combine zones, I imagine it's possible that a string that wasn't notable in either of |this| and |other| becomes notable when you add them.  But this append() call doesn't account for that.

I think you might be better off not having |ZoneStats::notableStrings| -- the |strings| hash table should hold all the info you need, and you could compute the notable strings from it right at the end.  In fact you appear to do that in FindNotableStrings(), so now I'm confused about how its |notableStrings| manipulations interact with the ones in this function.

Or perhaps you need it just to have as a placeholder at the end, in which case it should remain empty until then, and you should assert that it's empty just before it gets used.  Assuming I'm right and you can do that, this might remove the need for some of the NotableStringInfo constructors.

[Actually, thinking even more, it's an interesting question whether a string that doesn't appear as notable for any individual zone should be able to appear as notable in the js-main-runtime summary.  The way you've handled |notableStrings|, the answer is "no";  the way you've handled |strings|, the answer is "yes".  I think the latter is probably best, though I'm open to arguments otherwise.]

::: js/src/jsmemorymetrics.cpp
@@ +44,5 @@
>  
> +/* static */ HashNumber
> +StringHashPolicy::hash(const Lookup &l)
> +{
> +    const jschar* chars = l->maybeChars();

'*' on the right.

@@ +56,5 @@
> +    return mozilla::HashBytes(chars, l->length() * sizeof(jschar));
> +}
> +
> +/* static */ bool
> +StringHashPolicy::match(const JSString* const &k, const Lookup &l)

|const JSString *const &k|

@@ +94,5 @@
> +
> +NotableStringInfo::NotableStringInfo(JSString *str, const StringInfo &info)
> +    : StringInfo(info)
> +{
> +    bufferSize = Min(str->length() + 1, size_t(4096));

Min(length, 4096)?  I guess it's because you don't know how long the string will be once you've converted it to ASCII with escapes, so you're just being conservative?  I can imagine cases where it'll be much more than necessary -- e.g. if you have a notable string of length 100, even in the worst case it'll only be 600 bytes.

Having said that, it's probably not worth trying to get this length exactly right in those cases... but it is worth a brief comment explaining that extra precision isn't worth the effort.

@@ +135,5 @@
> +
> +NotableStringInfo &NotableStringInfo::operator=(MoveRef<NotableStringInfo> info)
> +{
> +    this->~NotableStringInfo();
> +    new(this) NotableStringInfo(info);

|new (this)| is used in SpiderMonkey.

@@ +435,5 @@
> +        MOZ_ASSERT(zStats.gcHeapStringsNormal >= info.sizeOfNormalStringGCThings);
> +        MOZ_ASSERT(zStats.stringCharsNonNotable >= info.sizeOfAllStringChars);
> +        zStats.gcHeapStringsShort -= info.sizeOfShortStringGCThings;
> +        zStats.gcHeapStringsNormal -= info.sizeOfNormalStringGCThings;
> +        zStats.stringCharsNonNotable -= info.sizeOfAllStringChars;

I think the subtraction is because we're moving these values from the non-notable bucket into the notable bucket?  Might be worth a brief comment.

@@ +438,5 @@
> +        zStats.gcHeapStringsNormal -= info.sizeOfNormalStringGCThings;
> +        zStats.stringCharsNonNotable -= info.sizeOfAllStringChars;
> +    }
> +
> +    // zStats.strings holds unrooted JSString*'s, which we don't want to

s/JSString*'s/JSString pointers/ ?

@@ +486,5 @@
>  #endif
> +
> +        // Move any strings which take up more than the sundries threshold
> +        // (counting all of their copies together) into notableStrings.
> +        FindNotableStrings(zStats);

W.r.t. my comments earlier, I think |zStats->notableStrings| is empty at t this point...

@@ +491,3 @@
>      }
>  
> +    FindNotableStrings(rtStats->zTotals);

... but here, |rtStats->zTotals->notableStrings| won't be empty.  And then FindNotableStrings() will add to it (and maybe you'll end up with duplicates?)

If you don't do the append() in ZoneStats::add(), then I think the vector will be empty here.  And so you could assert at the top of FindNotableStrings that |notableStrings| is empty.

[Alternatively, if you think there shouldn't be any notable strings listed in js-main-runtime that aren't also notable in at least one zone, then |rtStats->zTotals->notableStrings| should already have the appropriate entries at this point, and the FindNotableStrings() call isn't necessary.]

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1471,5 @@
> +    size_t gcHeapStringsNormal = zStats.gcHeapStringsNormal;
> +    size_t stringCharsNonNotable = zStats.stringCharsNonNotable;
> +
> +    for (size_t i = 0; i < zStats.notableStrings.length(); i++) {
> +        const JS::NotableStringInfo& info = zStats.notableStrings[i];

'&' on the right.

@@ +1496,5 @@
> +        // use either a lot of gc-heap memory or a lot of string-chars memory,
> +        // but only strings of a certain sweet-spot length will have their
> +        // memory usage spread roughly equally between the two types of memory.
> +        //
> +        // We don't want to cluter up 'gc-heap/strings' with entries for notable

s/cluter/clutter/

@@ +1498,5 @@
> +        // memory usage spread roughly equally between the two types of memory.
> +        //
> +        // We don't want to cluter up 'gc-heap/strings' with entries for notable
> +        // strings which use relatively little GC memory, so instead we report a
> +        // notable in gc-heap/strings only if it uses |notableSize() / 2| or

s/notable in/notable string in/

Comment 32

[Nicholas Nethercote [inactive]]

Comment on attachment 782024 [details] [diff] [review]
Part 3, v2: Rework JS memory reporters so gc-heap is no longer a top-level node.

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

I've done a quick review of the code, with some comments below.  It basically looks good.

I haven't yet compiled and run it, because I figure I'll wait until you've fixed up the tricky parts of the previous patch, and then rebased this one.  When you do that, would you mind filing a new bug and moving the updated version of this patch to it?  Thanks!

BTW, in the previous patch there was a place where you subtracted some counts.  Can this patch remove that?

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1300,5 @@
>      "'explicit/js-non-window/runtime/temporary').");
>  
>  // The REPORT* macros do an unconditional report.  The ZCREPORT* macros are for
>  // compartments and zones; they aggregate any entries smaller than
> +// SUNDRIES_THRESHOLD into "sundries/js-gc-heap" and "sundries/heap" entries

"sundries/malloc-heap" for consistency with those below?

@@ +1421,5 @@
>  {
>      const nsAutoCString& pathPrefix = extras.pathPrefix;
>      size_t gcTotal = 0, gcHeapSundries = 0, otherSundries = 0;
>  
> +    ZCREPORT_GC_BYTES(pathPrefix + NS_LITERAL_CSTRING("arena-admin"),

"gc-arena-admin"?

@@ +1428,5 @@
>                        "heap, within arenas, that is used (a) to hold internal "
>                        "bookkeeping information, and (b) to provide padding to "
>                        "align GC things.");
>  
> +    ZCREPORT_GC_BYTES(pathPrefix + NS_LITERAL_CSTRING("unused-js-gc-things"),

Is "js-gc" necessary when we're withing a "js" sub-tree of one kind or another?  Probably not;  "gc" would suffice.  In which case, all the cases would have to be changed.

@@ +1439,5 @@
>                        zStats.gcHeapLazyScripts,
>                        "Memory on the garbage-collected JavaScript "
>                        "heap that represents scripts which haven't executed yet.");
>  
> +    ZCREPORT_GC_BYTES(pathPrefix + NS_LITERAL_CSTRING("type-objects"),

"type-objects/js-gc-things"?  Makes it consistent with ones like "lazy-scripts/js-gc-things", and about:memory will collapse it into a single line because there are no other children.

@@ +1444,5 @@
>                        zStats.gcHeapTypeObjects,
>                        "Memory on the garbage-collected JavaScript "
>                        "heap that holds type inference information.");
>  
> +    ZCREPORT_GC_BYTES(pathPrefix + NS_LITERAL_CSTRING("ion-codes"),

Ditto?

@@ +1455,1 @@
>                     zStats.lazyScripts,

All these field names mirror the path names.  I'm trying to decide whether to be a PITA and insist you change them all to match the new paths...

@@ +1505,1 @@
>                              "(length=%d, copies=%d, \"%s\"%s)",

I'd put the trailing '/' here, so you don't have to prepend it to the two children.

@@ +1514,5 @@
> +            "together use more than %d bytes total of JS GC heap and malloc heap "
> +            "memory.\n\n"
> +            "These headers may contain the string data itself, if the string "
> +            "is short enough.  If so, the string won't have any memory reported "
> +            "under 'string-chars/.",

"user 'malloc-heap/string-chars'." ?

@@ +1567,2 @@
>                          gcHeapSundries,
> +                        "The sum of all the js-gc-heap and js-gc-things measurements "

Single quotes around the sub-paths (sorry they weren't already there).

@@ +1575,2 @@
>                       nsIMemoryReporter::KIND_HEAP, otherSundries,
> +                     "The sum of all the malloc-heap measurements that are too "

Ditto.

@@ +1693,5 @@
>      JS_ASSERT(asmJSHeap == 0 || asmJSNonHeap == 0);
>      if (asmJSHeap > 0) {
> +        REPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("objects/malloc-heap/elements/asm.js"),
> +                     nsIMemoryReporter::KIND_HEAP, asmJSHeap,
> +                     "Memory allocated on the malloc heap for object element arrays uses as asm.js "

s/uses/used/

@@ +1699,5 @@
>      }
>      if (asmJSNonHeap > 0) {
> +        REPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("objects/non-heap/elements/asm.js"),
> +                     nsIMemoryReporter::KIND_NONHEAP, asmJSNonHeap,
> +                     "Memory allocated for object element arrays uses as asm.js array buffers. "

Ditto.

@@ +1957,5 @@
>      // We don't want to report decommitted memory in "explicit", so we just
>      // change the leading "explicit/" to "decommitted/".
>      nsCString rtPath2(rtPath);
>      rtPath2.Replace(0, strlen("explicit"), NS_LITERAL_CSTRING("decommitted"));
> +    REPORT_GC_BYTES(rtPath2 + NS_LITERAL_CSTRING("decommitted-arenas"),

Getting rid of the 'gc-heap' nodes looks good within compartments.  But for runtime-level things, I think it's better to keep the gc-heap trees.  Especially for "unused-chunks", "unused-arenas", and "chunk-admin", will all make sense as a group because they're different kinds of wasted GC heap space.  So I think all the changes from here to the end of the patch aren't necessary.

Comment 33

[Justin Lebar (not reading bugmail)]

It's going to take me twice as long to do this if I have to modify the patch below and then rebase this code.

If we're going to take this patch anyway, what is the benefit of this intermediate step?  I was under the impression that as a general rule we don't check in code that we're just about to change.

Comment 34

[Justin Lebar (not reading bugmail)]

Thanks for the comments.

>@@ +239,5 @@
>>          return js::MemoryReportingSundriesThreshold();
>>      }
>>  
>> +    // The amount of memory we allocated for |buffer| (not
>> +    // malloc_usable_size(buffer)).
>
>This comment is confusing.  'malloc_usable_size(buffer)' is, by definition,
>exactly the amount of memory we allocated for |buffer|.  At this point I'm not
>sure what would be a better thing to say here.

That depends on who "we" is.  I meant "we the programmer".

I'm not sure what to say here.  Maybe

  "This field holds |x|, in |buffer = malloc(x)|."

? But that's a mouthful.

>@@ +282,5 @@
>>      // Add other's numbers to this object's numbers.
>> +    void add(const ZoneStats &other) {
>> +        ZoneStatsPod::add(other);
>> +
>> +        notableStrings.append(other.notableStrings);
>
>At this point I'm unsure exactly what |notableStrings| is holding.  When you
>combine zones, I imagine it's possible that a string that wasn't notable in
>either of |this| and |other| becomes notable when you add them.  But this
>append() call doesn't account for that.

Good point.

I totally agree this is complicated; that's why I'm pushing back on writing it
twice.

I'm happy to write code on top of or part of part 2 that does this correctly.

>[Actually, thinking even more, it's an interesting question whether a string
>that doesn't appear as notable for any individual zone should be able to
>appear as notable in the js-main-runtime summary.  The way you've handled
>|notableStrings|, the answer is "no";  the way you've handled |strings|, the
>answer is "yes".  I think the latter is probably best, though I'm open to
>arguments otherwise.]

I also think "yes" is better than "no" here.

>> +NotableStringInfo::NotableStringInfo(JSString *str, const StringInfo &info)
>> +    : StringInfo(info)
>> +{
>> +    bufferSize = Min(str->length() + 1, size_t(4096));
>
>Min(length, 4096)?  I guess it's because you don't know how long the string
>will be once you've converted it to ASCII with escapes, so you're just being
>conservative?  I can imagine cases where it'll be much more than necessary --
>e.g. if you have a notable string of length 100, even in the worst case it'll
>only be 600 bytes.

Did you think this was Max()?

Comment 35

[Nicholas Nethercote [inactive]]

> That depends on who "we" is.  I meant "we the programmer".

How about "the amount requested".


> Good point.
> 
> I totally agree this is complicated; that's why I'm pushing back on writing
> it twice.
> 
> I'm happy to write code on top of or part of part 2 that does this correctly.

I assume by the "writing it twice" you're talking about parts of the code that are affected by both part 2 and part 3?  But this function isn't such a part, AFAICT.

> I also think "yes" is better than "no" here.

I'm fine with that, so long as the code does it consistently.


> >Min(length, 4096)?  I guess it's because you don't know how long the string
> >will be once you've converted it to ASCII with escapes, so you're just being
> >conservative?  I can imagine cases where it'll be much more than necessary --
> >e.g. if you have a notable string of length 100, even in the worst case it'll
> >only be 600 bytes.
> 
> Did you think this was Max()?

Yeah.  Ignore what I said.

Comment 36

[Justin Lebar (not reading bugmail)]

> But this function isn't such a part, AFAICT.

Indeed; if all that's required is to modify this one function, it's not a big deal.  I initially thought fixing this required a more global change.  Now I'm not as sure.

If I can do this easily, I will.  But I was already burned once by thinking that a change like this would be easy, and I still don't understand why it's important that I write and then rewrite this code.

> I think you might be better off not having |ZoneStats::notableStrings| -- the |strings| 
> hash table should hold all the info you need, and you could compute the notable strings 
> from it right at the end.

I'm not sure where is "right at the end", but notableStrings is there because raw JSString* is safe only so long as we can't GC (aiui).  Multi-reporter callbacks run JS and so can GC, so before I start running those callbacks I need to convert everything to char*.

So I think we need something like notableStrings, somewhere.

> Is "js-gc" necessary when we're withing a "js" sub-tree of one kind or another?

I took a cue from the memory reporter descriptions, which all say "garbage-collected JavaScript heap".  "gc heap" probably screams "JS" to SpiderMonkey hackers, but perhaps not to everyone else.

I don't really care, but I wanted to give you a chance to change your mind.  :)

Comment 37

[Nicholas Nethercote [inactive]]

Just merge the two damned patches and quit complaining.

Comment 38

[Justin Lebar (not reading bugmail)]

> notableStrings.append(other.notableStrings);

You're totally right about this line being bogus.

In these patches I was trying to avoid a false collision where two long strings with the same length and prefix get merged into one notable string.  So that's why I aggregate things as JSString*'s first.  But obviously the line above isn't going to work.

Comment 39

[Justin Lebar (not reading bugmail)]

There is a bug in our current huge strings reporter where we replace "/" with "\/", while in about:memory we merely flip backslashes to forward slashes.

With this patch, we report shorter strings, which causes some strings generated by about:memory to show up in about:memory, after you measure twice.  Then I think this bug causes us to incorrectly nest things, because strings have forward slashes in them before we flip backslashes.

I'm going to fix this to replace "/" with "\", but we might want to think about whether we can switch to "\/" instead of "/" -- that way, we can output strings which have both "/" and "\" in them.

Comment 40

[Justin Lebar (not reading bugmail)]

Attachment: Part 1.5, v1: Fix escaping for huge strings displayed in about:memory.

We were escaping "/" to "\/", but about:memory expects us to escape "/" to "\".

Escaping "/" to "\/" is probably quite sensible, since then we can represent strings which have both "\" and "/".  But that's a bigger change.

Comment 41

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Modify the JS memory reporter to consider a string as "notable" if we have many small copies of it.

Comment 42

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v4: Modify the JS memory reporter to consider a string as "notable" if we have many small copies of it.

Comment 43

[Nicholas Nethercote [inactive]]

Comment on attachment 782742 [details] [diff] [review]
Part 2, v4: Modify the JS memory reporter to consider a string as "notable" if we have many small copies of it.

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

r=me, but there's one more thing to discuss, which I'll do in the next comment.

::: js/public/MemoryMetrics.h
@@ +216,5 @@
> +// NotableStringInfo::notableSize() bytes of memory), so we can report it
> +// individually.
> +//
> +// Essentially the only difference between this class and StringInfo is that
> +// NotableStringInfo holds a copy of the string's data.

s/data/chars/ ?

@@ +266,1 @@
>      {

Opening brace goes on previous line.

@@ +276,5 @@
> +    // Add other's numbers to this object's numbers.  Both objects'
> +    // notableStrings vectors must be empty at this point, because we can't
> +    // merge them.  (A NotableStringInfo contains only a prefix of the string,
> +    // so we can't tell whether two NotableStringInfo objects correspond to the
> +    // same string.)

Good comment.

::: js/src/jsmemorymetrics.cpp
@@ +414,5 @@
>  
> +static void
> +FindNotableStrings(ZoneStats &zStats)
> +{
> +    using namespace JS;

Can you assert that zStats.notableStrings is empty here?

@@ +417,5 @@
> +{
> +    using namespace JS;
> +
> +    for (ZoneStats::StringsHashMap::Range r = zStats.strings.all();
> +         !r.empty(); r.popFront()) {

SM style for multi-line for loop headers is this:

for (ZoneStats::StringsHashMap::Range r = zStats.strings.all();
     !r.empty();
     r.popFront())
{

@@ +420,5 @@
> +    for (ZoneStats::StringsHashMap::Range r = zStats.strings.all();
> +         !r.empty(); r.popFront()) {
> +
> +        JSString *str = r.front().key;
> +        StringInfo& info = r.front().value;

'&' on the right.

Comment 44

[Nicholas Nethercote [inactive]]

If you do two verbose measurements in a row in about:memory, you end up with lots of entries like this:

> │  │   └────848,080 B (01.95%) -- notable
> │  │        ├──128,000 B (00.29%) ── string(length=346, copies=126, "Memory allocated to hold headers for copies of the given notable string.  A string is notable if all of its copies together use more than 8192 bytes total of JS GC heap and malloc heap memory./n/nThese headers may contain the string data itself, if the string is short enough.  If so, the string won't have any memory reported under 'string-chars")
> │  │        ├──102,816 B (00.24%) ── string(length=161, copies=306, "Memory on the garbage-collected JavaScript heap that holds shapes that (a) are in a property tree, and (b) represent an object whose parent is the global object.")
> │  │        ├───84,448 B (00.19%) ── string(length=96, copies=406, "The sum of all the non-gc-heap measurements that are too small to be worth showing individually.")
> │  │        ├───80,256 B (00.18%) ── string(length=92, copies=418, "The sum of all the gc-heap measurements that are too small to be worth showing individually.")
> │  │        ├───62,464 B (00.14%) ── string(length=260, copies=61, "Memory allocated for the non-fixed object slot arrays, which are used to represent object properties. Some objects also contain a fixed number of slots which are stored on the JavaScript heap; those slots are not counted here, but in 'gc-heap/objects' instead.")
> │  │        ├───56,992 B (00.13%) ── string(length=100, copies=274, "Memory used by compartment-wide tables storing shape information for use during object construction.")
> │  │        ├───55,744 B (00.13%) ── string(length=200, copies=134, "Memory on the garbage-collected JavaScript heap that holds JSScript instances. A JSScript is created for each user-defined function in a script. One is also created for the top-level code in a script.")
> │  │        ├───51,840 B (00.12%) ── string(length=76, copies=324, "Memory on the garbage-collected JavaScript heap that holds function objects.")
> │  │        ├───29,696 B (00.07%) ── string(length=32, copies=232, "shapes-extra/compartment-tables/")
> │  │        ├───26,112 B (00.06%) ── string(length=89, copies=136, "Memory on the garbage-collected JavaScript heap that collates data common to many shapes.")
> │  │        ├───17,408 B (00.04%) ── string(length=351, copies=17, "Memory allocated to hold characters of strings whose characters take up less than than 8192 bytes of memory./n/nSometimes more memory is allocated than necessary, to simplify string concatenation.  Each string also includes a header which is stored on the compartment's JavaScript heap; that header is not counted here, but in 'gc-heap/strings' instea")
> │  │        ├───17,408 B (00.04%) ── string(length=389, copies=17, "Memory on the garbage-collected JavaScript heap that holds headers for strings that are not short and that don't appear under 'gc-heap/strings/notable*'./n/n String headers contain various pieces of information about a string, but do not contain (except in the case of very short strings) the string characters;  characters in longer strings are counted under 'gc-heap/string-chars' instea")
> │  │        ├───16,896 B (00.04%) ── string(length=72, copies=66, "This value is the sum of 2 memory reporters that all have the same path.")
> │  │        ├───16,048 B (00.04%) ── string(length=131, copies=59, "Memory on the garbage-collected JavaScript heap that holds ordinary (i.e. not otherwise distinguished my memory reporters) objects.")
> │  │        ├───14,976 B (00.03%) ── string(length=31, copies=234, "shapes-extra/compartment-tables")
> │  │        ├───12,400 B (00.03%) ── string(length=198, copies=31, "Memory allocated to hold string data for copies of the given notable string.  A string is notable if all of its copies together use more than 8192 bytes total of JS GC heap and malloc heap memory./n")
> │  │        ├───12,096 B (00.03%) ── string(length=107, copies=54, "Memory used by the event targets table in a window's DOM, and the objects it points to, which include XHRs.")
> │  │        ├───10,880 B (00.03%) ── string(length=28, copies=170, "shapes/tree/global-parented/")
> │  │        ├────9,776 B (00.02%) ── string(length=102, copies=47, "Memory on the garbage-collected JavaScript heap taken by empty GC thing slots within non-empty arenas.")
> │  │        ├────8,960 B (00.02%) ── string(length=74, copies=56, "file:///home/njn/moz/mi9/d64/dist/bin/chrome/toolkit/res/hiddenWindow.html")
> │  │        ├────8,736 B (00.02%) ── string(length=109, copies=39, "Memory (approximate) used to store the schema for all databases associated with connections to this database.")
> │  │        ├────8,640 B (00.02%) ── string(length=78, copies=54, "Memory used by a window's DOM that isn't measured by the other 'dom/' numbers.")
> │  │        ├────8,000 B (00.02%) ── string(length=24, copies=125, "/u2502   /u2502  /u2502 ")
> │  │        └────7,488 B (00.02%) ── string(length=90, copies=39, "Memory (approximate) used by all prepared statements used by connections to this database.")
> │  ├──────65,536 B (00.15%) ── type-pool


Some of these are memory report descriptions.  Some of them are paths or sub-paths.

This is new because these strings are not terribly long -- so they didn't show up as huge strings -- but there can be many copies of them, so they show up as notable.

That's a small example, where I just had about:memory loaded.  If I load techcrunch.com in a 2nd tab and then measure twice, I get *many* more such entries, including nested ones like this:

> 16,384 B (00.00%) ── string(length=4141, copies=1, "string(length=4140, copies=2, "string(length=4183, copies=2, "js-main-runtime/zones/string-chars/notable/string(length=4234, copies=1, "..."

It doesn't always happen;  I guess it depends on whether GC runs after the previous about:memory generation occurs.  But in the worst case I end up with something like 75% of the entries in about:memory being notable strings relating to about:memory itself.  This completely destroys about:memory's usefulness, and so this patch cannot land without something being done about this.

I'm not sure how to fix this.  There's an argument to be made that about:memory and the memory reporters are profligate with strings, but that's not easy to fix.

I'm also getting fairly frequent OOM crashes due to str->getCharsNonDestructive() failing inside NotableStringInfo().  I'm not sure why.  I'll do some digging.

Comment 45

[Nicholas Nethercote [inactive]]

> I'm also getting fairly frequent OOM crashes due to
> str->getCharsNonDestructive() failing inside NotableStringInfo().

Oh.  It's because you forgot the ! in front of str->getCharsNonDestructive().

Comment 46

[Nicholas Nethercote [inactive]]

Some testing of this code would be good.  test_memoryReporters.xul is the obvious place.  It doesn't need to be intricate, but some basic coverage would be good.

Comment 47

[Justin Lebar (not reading bugmail)]

> Some testing of this code would be good.

The idea is to declare a large string and/or a series of small strings and then ensure that they show up in the memory report?

> But in the worst case I end up with something like 75% of the entries in about:memory being notable 
> strings relating to about:memory itself.  This completely destroys about:memory's usefulness, and so 
> this patch cannot land without something being done about this.

I guess the problem is particularly in the aggregated JS reporter outside explicit?  Because inside explicit, all of about:memory's strings are cordoned off anyway.

Or is your argument that this introduces a lot more churn into about:memory's memory usage, and that throws off our numbers?

Comment 48

[Nicholas Nethercote [inactive]]

> The idea is to declare a large string and/or a series of small strings and
> then ensure that they show up in the memory report?

Yes.

> I guess the problem is particularly in the aggregated JS reporter outside
> explicit?  Because inside explicit, all of about:memory's strings are
> cordoned off anyway.
> 
> Or is your argument that this introduces a lot more churn into
> about:memory's memory usage, and that throws off our numbers?

The churn isn't great, but the bigger problem was just that when it happens badly, 75% of about:memory's contents was notable strings from about:memory, which badly obfuscated the interesting measurements.

----

On a different note, you can use mozilla::HashString() on a jschar* string instead of HashBytes().

Comment 49

[Justin Lebar (not reading bugmail)]

> the bigger problem was just that when it happens badly, 75% of about:memory's contents was notable 
> strings from about:memory, which badly obfuscated the interesting measurements.

But it's just the ones outside the about:memory compartment that are the problem, right?  Because you can compact the about:memory compartment to make them go away there.

I'm also not sure what to do about this.  We could hack about:memory so that it forces the |notable| strings closed, even if there are a lot of them.  Or we could add another mode, orthogonal to verbose, which allow us to toggle whether or not we collect strings.

Comment 50

[Nicholas Nethercote [inactive]]

> But it's just the ones outside the about:memory compartment that are the
> problem, right?  Because you can compact the about:memory compartment to
> make them go away there.

But strings are in zones now, not compartments, so the about:memory strings get mixed up with lots of other compartments' strings :(

Comment 51

[Nicholas Nethercote [inactive]]

I have better steps to reproduce the nasty cases.

- Open techcrunch.com, scroll down over a few stories
- Open about:memory in a second tab
- Click 'verbose'
- Click 'measure' repeatedly.  As 'explicit' increases, things will get worse.  (If 'explicit' drops, it will get better.)  Try to get 'explicit' to increase, say, five times in a row.
- At this point, things will be bad.

Below are two kinds of multiply-nested strings, of the sort that cause problems.  It would be a hack, but if we didn't show any notable string that contained the substring "string(length=" I think that would fix most of the problem.  We'd still have various cases like this:

│   │  │   │  ├────244,096 B (00.15%) ── string(length=8, copies=3814, "explicit")

but there are only a few dozen like that, which isn't so bad.



│   │  │  ├──────32,768 B (00.02%) ── string(length=4141, copies=2, "string(length=4140, copies=2, "string(length=4140, copies=2, "string(length=4140, copies=2, "string(length=5351, copies=1, "* html #{0} a#{0}-link{height:1% !important;}#{0}{position:relative !important;overflow:visible !important;display:block !important;}#{0} a#{0}-link{border:0 !important;height:20px !important;text-decoration:none !important;padding:0 !important;margin:0 !important;display:inline-block !important;}#{0} a#{0}-link:link, #{0} a#{0}-link:visited, #{0} a#{0}-link:hover, #{0} a#{0}-link:active{border:0 !important;text-decoration:none !important;}#{0} a#{0}-link:after{content:"." !important;display:block !important;clear:both !important;visibility:hidden !important;line-height:0 !important;height:0 !important;}#{0} #{0}-logo{background:url(http://static.licdn.com/scds/common/u/img/sprite/sprite_connect_v13.png) 0px -276px no-repeat !important;cursor:pointer !important;border:0 !important;text-indent:-9999em !important;overflow:hidden !important;padding:0 !important;margin:0 !important;position:absolute !important;left:0px !important;top:0px !important;display:block !important;width:20px !important;height:20px !important;float:right !important;}#{0}.hovered #{0}-logo{background-position:-20px -276px !important;}#{0}.clicked #{0}-logo, #{0}.down #{0}-logo{background-position:-40px -276px !important;}.IN-shadowed #{0} #{0}-logo{}#{0} #{0}-title{color:#333 !important;cursor:pointer !important;display:block !important;white-space:nowrap !important;float:left !important;margin-left:1px !important;vertical-align:top !important;overflow:hidden !important;text-align:center !important;height:18px !important;padding:0 4px 0 23px !important;border:1px solid #000 !important;border-top-color:#E2E2E2 !important;border-right-color:#BFBFBF !important;border-bottom-color:#B9B9B9 !important;border-left-color:#E2E2E2 !important;border-left:0 !important;text-shadow:#FFFFFF -1px 1px 0 !important;line-height:20px !important;border-radius:0 !important;-moz-border-radius:0 !important;border-top-right-radius:2px !important;border-bottom-right-radius:2px !important;-moz-border-radius-topright:2px !important;-moz-border-radius-bottomright:2px !important;background-color:#ECECEC !important;background-image:-moz-linear-gradient(top, #FEFEFE 0%, #ECECEC 100%) !important;}#{0}.hovered #{0}-title{border:1px solid #000 !important;border-top-color:#ABABAB !important;border-right-color:#9A9A9A !important;border-bottom-color:#787878 !important;border-left-color:#04568B !important;border-left:0 !important;background-color:#EDEDED !important;background-image:-moz-linear-gradient(top, #EDEDED 0%, #DEDEDE 100%) !important;}#{0}.clicked #{0}-title, #{0}.down #{0}-title{color:#666 !important;border:1px solid #000 !important;border-top-color:#B6B6B6 !important;border-right-color:#B3B3B3 !important;border-bottom-color:#9D9D9D !important;border-left-color:#49627B !important;border-left:0 !important;background-color:#DEDEDE !important;background-image:-moz-linear-gradient(top, #E3E3E3 0%, #EDEDED 100%) !important;}.IN-shadowed #{0} #{0}-title{}.IN-shadowed #{0}.hovered #{0}-title{}.IN-shadowed #{0}.clicked #{0}-title, .IN-shadowed #{0}.down #{0}-title{}#{0} #{0}-title-text, #{0} #{0}-title-text *{color:#333 !important;font-size:11px !important;font-family:Arial, sans-serif !important;font-weight:bold !important;font-style:normal !important;display:inline-block !important;background:transparent none !important;vertical-align:baseline !important;height:18px !important;line-height:20px !important;float:none !important;}#{0} #{0}-title-text strong{font-weight:bold !important;}#{0} #{0}-title-text em{font-style:italic !important;}#{0}.hovered #{0}-title-text, #{0}.hovered #{0}-title-text *{color:#000 !important;}#{0}.clicked #{0}-title-text, #{0}.down #{0}-title-text, #{0}.clicked #{0}-title-text *, #{0}.down #{0}-title-text *{color:#666 !important;}#{0} #{0}-title #{0}-mark{display:inline-block !important;width:0px !important;overflow:hidden !important;}.success #{0} #{0}-title{color:#333 !important;border-top-color:#E2E2E2 !important;border-right-color:#" (truncated))

│   │  │  ├───────8,192 B (00.00%) ── string(length=2543, copies=1, "js-main-runtime/zones/string-chars/notable/string(length=2467, copies=1, "explicit//window-objects//top(about:memory, id=8)//js-zone(0x7fc7bcfb8800)//string-chars//notable//string(length=2341, copies=1, "explicit////window-objects////top(about:memory, id=8)////js-zone(0x7fc7bcfb8800)////string-chars////notable////string(length=2215, copies=1, "js-main-runtime////////zones////////string-chars////////notable////////string(length=2139, copies=1, "timestamp,dynamicVariablePrefix,visitorID,fid,vmk,visitorMigrationKey,visitorMigrationServer,visitorMigrationServerSecure,ppu,charSet,visitorNamespace,cookieDomainPeriods,cookieLifetime,pageName,pageURL,referrer,contextData,currencyCode,lightProfileID,lightStoreForSeconds,lightIncrementBy,retrieveLightProfiles,deleteLightProfiles,retrieveLightData,variableProvider,channel,server,pageType,transactionID,purchaseID,campaign,state,zip,events,events2,products,linkName,linkType,prop1,eVar1,prop2,eVar2,prop3,eVar3,prop4,eVar4,prop5,eVar5,prop6,eVar6,prop7,eVar7,prop8,eVar8,prop9,eVar9,prop10,eVar10,prop11,eVar11,prop12,eVar12,prop13,eVar13,prop14,eVar14,prop15,eVar15,prop16,eVar16,prop17,eVar17,prop18,eVar18,prop19,eVar19,prop20,eVar20,prop21,eVar21,prop22,eVar22,prop23,eVar23,prop24,eVar24,prop25,eVar25,prop26,eVar26,prop27,eVar27,prop28,eVar28,prop29,eVar29,prop30,eVar30,prop31,eVar31,prop32,eVar32,prop33,eVar33,prop34,eVar34,prop35,eVar35,prop36,eVar36,prop37,eVar37,prop38,eVar38,prop39,eVar39,prop40,eVar40,prop41,eVar41,prop42,eVar42,prop43,eVar43,prop44,eVar44,prop45,eVar45,prop46,eVar46,prop47,eVar47,prop48,eVar48,prop49,eVar49,prop50,eVar50,prop51,eVar51,prop52,eVar52,prop53,eVar53,prop54,eVar54,prop55,eVar55,prop56,eVar56,prop57,eVar57,prop58,eVar58,prop59,eVar59,prop60,eVar60,prop61,eVar61,prop62,eVar62,prop63,eVar63,prop64,eVar64,prop65,eVar65,prop66,eVar66,prop67,eVar67,prop68,eVar68,prop69,eVar69,prop70,eVar70,prop71,eVar71,prop72,eVar72,prop73,eVar73,prop74,eVar74,prop75,eVar75,hier1,hier2,hier3,hier4,hier5,list1,list2,list3,tnt,pe,pev1,pev2,pev3,resolution,colorDepth,javascriptVersion,javaEnabled,cookiesEnabled,browserWidth,browserHeight,connectionType,homepage,pageURLRest,plugins,trackingServer,trackingServerSecure,trackingServerBase,fpCookieDomainPeriods,disableBufferedRequests,mobile,visitorSampling,visitorSamplingGroup,dynamicAccountSelection,dynamicAccountList,dynamicAccountMatch,trackDownloadLinks,trackExternalLinks,trackInlineStats,linkLeaveQueryString,linkDownloadFileTypes,linkExternalFilters,linkInternalFilters,linkTrackVars,linkTrackEvents,li")

Comment 52

[Nicholas Nethercote [inactive]]

> SM style for multi-line for loop headers is this:
> 
> for (ZoneStats::StringsHashMap::Range r = zStats.strings.all();
>      !r.empty();
>      r.popFront())
> {

Oh, but SM style also allows 99-char lines for code, so this can go on a single line.

Comment 53

[Nicholas Nethercote [inactive]]

Attachment: Part 3b: Avoid lots of nested "string(length=" strings.

This patch implements the "string(length=" hack.  It works pretty well.  In the
worst case, you can still have 100s of strings like this:

│   │     ├──────10,240 B (00.01%) ── string(length=582, copies=5, "Main
Process:explicit/window-objects/top(http://techcrunch.com/,
id=8)/active/window(https://apis.google.com/_/+1/fastbutton?bsv&size=medium&hl=en-US&origin=http%3A%2F%2Ftechcrunch.com&url=http%3A%2F%2Ftechcrunch.com%2F2013%2F07%2F30%2Fslidepay%2F&gsrc=3p&jsh=m%3B%2F_%2Fscs%2Fapps-static%2F_%2Fjs%2Fk%3Doz.gapi.en.ccWIpBrYT5k.O%2Fm%3D__features__%2Fam%3DEQ%2Frt%3Dj%2Fd%3D1%2Frs%3DAItRSTMAZJk5WDYkdf7P_ZqZap0b82OjJw#_methods=onPlusOne%2C_ready%2C_close%2C_open%2C_resizeMe%2C_renderstart%2Concircled&id=I2_1375235431907&parent=http%3A%2F%2Ftechcrunch.com&pfname=&rpctoken=42385104)")

but it avoids potentially 1000s of nested "string(length=" strings.

W.r.t. the code itself, the hardcoding of "string(length=" in |pat| is kinda
gross.  Ideally we'd factor that out along with the same string occurrence in
ReportZoneStats, but one's in JS and the other's in xpconnect, and one is a
jschar* and the other is char*...  feel free to improve it if you want!

Comment 54

[Nicholas Nethercote [inactive]]

I wonder if it would be a good idea to increase the notable string threshold to 16 KiB.  8 KiB feels a little small, now.

Comment 55

[Justin Lebar (not reading bugmail)]

(In reply to Nicholas Nethercote [:njn] from comment #54)
> I wonder if it would be a good idea to increase the notable string threshold
> to 16 KiB.  8 KiB feels a little small, now.

I'd be happy to do 16kb on desktop and 8kb on B2G.

Comment 56

[Justin Lebar (not reading bugmail)]

> if (!str->isLinear())
>   MOZ_CRASH("non-linear");

I'm not sure why we can assume that our strings are linear; I thought that was the whole reason I had to do the rigamarole with getCharsNonDestructive.

If you don't mind, I think it would be better to do this check in XPCJSRuntime instead.  We're already applying logic to the result we get out of JS (to determine which notability bucket(s) the string goes into), and this way we destructively filter the data at the last possible moment, which seems good.

Comment 57

[Justin Lebar (not reading bugmail)]

> On a different note, you can use mozilla::HashString() on a jschar* string instead of 
> HashBytes().

In your other bug you may want to try using HashBytes instead of HashString.  HashBytes goes one word at a time, while HashString goes one (js)char at a time.

If HashBytes is faster, maybe we should make HashString call HashBytes.

Comment 58

[Justin Lebar (not reading bugmail)]

Attachment: Part 4: Elide notable strings which contain the string 'string(length='.

Comment 59

[Justin Lebar (not reading bugmail)]

> I'd be happy to do 16kb on desktop and 8kb on B2G.

But maybe we can do this separately, as this bug is getting kind of long?

Comment 60

[Nicholas Nethercote [inactive]]

Comment on attachment 783675 [details] [diff] [review]
Part 4: Elide notable strings which contain the string 'string(length='.

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

Looks good.  This seems worth merging into the previous patch before landing.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1484,5 @@
> +        // there's a GC in the meantime), and so on ad infinitum.
> +        //
> +        // To avoid cluttering up about:memory like this, we stick notable
> +        // strings which contain "string(length=" into their own bucket.
> +        if (FindInReadable(NS_LITERAL_CSTRING("string(length="), notableString)) {

Now that you're doing this in XPCJSRuntime, you should be able to factor out "string(length=" into a variable and then use it here and also when generating the corresponding paths.

@@ +1486,5 @@
> +        // To avoid cluttering up about:memory like this, we stick notable
> +        // strings which contain "string(length=" into their own bucket.
> +        if (FindInReadable(NS_LITERAL_CSTRING("string(length="), notableString)) {
> +            gcHeapStringsAboutMemory += info.totalGCThingSizeOf();
> +            stringCharsAboutMemory += info.sizeOfAllStringChars;

At first I thought "If we're adding to these buckets, do we need to subtract from other buckets?"  But looking more, I don't think that's right.  And if it was, this assertion would be failing:

  MOZ_ASSERT(gcTotal == rtStats.gcHeapChunkTotal);

and presumably it's not.

Comment 61

[Nicholas Nethercote [inactive]]

> If HashBytes is faster, maybe we should make HashString call HashBytes.

I just measured.  On 64-bit, HashBytes() executes slightly more instructions.  In my most compelling case (parsing the Unreal demo, as per bug 899834), switching to HashBytes() increased the instruction count by 2.5% and slowed it down by about 3%.  I suspect the memcpy() and the post-word-loop check are the cause.

Comment 62

[Justin Lebar (not reading bugmail)]

Interesting that the compiler isn't optimizing out the memcpy.  Anyway, thanks for checking.

Comment 63

[Justin Lebar (not reading bugmail)]

> At first I thought "If we're adding to these buckets, do we need to subtract from other buckets?"  
> But looking more, I don't think that's right.

Although this is correct, I /do/ need to do

  gcTotal += info.totalGCThingSize()

which I was missing before!

I don't feel like factoring out "string(length=" makes sense; it's in the middle of a longer string, so I'd have to do something like

  nsCString path = pathPrefix + nsPringfCString("strings/notable/%s%d, copies=%d)", lengthToken.get(), info.length, info.numCopies);

Are you OK if I leave it how it is?

Comment 64

[Nicholas Nethercote [inactive]]

> Although this is correct, I /do/ need to do
> 
>   gcTotal += info.totalGCThingSize()

Were you getting assertion failures?  gcTotal is supposed to be checked...

> I don't feel like factoring out "string(length=" makes sense; it's in the
> middle of a longer string, so I'd have to do something like
> 
>   nsCString path = pathPrefix + nsPringfCString("strings/notable/%s%d,
> copies=%d)", lengthToken.get(), info.length, info.numCopies);
> 
> Are you OK if I leave it how it is?

If you use a macro it's simpler:

  #define STRING_LENGTH  "string(length="

  nsCString path = pathPrefix + "strings/notable/" STRING_LENGTH "%d, copies=%d, info.length, info.numCopies);

Comment 65

[Justin Lebar (not reading bugmail)]

> Were you getting assertion failures?  gcTotal is supposed to be checked...

I wasn't, which is rather odd.  It's possible I had an opt build going, although I don't think so.

> If you use a macro it's simpler:

I still don't buy it, but okay.

Comment 66

[Justin Lebar (not reading bugmail)]

> I wasn't, which is rather odd.

Oh, I see.  It was right now it was, because I was doing ZCREPORT_GC_BYTES of this value.

I only need to do it for huge strings because I can't use that macro.

Comment 67

[Justin Lebar (not reading bugmail)]

> because I can't use that macro.

...because I don't want huge strings to get sundrified.

Comment 68

[Justin Lebar (not reading bugmail)]

Attachment: Part 5: Tests.

Does this look OK?  I didn't want to box us in to an exact memory reporter path; that's just annoying if we want to change things.

Comment 69

[Nicholas Nethercote [inactive]]

Comment on attachment 784726 [details] [diff] [review]
Part 5: Tests.

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

Perfect!

Comment 70

[Justin Lebar (not reading bugmail)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4bdf8611ec57
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3baebe7cc655
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d01358ff4b15
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8220cb61d5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8b6ba99c05

Comment 71

[Justin Lebar (not reading bugmail)]

Bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/4aa234138f44

Comment 72

[Justin Lebar (not reading bugmail)]

Backed out for bustage.  :(  Our good friend -Werror.

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

Comment 73

[Justin Lebar (not reading bugmail)]

Into try: https://tbpl.mozilla.org/?tree=Try&rev=4e5aa098a7b1

Comment 74

[Justin Lebar (not reading bugmail)]

That was a bad base rev.  Let's try again.

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

Comment 75

[Justin Lebar (not reading bugmail)]

Almost there...

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

Comment 76

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2634e0f7656
https://hg.mozilla.org/integration/mozilla-inbound/rev/0482f992a8f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e603eea759e
https://hg.mozilla.org/integration/mozilla-inbound/rev/644491cfe22d
https://hg.mozilla.org/integration/mozilla-inbound/rev/26f820862035

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c2634e0f7656
https://hg.mozilla.org/mozilla-central/rev/0482f992a8f7
https://hg.mozilla.org/mozilla-central/rev/8e603eea759e
https://hg.mozilla.org/mozilla-central/rev/644491cfe22d
https://hg.mozilla.org/mozilla-central/rev/26f820862035