972712 - Report more notable stuff in the JS memory reporter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Nicholas Nethercote [inactive]]

Currently all script sources get lumped into a single bucket in about:memory.
We can do better:
- identify files that have significant source size;
- distinguished compressed and uncompressed script sources.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Report script sources in more detail.

This patch implements more detailed reporting of script sources. It's heavily
inspired by the existing reporting of notable strings.

It also adds some OOM checking that was missing in the existing notable string
code.

Here is some sample output. Things to note:

- Files with more than 16KiB of source data are identified as "notable"; the
  rest are aggregated as "normal".

- Multiple script-sources can come from a single file, if the file is XML
  (thanks to CDATA sections). I've used "fragments" for this count, but I'm
  open to other suggestions. Maybe just "scripts"?

- Some script-sources don't have a filename. I don't know why. They get
  aggregated under "(none)".

> ├───1,665,400 B (00.58%) -- script-sources
> │   ├──1,272,296 B (00.44%) -- notable
> │   │  ├────136,504 B (00.05%) -- source(fragments=204, chrome://browser/content/tabbrowser.xml)
> │   │  │    ├───88,224 B (00.03%) ── compressed
> │   │  │    ├───29,376 B (00.01%) ── misc
> │   │  │    └───18,904 B (00.01%) ── uncompressed
> │   │  ├────135,328 B (00.05%) -- source(fragments=1, http://js.nyt.com/js2/build/sitewide/sitewide.js)
> │   │  │    ├──135,168 B (00.05%) ── compressed
> │   │  │    └──────160 B (00.00%) ── misc
> ...
> │   │  ├─────33,936 B (00.01%) -- source(fragments=303, (none))
> │   │  │     ├──29,088 B (00.01%) ── misc
> │   │  │     └───4,848 B (00.00%) ── uncompressed
> ...
> │   │  └─────16,512 B (00.01%) -- source(fragments=1, http://js.nyt.com/js/mtr.js)
> │   │        ├──16,384 B (00.01%) ── compressed
> │   │        └─────128 B (00.00%) ── misc
> │   └────393,104 B (00.14%) -- normal
> │        ├──281,712 B (00.10%) ── compressed
> │        ├───82,352 B (00.03%) ── misc
> │        └───29,040 B (00.01%) ── uncompressed

Comment 2

[Till Schneidereit [:till]]

Comment on attachment 8376029 [details] [diff] [review]
Report script sources in more detail.

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

I'm in favor of renaming fragment to script, definitely. That's what we're calling them everywhere else (and what they are, really) after all.

So, r=me with feedback addressed, an explanation for the "/" escaping thing, and s/fragment/script/g.

::: js/public/MemoryMetrics.h
@@ +284,5 @@
>      size_t mallocHeap;
>  };
>  
> +// Holds data about a notable string (one which, counting all duplicates, uses
> +// a certain amount of memory) so we can report it individually.

"more than a certain [...]"?

@@ +388,5 @@
>      macro(_, _, sourceDataCache) \
>      macro(_, _, scriptData) \
> +    macro(_, _, scriptSourcesNormalCompressed) \
> +    macro(_, _, scriptSourcesNormalUncompressed) \
> +    macro(_, _, scriptSourcesNormalMisc)

Why do these have "Normal" in their name? At least in this list, normal seems to be the only flavor scriptSources come in. During my review, I kept looking for other flavors to show up. I realize that this is in contrast to the notable sources, the sizes of which aren't stored here. Maybe either rename to nonNotableSources or also rename the scriptSources and notableScriptSources below to scriptSourcesNormal and scriptSourcesNotable?

::: js/src/vm/MemoryMetrics.cpp
@@ +176,5 @@
> +  : ScriptSourceInfo(info)
> +{
> +    size_t bytes = strlen(filename) + 1;
> +    filename_ = js_pod_malloc<char>(bytes);
> +    if (!filename_) {

Nit: no braces needed

@@ +536,5 @@
> +        const char *filename = r.front().key();
> +        ScriptSourceInfo &info = r.front().value();
> +
> +        // If this script-source is too small, or if we can't grow the
> +        // notableScriptSources vector, skip this string.

Shouldn't we just bail out entirely in the latter case? It's probably not important either way, but seems to make more sense to me. Or is there a way we could actually recover before the next attempt?

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2291,5 @@
> +        // path, because we don't want any forward slashes in the string to
> +        // count as path separators.
> +        nsDependentCString notableString(info.filename_);
> +        nsCString escapedString(notableString);
> +        escapedString.ReplaceSubstring("/", "\\");

Hmm, this isn't too great. In constrast to about:memory's usage of "/", the "/"'s here actually *are* file path separators, after all. Then again, in your example output, the file paths/URLs are displayed correctly, so that seems fine. How does that work, though? Maybe briefly explain that in the comment?

Comment 3

[Nicholas Nethercote [inactive]]

> During my review, I kept
> looking for other flavors to show up. I realize that this is in contrast to
> the notable sources, the sizes of which aren't stored here. Maybe either
> rename to nonNotableSources or also rename the scriptSources and
> notableScriptSources below to scriptSourcesNormal and scriptSourcesNotable?

I changed "normal" to "non-notable" and also renamed |scriptSources| as |allScriptSources|.

> > +        // If this script-source is too small, or if we can't grow the
> > +        // notableScriptSources vector, skip this string.
> 
> Shouldn't we just bail out entirely in the latter case? It's probably not
> important either way, but seems to make more sense to me. Or is there a way
> we could actually recover before the next attempt?

This code is already inconsistent about handling OOMs; in some places it just aborts, in others it handles it less abruptly. I am... continuing that tradition :)

> Hmm, this isn't too great. In contrast to about:memory's usage of "/", the
> "/"'s here actually *are* file path separators, after all. Then again, in
> your example output, the file paths/URLs are displayed correctly, so that
> seems fine. How does that work, though? Maybe briefly explain that in the
> comment?

The "path separators" mentioned in the comment refer to memory reporter paths, which dictate the shape of the tree in about:memory; they're different to file system paths. about:memory converts all backslashes to forward slashes; I added a brief comment explaining that. It's a gross old hack that's hard to avoid :(

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Report script sources in more detail.

Updated with comments addressed. Carrying over r+.

Comment 5

[Nicholas Nethercote [inactive]]

I've extended the scope of this bug significantly: I will be changing how strings, script-sources, objects and shapes are reported. Patches to come shortly.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Don't report short strings separately.

This patch removes the separate reporting of short strings. It wasn't that
useful, and it misleadingly reported short strings (i.e. double-width inline GC
string things) but not inline strings (i.e. single-width inline GC string
things). The distinction between notable and non-notable strings is much more
useful and interesting.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Shorten JS memory report descriptions.

This patch:
- Shortens the descriptions used by the JS memory reporter. Mostly the amount
  of information is unchanged -- e.g. I removed lots of redundant "Memory used
  by ..." prefixes, and just made the language more concise. (The verbosity has
  been bugging me for a while.)

- Removes some no-longer-necessary REPORT macros.

- Changes the indentation used on the REPORT macro calls to always be 4-deep on
  subsequent lines, instead of indenting to the first paren, which results in
  fewer line breaks within the strings.

Note that the "slow array objects" size is always zero because they haven't 
existed for a while. That size will be removed in a subsequent patch.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Rework notable string reporting.

This patch does several things involving notable string reporting. (Apologies.)

- Currently notable strings are identified in each zone, and then re-identified
  in the "js-main-runtime" tree in "Other measurements". The re-identification
  adds complexity, isn't useful, bloats the output, and slightly increases the
  size of memory spike caused by the notable string detection. So this patch
  removes the re-identification.

- Inline ZoneStatsPod into ZoneStats, because the separation isn't useful.

- Macroizes StringInfo and uses it more extensively.

- Replaces NotableStringInfo::notableSize() with StringInfo::isNotable(), and
  increases the threshold from 8 KiB to 16 KiB.

- Renames ZoneStats::strings as ZoneStats::allStrings.

- Tweaks OOM handling in a few places.

- Tweaks the output slightly, from this:

> ├──196,448 B (00.18%) -- non-notable
> │  ├───99,840 B (00.09%) ── malloc-heap
> │  └───96,608 B (00.09%) ── gc-heap
> └───33,952 B (00.03%) -- notable
>     ├──16,416 B (00.02%) -- string(length=6285, copies=1, "..." (truncated))
>     │  ├──16,384 B (00.02%) ── malloc-heap
>     │  └──────32 B (00.00%) ── gc-heap

  to this:

> ├──617,928 B (00.67%) -- string(<non-notable strings>)
> │  ├──328,168 B (00.36%) ── malloc-heap
> │  └──289,760 B (00.32%) ── gc-heap
> ├───98,336 B (00.11%) -- string(length=49054, copies=1, "..." (truncated))
> │   ├──98,304 B (00.11%) ── malloc-heap
> │   └──────32 B (00.00%) ── gc-heap

  which reduces the path depth for notable strings by one.

- Reduces the max shown portion of notable strings from 4 KiB to 1 KiB, because
  4 KiB is just excessive and sometimes actively annoying.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Report script sources in more detail.

This is a tweaked version of the already-r+'d patch. It has changed enough that
I'm asking for a new review.

New sample output:

> ├────677,744 B (00.75%) -- script-sources
> │    ├──193,616 B (00.22%) -- source(scripts=1409, <non-notable files>)
> │    │  ├──102,320 B (00.11%) ── compressed
> │    │  ├───65,136 B (00.07%) ── misc
> │    │  └───26,160 B (00.03%) ── uncompressed
> │    ├──135,760 B (00.15%) -- source(scripts=216, /browser/content/tabbrowser.xml)
> │    │  ├───84,224 B (00.09%) ── compressed
> │    │  ├───31,104 B (00.03%) ── misc
> │    │  └───20,432 B (00.02%) ── uncompressed

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Report script sources in more detail.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Add BaseShape::clasp().

This is needed for the next patch.

Comment 12

[Till Schneidereit [:till]]

Comment on attachment 8379427 [details] [diff] [review]
(part 1) - Don't report short strings separately.

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

nice

Comment 13

[Till Schneidereit [:till]]

Comment on attachment 8379432 [details] [diff] [review]
(part 2) - Shorten JS memory report descriptions.

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

*So* much nicer!

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1820,5 @@
>  
>          if (info.mallocHeap > 0) {
> +            REPORT_BYTES(path + NS_LITERAL_CSTRING("malloc-heap"),
> +                KIND_HEAP, info.mallocHeap,
> +                "Non-inline string characters of a notable string. "

Nit: space at the end

@@ +1827,5 @@
>      }
>  
>      ZCREPORT_GC_BYTES(pathPrefix + NS_LITERAL_CSTRING("strings/non-notable/gc-heap"),
> +        zStats.stringsGCHeap,
> +        "Non-notable strings. " MAYBE_INLINE);

Are the entries for notable and non-notable strings guaranteed to always be shown together (and in that order)? If not, it might be good to explain what "non-notable" means. Maybe something like "i.e. ones that don't have many exact copies"?
Also, nit: space at the end

@@ +1832,4 @@
>  
>      ZCREPORT_BYTES(pathPrefix + NS_LITERAL_CSTRING("strings/non-notable/malloc-heap"),
> +        zStats.stringsMallocHeap,
> +        "Non-inline string characters of non-notable strings. "

Nit: space at the end

@@ +1838,5 @@
>      if (stringsNotableAboutMemoryGCHeap > 0) {
>          ZCREPORT_GC_BYTES(pathPrefix + NS_LITERAL_CSTRING("strings/notable/about-memory/gc-heap"),
> +            stringsNotableAboutMemoryGCHeap,
> +            "Notable strings that contain the characters '" STRING_LENGTH "', "
> +            "which are probably from about:memory itself." MAYBE_INLINE);

I think it might be good to still include the info that these (and the following ones) are filtered out. I'm almost entirely convinced that, otherwise, people will stumble upon this and think that we have an issue here that we don't really have.

@@ +2050,5 @@
>      }
>      if (nonHeapElementsAsmJS > 0) {
>          REPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("objects/non-heap/elements/asm.js"),
> +            KIND_NONHEAP, nonHeapElementsAsmJS,
> +            "asm.js array buffer elements outside the malloc heap.");

"outside both the malloc and the GC heap"?

@@ +2261,4 @@
>  
>      REPORT_GC_BYTES(rtPath + NS_LITERAL_CSTRING("gc-heap/chunk-admin"),
> +        rtStats.gcHeapChunkAdmin,
> +        "Book-keeping information within GC chunks.");

According to my dictionary, "bookkeeping" is a proper word

Comment 14

[Till Schneidereit [:till]]

Comment on attachment 8379432 [details] [diff] [review]
(part 2) - Shorten JS memory report descriptions.

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1820,5 @@
>  
>          if (info.mallocHeap > 0) {
> +            REPORT_BYTES(path + NS_LITERAL_CSTRING("malloc-heap"),
> +                KIND_HEAP, info.mallocHeap,
> +                "Non-inline string characters of a notable string. "

And of course, this and all other nits of its ilk should be ignored ...

Comment 15

[Till Schneidereit [:till]]

Comment on attachment 8379452 [details] [diff] [review]
(part 3) - Rework notable string reporting.

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

I like it

::: js/public/MemoryMetrics.h
@@ +253,1 @@
>  // NotableStringInfo holds a copy of the string's chars.

It doesn't hold a copy of the entire string, though - just the first 1024 bytes

::: js/src/vm/MemoryMetrics.cpp
@@ +103,5 @@
>  NotableStringInfo::NotableStringInfo(JSString *str, const StringInfo &info)
>    : StringInfo(info),
>      length(str->length())
>  {
> +    size_t bufferSize = Min(str->length() + 1, size_t(1024));

Both this and the notability limit of 16kb would ideally be constants in MemoryMetrics.h. They're not used in terribly many places, but they're magic constants, nevertheless

@@ +459,5 @@
>  
>      ZoneStatsVector &zs = rtStats->zoneStatsVector;
>      ZoneStats &zTotals = rtStats->zTotals;
>  
> +    // We don't find notable strings for zTotals. So we first sum all the

s/find/look for/, perhaps?

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1793,5 @@
>          // even more notable strings the next time we open about:memory (unless
>          // 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.

Nit: "//" before "bucket"

@@ +1830,5 @@
>  
> +    nsCString nonNotablePath = pathPrefix;
> +    nonNotablePath += zStats.isTotals
> +                    ? NS_LITERAL_CSTRING("strings/")
> +                    : NS_LITERAL_CSTRING("strings/string(<non-notable strings>)/");

Maybe "all non-notable strings"?

Comment 16

[Till Schneidereit [:till]]

Comment on attachment 8379459 [details] [diff] [review]
(part 5) - Add BaseShape::clasp().

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

Sure. Where's this magical next patch you're talking about, though?

Comment 17

[Till Schneidereit [:till]]

Comment on attachment 8379458 [details] [diff] [review]
(part 4) - Report script sources in more detail.

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

There's some inconsistency in the spelling of "script source" in comments. Maybe use "script source" when talking about the concept and ScriptSource when talking about actual instances of that class? I commented on this somewhat arbitrarily in-line, and apologize for that.

As an aside, I can't remember the last time I had a meaningful comment about things that weren't strictly for human consumption (as in, comments and printed strings) when reviewing your patches. I'm going to go ahead and say this is a testament to your patches' quality and not my mediocrity as a reviewer. ;)

::: js/public/MemoryMetrics.h
@@ +298,5 @@
> +
> +    bool isNotable() {
> +        size_t n = 0;
> +        FOR_EACH_SIZE(ADD_SIZE_TO_N)
> +        return n >= 16384;

This could use the same constant I'd like to see introduced in part 3

@@ +309,5 @@
> +#undef FOR_EACH_SIZE
> +};
> +
> +// Holds data about a notable script source file (one whose combined
> +// script-sources use more than a certain amount of memory) so we can report it

Either ScriptSources here, or script-sources 6 lines above. Or script sources for both, perhaps? I'm not entirely sure which is best

@@ +367,5 @@
> +        // prevent that, so it doesn't hurt to try again here.
> +        js_delete(allScriptSources);
> +    }
> +
> +    // The script-source measurements in |scriptSourceInfo| are initially for

script source or ScriptSource, potentially

@@ +368,5 @@
> +        js_delete(allScriptSources);
> +    }
> +
> +    // The script-source measurements in |scriptSourceInfo| are initially for
> +    // all script sources.  At the end, if the measurement granularity is

script-sources or ScriptSources, potentially

@@ +381,5 @@
> +                        js::CStringHashPolicy,
> +                        js::SystemAllocPolicy> ScriptSourcesHashMap;
> +
> +    // |allScriptSources| is only used transiently.  During the reporting phase
> +    // it is filled with info about every script-source in the runtime.  It's

script source or ScriptSource, potentially

::: js/src/vm/MemoryMetrics.cpp
@@ +406,5 @@
> +
> +                JS::RuntimeSizes::ScriptSourcesHashMap::AddPtr p =
> +                    rtStats->runtime.allScriptSources->lookupForAdd(filename);
> +                if (!p) {
> +                    // Ignore failure -- we just won't record the script-source as notable.

script source or ScriptSource, potentially

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2225,5 @@
> +    for (size_t i = 0; i < rtStats.runtime.notableScriptSources.length(); i++) {
> +        const JS::NotableScriptSourceInfo& scriptSourceInfo =
> +            rtStats.runtime.notableScriptSources[i];
> +
> +        // Escape / to \ before we put notableString into the memory reporter

s/notableString/the filename/

Comment 18

[Till Schneidereit [:till]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Created attachment 8379455 [details] [diff] [review]
> (part 1) - Report script sources in more detail.
> 
> This is a tweaked version of the already-r+'d patch. It has changed enough
> that
> I'm asking for a new review.

I'm not sure what happened to this. Did you decide to go with the original version after all, or did you accidentally mark this as obsolete instead of the old version?

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Report objects and shapes by their class.

(Whoops, forgot to upload this one.)

This patch changes the reporting of objects and shapes to be done w.r.t. their
classes. Example output:

> ├──139,688 B (00.18%) -- class(Object)
> │  ├──129,952 B (00.17%) -- objects
> │  │  ├──127,136 B (00.16%) ── gc-heap
> │  │  └────2,816 B (00.00%) ── malloc-heap/slots
> │  └────9,736 B (00.01%) -- shapes
> │       ├──8,328 B (00.01%) -- gc-heap
> │       │  ├──5,800 B (00.01%) ── tree
> │       │  ├──1,880 B (00.00%) ── dict
> │       │  └────648 B (00.00%) ── base
> │       └──1,408 B (00.00%) -- malloc-heap
> │          ├──1,120 B (00.00%) ── tree-kids
> │          └────288 B (00.00%) ── dict-tables
> ├───41,528 B (00.05%) -- class(<non-notable classes>)
> │   ├──21,536 B (00.03%) -- objects
> │   │  ├──17,040 B (00.02%) ── gc-heap
> │   │  └───4,496 B (00.01%) -- malloc-heap
> │   │      ├──4,224 B (00.01%) ── slots
> │   │      └────272 B (00.00%) ── misc
> │   └──19,992 B (00.03%) -- shapes
> │      ├──17,720 B (00.02%) -- gc-heap
> │      │  ├──10,160 B (00.01%) ── tree
> │      │  ├───5,040 B (00.01%) ── base
> │      │  └───2,520 B (00.00%) ── dict
> │      └───2,272 B (00.00%) -- malloc-heap
> │          ├──1,216 B (00.00%) ── dict-tables
> │          ├────896 B (00.00%) ── tree-kids
> │          └────160 B (00.00%) ── tree-tables

Something I haven't done, but which is possible, is to add object and shape
counts to the |class(...)| lines, e.g.:

  class(objects=1000, shapes=2000, Object)

Comment 20

[Nicholas Nethercote [inactive]]

> > (part 1) - Report script sources in more detail.
> > 
> > This is a tweaked version of the already-r+'d patch. It has changed enough
> > that
> > I'm asking for a new review.
> 
> I'm not sure what happened to this. Did you decide to go with the original
> version after all, or did you accidentally mark this as obsolete instead of
> the old version?

I uploaded it again with the correct "part 4" in the commit message.

So you just need to review part 6 (which I only just uploaded, whoops) and then you're done.

Comment 21

[Till Schneidereit [:till]]

Comment on attachment 8381129 [details] [diff] [review]
(part 6) - Report objects and shapes by their class.

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

\o/, this is truly great.

(And yes, a follow-up to include counts would make it greater, still.)

::: js/public/MemoryMetrics.h
@@ +148,5 @@
> +
> +    bool isNotable() {
> +        size_t n = 0;
> +        FOR_EACH_SIZE(ADD_SIZE_TO_N)
> +        return n >= 16384;

Ok, now I'd really like this to be a constant

@@ +168,5 @@
>  #undef FOR_EACH_SIZE
>  };
>  
> +// Holds data about a notable class (one whose combined object and shape data
> +// uses more than a certain amount of memory) so we can report it individually.

This should somehow say that we're concerned with the memory used by the class' instances.

::: js/src/vm/MemoryMetrics.cpp
@@ +681,5 @@
> +    // notable classes within each zone.
> +    for (size_t i = 0; i < cs.length(); i++)
> +        cTotals.addSizes(cs[i]);
> +
> +    for (size_t i = 0; i < cs.length(); i++)

Multi-line body, so needs braces

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1927,5 @@
> +    }
> +    if (nonHeapElementsAsmJS > 0) {
> +        REPORT_BYTES(path + NS_LITERAL_CSTRING("objects/non-heap/elements/asm.js"),
> +            KIND_NONHEAP, nonHeapElementsAsmJS,
> +            "asm.js array buffer elements outside the malloc heap.");

"outside the malloc and GC heaps"?

Comment 22

[Nicholas Nethercote [inactive]]

> >          if (info.mallocHeap > 0) {
> > +            REPORT_BYTES(path + NS_LITERAL_CSTRING("malloc-heap"),
> > +                KIND_HEAP, info.mallocHeap,
> > +                "Non-inline string characters of a notable string. "
> 
> Nit: space at the end

All these descriptions have MAYBE_INLINE or MAYBE_OVERALLOCATED appended to the end, so the space is necessary.

> Are the entries for notable and non-notable strings guaranteed to always be
> shown together (and in that order)? If not, it might be good to explain what
> "non-notable" means. Maybe something like "i.e. ones that don't have many
> exact copies"?

They're ordered by size, so the position of the non-notable ones could be arbitrary w.r.t. the notable ones. I've avoided distinguishing notable and non-notable in the descriptions because in the later patches the same code is used for both notable and non-notable reports, and altering the descriptions is a pain.

> According to my dictionary, "bookkeeping" is a proper word

It's also a word with three double letters in a row. And "sub-bookkeeper" has four, if you consider it to be a real word.

Comment 23

[Till Schneidereit [:till]]

(In reply to Nicholas Nethercote [:njn] from comment #22)
> All these descriptions have MAYBE_INLINE or MAYBE_OVERALLOCATED appended to
> the end, so the space is necessary.

Yeah, I realized that later on. Sorry for the noise.
> 
> > Are the entries for notable and non-notable strings guaranteed to always be
> > shown together (and in that order)? If not, it might be good to explain what
> > "non-notable" means. Maybe something like "i.e. ones that don't have many
> > exact copies"?
> 
> They're ordered by size, so the position of the non-notable ones could be
> arbitrary w.r.t. the notable ones. I've avoided distinguishing notable and
> non-notable in the descriptions because in the later patches the same code
> is used for both notable and non-notable reports, and altering the
> descriptions is a pain.

What I meant is that the entry for gc-heap bytes of notable strings explains what they are, but all other entries for both notable and non-notable strings don't. I guess it doesn't matter much, however: lots of entries require domain knowledge to fully understand, and that's ok.

> 
> > According to my dictionary, "bookkeeping" is a proper word
> 
> It's also a word with three double letters in a row. And "sub-bookkeeper"
> has four, if you consider it to be a real word.

Nice!

Comment 24

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b282d197d23f
https://hg.mozilla.org/integration/mozilla-inbound/rev/83b4c208e0c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/609b72cffe1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/655050f4a746
https://hg.mozilla.org/integration/mozilla-inbound/rev/753376a55f0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/519787a56627

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b282d197d23f
https://hg.mozilla.org/mozilla-central/rev/83b4c208e0c3
https://hg.mozilla.org/mozilla-central/rev/609b72cffe1f
https://hg.mozilla.org/mozilla-central/rev/655050f4a746
https://hg.mozilla.org/mozilla-central/rev/753376a55f0f
https://hg.mozilla.org/mozilla-central/rev/519787a56627

Comment 26

[Nicholas Nethercote [inactive]]

I backed out part 6 (and the patch from bug 978227) for causing intermittent ASAN failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed1abc1b556

Comment 27

[Nicholas Nethercote [inactive]]

And a follow-up to unbreak non-unified builds...
https://hg.mozilla.org/integration/mozilla-inbound/rev/b16e5c8194cb

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b16e5c8194cb

Comment 29

[Nicholas Nethercote [inactive]]

[Reopening because part 6 bounced.]

I did a try run, and reproduced the error: https://tbpl.mozilla.org/?tree=Try&rev=72df28adaf01, specifically https://tbpl.mozilla.org/php/getParsedLog.php?id=35771879&tree=Try&full=1.

Here's the ASAN report, slightly trimmed.

> ==2452==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6140004c6648 at pc 0x7fdd914834f4 bp 0x7fff140f6690 sp 0x7fff140f6688
> READ of size 8 at 0x6140004c6648 thread T0
>     #0 0x7fdd914834f3 in _ZL17StatsCellCallbackIL11Granularity0EEvP9JSRuntimePvS3_13JSGCTraceKindm js/src/vm/MemoryMetrics.cpp:432
>     #1 0x7fdd90af0929 in IterateCompartmentsArenasCells(JSRuntime*, JS::Zone*, void*, void (*)(JSRuntime*, void*, JSCompartment*), void (*)(JSRuntime*, void*, js::gc::Arena*, JSGCTraceKind, unsigned long), void (*)(JSRuntime*, void*, void*, JSGCTraceKind, unsigned long)) js/src/gc/Iteration.cpp:47
>     #2 0x7fdd90af018a in js::IterateZonesCompartmentsArenasCells(JSRuntime*, void*, void (*)(JSRuntime*, void*, JS::Zone*), void (*)(JSRuntime*, void*, JSCompartment*), void (*)(JSRuntime*, void*, js::gc::Arena*, JSGCTraceKind, unsigned long), void (*)(JSRuntime*, void*, void*, JSGCTraceKind, unsigned long)) js/src/gc/Iteration.cpp:63
>     #3 0x7fdd9147e0b9 in JS::CollectRuntimeStats(JSRuntime*, JS::RuntimeStats*, JS::ObjectPrivateVisitor*) js/src/vm/MemoryMetrics.cpp:659
>     ...
> 0x6140004c6648 is located 56 bytes inside of 5006896-byte region [0x6140004c6610,0x61400098cc40)
> freed by thread T0 here:
> ==2452==AddressSanitizer CHECK failed: compiler-rt/lib/asan/asan_allocator2.cc:228 "((id)) != (0)" (0x0, 0x0)
>     #0 0x44bcf4 in __asan::AsanCheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) compiler-rt/lib/asan/asan_rtl.cc:63
>     #1 0x450f21 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) compiler-rt/lib/sanitizer_common/sanitizer_common.cc:60
>     #2 0x42337c in GetStackTraceFromId compiler-rt/lib/asan/asan_allocator2.cc:228
>     #3 0x42337c in __asan::AsanChunkView::GetFreeStack(__sanitizer::StackTrace*) compiler-rt/lib/asan/asan_allocator2.cc:246
>     #4 0x448fca in __asan::DescribeHeapAddress(unsigned long, unsigned long) compiler-rt/lib/asan/asan_report.cc:383
>     #5 0x449ed4 in __asan_report_error compiler-rt/lib/asan/asan_report.cc:740
>     #6 0x44afd6 in __asan_report_load8 compiler-rt/lib/asan/asan_rtl.cc:253
>     #7 0x7fdd914834f3 in _ZL17StatsCellCallbackIL11Granularity0EEvP9JSRuntimePvS3_13JSGCTraceKindm js/src/vm/MemoryMetrics.cpp:432
>     #8 0x7fdd90af0929 in IterateCompartmentsArenasCells(JSRuntime*, JS::Zone*, void*, void (*)(JSRuntime*, void*, JSCompartment*), void (*)(JSRuntime*, void*, js
::gc::Arena*, JSGCTraceKind, unsigned long), void (*)(JSRuntime*, void*, void*, JSGCTraceKind, unsigned long)) js/src/gc/Iteration.cpp:47
>     #9 0x7fdd90af018a in js::IterateZonesCompartmentsArenasCells(JSRuntime*, void*, void (*)(JSRuntime*, void*, JS::Zone*), void (*)(JSRuntime*,void*, JSCompartment*), void (*)(JSRuntime*, void*, js::gc::Arena*, JSGCTraceKind, unsigned long), void (*)(JSRuntime*, void*, void*, JSGCTraceKind, unsigned long)) js/src/gc/Iter
ation.cpp:63
>     #10 0x7fdd9147e0b9 in JS::CollectRuntimeStats(JSRuntime*, JS::RuntimeStats*, JS::ObjectPrivateVisitor*) js/src/vm/MemoryMetrics.cpp:659
>     ...

This seems totally bogus, for two reasons.

- It claims that the code is reading in a previously-freed block of size
  5,006,896, which can't be right -- the JS engine wouldn't allocate a block
  of that size.

- It says the previous free occurred on the same line as the bad read
  (MemoryMetrics.cpp:432) which is nonsense -- there are no frees anywhere near
  that line.

And this looks rather suspicious:

> ==2452==AddressSanitizer CHECK failed: compiler-rt/lib/asan/asan_allocator2.cc:228 "((id)) != (0)" (0x0, 0x0)

It looks like it might be an ASAN bug. decoder, what do you think?

Comment 30

[Christian Holler (:decoder)]

(In reply to Nicholas Nethercote [:njn] from comment #29)

> - It claims that the code is reading in a previously-freed block of size
>   5,006,896, which can't be right -- the JS engine wouldn't allocate a block
>   of that size.

Why is that? Do you mean the internal allocations or external ones?

> 
> - It says the previous free occurred on the same line as the bad read
>   (MemoryMetrics.cpp:432) which is nonsense -- there are no frees anywhere
> near
>   that line.

I don't see that being reported in the trace. It rather fails creating the stack for the free'd location (and that's why the CHECK message is there)

So, my guess is: Yes this is an ASan bug, at least in the reporting mechanism. But I would guess it's not a bogus bug reported, but rather the memory is indeed not valid. Does the code in question intentionally scan over memory (like the GC scanner does)?

I guess the first step here would be to locally reproduce this, then attach GDB and see what memory it's touching there.

Comment 31

[Nicholas Nethercote [inactive]]

(In reply to Christian Holler (:decoder) from comment #30)
> (In reply to Nicholas Nethercote [:njn] from comment #29)
> 
> > - It claims that the code is reading in a previously-freed block of size
> >   5,006,896, which can't be right -- the JS engine wouldn't allocate a block
> >   of that size.
> 
> Why is that? Do you mean the internal allocations or external ones?

Here's the relevant code:

        const Class *clasp = base->clasp();
        const char *className = clasp->name;     // line 432

ASAN is saying that |clasp| points to a 5MB block, and the |name| field is 56 bytes inside it. This doesn't make any sense because Class is a struct that's a few hundred bytes at most, we don't create any 5MB arrays of Class objects, and |name| is actually the first field within Class.


> > - It says the previous free occurred on the same line as the bad read
> >   (MemoryMetrics.cpp:432) which is nonsense -- there are no frees anywhere
> > near
> >   that line.
> 
> I don't see that being reported in the trace. It rather fails creating the
> stack for the free'd location (and that's why the CHECK message is there)

Oh, that stack trace is for the failed check, not the allocation. Makes sense.

> So, my guess is: Yes this is an ASan bug, at least in the reporting
> mechanism. But I would guess it's not a bogus bug reported, but rather the
> memory is indeed not valid. Does the code in question intentionally scan
> over memory (like the GC scanner does)?

The reporter scans the live cells on the GC heap -- |base| is a GC cell -- but it shouldn't be doing anything unusual that would confuse ASAN.

Comment 32

[Christian Holler (:decoder)]

(In reply to Nicholas Nethercote [:njn] from comment #31)
> Here's the relevant code:
> 
>         const Class *clasp = base->clasp();
>         const char *className = clasp->name;     // line 432
> 
> ASAN is saying that |clasp| points to a 5MB block, and the |name| field is
> 56 bytes inside it. This doesn't make any sense because Class is a struct
> that's a few hundred bytes at most, we don't create any 5MB arrays of Class
> objects, and |name| is actually the first field within Class.

I still don't understand why that means the ASan trace is bogus. The base->clasp() (or even the base) pointer could be invalid, overwritten or otherwise corrupted. It could point to any bogus location, which could also be a 5MB block.

Comment 33

[Nicholas Nethercote [inactive]]

I just did another try run with part 6 and got several ASAN failures:
https://tbpl.mozilla.org/?tree=Try&rev=7a2806766fb3

But the stack are entirely lacking symbols, and ASAN isn't providing any useful info. decoder, any idea what's going on?

Comment 34

[Christian Holler (:decoder)]

(In reply to Nicholas Nethercote [:njn] from comment #33)
>
> But the stack are entirely lacking symbols, and ASAN isn't providing any
> useful info. decoder, any idea what's going on?

This is bug 1020590. Symbols were broken for a while, but glandium landed a fix to inbound yesterday, so it should be working again now.

Comment 35

[Nicholas Nethercote [inactive]]

I did another ASAN run on part 6, and got a seg fault in lastProperty() without any useful info from ASAN two times out of ~35:

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

Then I did a run without part 6 (i.e. just the current m-c):

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

and also got the same lastProperty() crash one time out of ~35. I can't tell if this is a known crash; I couldn't find anything about it in Bugzilla.

Maybe I should just try landing the patch again.

Comment 36

[Nicholas Nethercote [inactive]]

Five of the six patches in this bug have landed, and bug 1023719 is now open for the remaining part. This bug can be closed.