1475899 - Add memory reporter for committed size of thread stacks

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Kris Maglione [:kmag]]

We don't have good diagnostics for actual memory usage of thread stacks. The best we have is a guess at the total memory usage of all stacks combined.

We can do better by keeping track of the stack locations of the threads we actually create, and mapping them to actual VM usage data. That would tell us which threads are actually causing memory issues, and help us decide what to do about them.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1475899: Track live nsThreads and add a method of enumerating them.

This will allow us to enumerate active threads in order to report their
memory.

Review commit: https://reviewboard.mozilla.org/r/257108/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257108/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1475899: Part 2 - Store stack base pointer for nsThreads on Linux.

This change stores the base allocation address and actual allocated stack size
(as opposed to the requested size, which may be 0), so that we can map them to
VM regions in a memory reporter.

Review commit: https://reviewboard.mozilla.org/r/257110/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257110/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1475899: Part 3 - Add helper for parsing memory mappings on Linux.

The only real way to get memory mapping information on Linux is to read and
parse /proc/self/maps, and infer information about a specific pointer based on
its contents.

This patch adds a helper which parses the entire contents of smaps, and
returns an array of objects describing each region. It also updates resident
unique reporter to use that helper. Later patches use it to map stack base
addresses to VM regions, and report their usage data.

Review commit: https://reviewboard.mozilla.org/r/257112/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257112/

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1475899: Part 4 - Add memory reporter for committed thread stack sizes on Linux.

Later patches add support for Windows. OS-X will need a follow-up.

Review commit: https://reviewboard.mozilla.org/r/257114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257114/

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1475899: Part 5 - Add thread stack memory reporter for Windows.

Review commit: https://reviewboard.mozilla.org/r/257116/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257116/

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8992234 [details]
Bug 1475899: Part 4 - Add memory reporter for committed thread stack sizes on Linux.

https://reviewboard.mozilla.org/r/257114/#review263902


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: xpcom/base/nsMemoryReporterManager.cpp:1410
(Diff revision 1)
> +#ifdef XP_LINUX
> +class ThreadStacksReporter final : public nsIMemoryReporter
> +{
> +  MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)
> +
> +  ~ThreadStacksReporter() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~ThreadStacksReporter() {}
  ^                       ~~
                          = default;

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8992235 [details]
Bug 1475899: Part 5 - Add thread stack memory reporter for Windows.

https://reviewboard.mozilla.org/r/257116/#review263904


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: xpcom/base/nsMemoryReporterManager.cpp:1412
(Diff revision 1)
> -#ifdef XP_LINUX
> +#if defined(XP_LINUX) || defined(XP_WIN)
>  class ThreadStacksReporter final : public nsIMemoryReporter
>  {
>    MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)
>  
>    ~ThreadStacksReporter() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~ThreadStacksReporter() {}
  ^                       ~~
                          = default;

Comment 8

[Molly Howell (she/her) [:mhowell]]

Aaron, would you be able to take this review from me? I'm out on PTO for two weeks, and I don't want to make Kris wait that long.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8992235 [details]
Bug 1475899: Part 5 - Add thread stack memory reporter for Windows.

https://reviewboard.mozilla.org/r/257116/#review264102

::: xpcom/base/MemoryInfo.h:14
(Diff revision 1)
> + * MemoryInfo is a helper class which describes the attributes and sizes of a
> + * particular region of VM memory on Windows. It roughtly corresponds to the
> + * values in a MEMORY_BASIC_INFORMATION struct, summed over an entire region or
> + * memory.

Note: I know that having a separate module for this seems like overkill, but Boris mentioned wanting to use the commit size of thread stacks to decide which threads in thread pools to kill. So I tried to implement it in a way that was reusable for that and similar purposes.

I also tried to do it in a way that didn't require callers to include "windows.h", since there are places (e.g., most DOM code) where we can't do that because of the macros it defines. So permission/page type flags get their own enums here rather than relying on the WinAPI constants.

Comment 10

[Eric Rahm [:erahm]]

Comment on attachment 8992231 [details]
Bug 1475899: Track live nsThreads and add a method of enumerating them.

https://reviewboard.mozilla.org/r/257108/#review264116

Looks good, minor suggestions.

::: xpcom/threads/nsThread.h:200
(Diff revision 1)
>    mozilla::TimeStamp mCurrentEventStart;
>    uint32_t mCurrentEventLoopDepth;
>    RefPtr<mozilla::PerformanceCounter> mCurrentPerformanceCounter;
>  };
>  
> +class MOZ_STACK_CLASS nsThreadEnumerator final

A small test for this and making sure threads are add/removed as expected would be nice. It's possible you do that later on.

::: xpcom/threads/nsThread.cpp:382
(Diff revision 1)
>  }
>  
> +/* static */ mozilla::Mutex&
> +nsThread::ThreadListMutex()
> +{
> +  static Mutex sMutex("nsThread::ThreadListMutex");

This might need to be an `OffTheBooksMutex`, it depends on what point threads are created during XPCOM init.

I _think_ we prefer `StaticMutex` for things like this, but using function scope to initialize is fine too.

::: xpcom/threads/nsThread.cpp:746
(Diff revision 1)
>      return nullptr;
>    }
>  
> +  {
> +    MutexAutoLock mal(ThreadListMutex());
> +    if (isInList()) {

I guess we should assert this as well.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8992231 [details]
Bug 1475899: Track live nsThreads and add a method of enumerating them.

https://reviewboard.mozilla.org/r/257108/#review264116

> This might need to be an `OffTheBooksMutex`, it depends on what point threads are created during XPCOM init.
> 
> I _think_ we prefer `StaticMutex` for things like this, but using function scope to initialize is fine too.

Oh, I completely forgot StaticMutex was a thing. It has the unfortunate side-effect of winding up with the super generic name "StaticMutex", though...

> I guess we should assert this as well.

There are some nsThreads that never wind up in the list. The main thread, in particular, but also any other threads we create externally but then make nsThread wrappers for when someone calls GetCurrentThread().

I'm intentionally ignoring the main thread for now. It might be good to support the others, but I'd ratehr do that in a follow-up, if at all. Need to consider the implications.

Comment 12

[Eric Rahm [:erahm]]

Comment on attachment 8992232 [details]
Bug 1475899: Part 2 - Store stack base pointer for nsThreads on Linux.

https://reviewboard.mozilla.org/r/257110/#review264122

Minor sugestions / comments. I'd like a linux internals expert to weigh in here as well.

::: xpcom/threads/nsThread.cpp:433
(Diff revision 1)
> +    // Glibc prior to 2.27 reports the stack size and base including the guard
> +    // region, so we need to compensate for it to get accurate accounting.
> +    // Also, this behavior difference isn't guarded by a versioned symbol, so we
> +    // actually need to check the runtime glibc version, not the version we were
> +    // compiled against.
> +    static bool sAddGuardSize = ({

nit--: Maybe call this `sAdjustForGuardSize`

::: xpcom/threads/nsThread.cpp:442
(Diff revision 1)
> +    });
> +    if (sAddGuardSize) {
> +      size_t guardSize;
> +      pthread_attr_getguardsize(&attr, &guardSize);
> +
> +      self->mStackBase = reinterpret_cast<char*>(self->mStackBase) + guardSize;

Does the direction of stack growth matter here? Can you add a comment either way?

::: xpcom/threads/nsThread.cpp:461
(Diff revision 1)
> +    // What this does get us, however, is a different set of VM flags on our
> +    // thread stacks compared to normal heap memory. Which makes the Linux
> +    // kernel report them as separate regions, even when they are adjacent to
> +    // heap memory. This allows us to accurately track the actual memory
> +    // consumption of our allocated stacks.
> +    madvise(self->mStackBase, stackSize, MADV_NOHUGEPAGE);

Can you please add glandium or jld to this review? They're probably better suited to know if all of this is correct.

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8992232 [details]
Bug 1475899: Part 2 - Store stack base pointer for nsThreads on Linux.

https://reviewboard.mozilla.org/r/257110/#review264122

> Does the direction of stack growth matter here? Can you add a comment either way?

It does matter. For stacks that grow up, we get a guard at the top of the stack, rather than the bottom, so we don't need the base pointer adjustment (but do need the size adjustment).

But I don't think we support any Linux platforms with stacks that grow up, and the only side-effect of ignoring them is under-reporting one page, so I decided to ignore them. I can add a comment, though.

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧]

Comment on attachment 8992232 [details]
Bug 1475899: Part 2 - Store stack base pointer for nsThreads on Linux.

https://reviewboard.mozilla.org/r/257110/#review264186

::: xpcom/threads/nsThread.cpp:53
(Diff revision 1)
>  #include "ThreadEventTarget.h"
>  
>  #include "mozilla/dom/ContentChild.h"
>  
>  #ifdef XP_LINUX
> +#include <gnu/libc-version.h>

See below about glibc extensions.

::: xpcom/threads/nsThread.cpp:435
(Diff revision 1)
> +    // Also, this behavior difference isn't guarded by a versioned symbol, so we
> +    // actually need to check the runtime glibc version, not the version we were
> +    // compiled against.
> +    static bool sAddGuardSize = ({
> +      unsigned major, minor;
> +      sscanf(gnu_get_libc_version(), "%u.%u", &major, &minor) < 2 ||

Can you ifdef this for glibc?

Musl seems to not need this adjustment.  Bionic has the same kind of problem as glibc (https://android.googlesource.com/platform/bionic.git/+/d6c678ca90d6f7843a9186515fa38d9eec467ff6) but would need a different version check, but also I don't know if we care about this on Android yet.

::: xpcom/threads/nsThread.cpp:461
(Diff revision 1)
> +    // What this does get us, however, is a different set of VM flags on our
> +    // thread stacks compared to normal heap memory. Which makes the Linux
> +    // kernel report them as separate regions, even when they are adjacent to
> +    // heap memory. This allows us to accurately track the actual memory
> +    // consumption of our allocated stacks.
> +    madvise(self->mStackBase, stackSize, MADV_NOHUGEPAGE);

This hack seems reasonable, but it would be nice to verify that this doesn't somehow get merged with other no-huge-page memory.  Could we assert that we get the expected memory range when traversing `smaps`?

Comment 15

[Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧]

Comment on attachment 8992233 [details]
Bug 1475899: Part 3 - Add helper for parsing memory mappings on Linux.

https://reviewboard.mozilla.org/r/257112/#review264196

::: xpcom/base/MemoryMapping.cpp:134
(Diff revision 1)
> +    char flags[4] = "---";
> +    char name[512];
> +
> +    name[0] = 0;
> +
> +    if (sscanf(buffer, "%zx-%zx %4c %zx %*u:%*u %*u %511s",

FYI, there's some similar-looking code in the profiler: https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/tools/profiler/core/shared-libraries-linux.cc#195

Maybe they could be deduplicated at some point in the future.

Comment 16

[Eric Rahm [:erahm]]

Comment on attachment 8992233 [details]
Bug 1475899: Part 3 - Add helper for parsing memory mappings on Linux.

https://reviewboard.mozilla.org/r/257112/#review264128

This looks...okay :) Setting up an offset table definitely makes parsing easier, but also feels a little sketchy (in the what if we add a field sense). Can you tag jld in as well, I believe he wrote the original parser and might flag concerns about different formats b/c kernels etc.

::: xpcom/base/MemoryMapping.h:73
(Diff revision 1)
> +  };
> +
> +  using PermSet = EnumSet<Perm>;
> +
> +  MemoryMapping(uintptr_t aStart, uintptr_t aEnd,
> +                PermSet aPerms, size_t aOffset,

nit: const PermSet&

::: xpcom/base/MemoryMapping.h:171
(Diff revision 1)
> +  {
> +    const char* mName;
> +    size_t mOffset;
> +  };
> +
> +  static const Field sFields[20];

It seems like we can just move these parsing helpers to the cpp file.

::: xpcom/base/MemoryMapping.h:173
(Diff revision 1)
> +    size_t mOffset;
> +  };
> +
> +  static const Field sFields[20];
> +
> +  size_t& GetField(const Field& aField)

Nit: maybe call this `ValueForField` or something given the name of the `Field` struct.

::: xpcom/base/MemoryMapping.cpp:123
(Diff revision 1)
> +  // getline() will reallocate the buffer if it isn't large enough, and
> +  // replace the value in our pointer. In the end, we need to free whatever its
> +  // final value is. UniquePtr does not give us a way to handle this. Scoped
> +  // does.
> +  size_t bufferSize = 1024;
> +  ScopedCharArray buffer(new char[bufferSize]);

nit: use `moz_xmalloc` here.

::: xpcom/base/MemoryMapping.cpp:134
(Diff revision 1)
> +    char flags[4] = "---";
> +    char name[512];
> +
> +    name[0] = 0;
> +
> +    if (sscanf(buffer, "%zx-%zx %4c %zx %*u:%*u %*u %511s",

A comment here with a concrete line example would be helpful.

::: xpcom/base/MemoryMapping.cpp:165
(Diff revision 1)
> +    char* savePtr;
> +    char* fieldName = strtok_r(line, ":", &savePtr);
> +    if (!fieldName) {
> +      continue;
> +    }
> +    auto* field = FindEntry(fieldName, MemoryMapping::sFields);

It might be worth documenting whether or not there are fields we're ignoring.

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8992232 [details]
Bug 1475899: Part 2 - Store stack base pointer for nsThreads on Linux.

https://reviewboard.mozilla.org/r/257110/#review264186

> Can you ifdef this for glibc?
> 
> Musl seems to not need this adjustment.  Bionic has the same kind of problem as glibc (https://android.googlesource.com/platform/bionic.git/+/d6c678ca90d6f7843a9186515fa38d9eec467ff6) but would need a different version check, but also I don't know if we care about this on Android yet.

Do we support building on Linux with non-glibc-libcs? My impression was that we didn't, though I wasn't considering Android, so I guess we need the check either way.

> This hack seems reasonable, but it would be nice to verify that this doesn't somehow get merged with other no-huge-page memory.  Could we assert that we get the expected memory range when traversing `smaps`?

I added std::min() when checking the allocation sizes to deal with that possibility, but I guess an assert would make sense too.

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8992233 [details]
Bug 1475899: Part 3 - Add helper for parsing memory mappings on Linux.

https://reviewboard.mozilla.org/r/257112/#review264128

All of the field locations are calculated using offsetof, so adding a field shouldn't cause any problems.

> nit: const PermSet&

It's actually cheaper to just pass by value, since EnumSets are just `uint32_t`s.

> It seems like we can just move these parsing helpers to the cpp file.

I made them static members so I could use `offsetof` with private fields. Although I admittedly didn't check whether `offsetof` worked with private fields if I didn't make them static members.

Comment 19

[Eric Rahm [:erahm]]

Comment on attachment 8992233 [details]
Bug 1475899: Part 3 - Add helper for parsing memory mappings on Linux.

https://reviewboard.mozilla.org/r/257112/#review264128

Sure, I just mean they could end up out of sync. I suppose we could just add comment that the static table needs to be updated for new fields.

Comment 20

[Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧]

(In reply to Kris Maglione [:kmag] from comment #17)
> Do we support building on Linux with non-glibc-libcs? My impression was that
> we didn't, though I wasn't considering Android, so I guess we need the check
> either way.

There are people who build with musl, and sometimes they file bugs about it.  Since the fix in that case is just `false`, it seems easy enough to support.

Comment 21

[Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧]

I notice there's support on Windows and Linux, but not Mac.  It looks like vm_region_extended_info would have the information we'd need, but I haven't tried it.  Should I file a followup for that?

Comment 22

[Kris Maglione [:kmag]]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #21)
> I notice there's support on Windows and Linux, but not Mac.  It looks like
> vm_region_extended_info would have the information we'd need, but I haven't
> tried it.  Should I file a followup for that?

Yup, I was already planning to file a follow-up. I mentioned it in part 4.

Comment 23

[Aaron Klotz [:aklotz]]

Comment on attachment 8992235 [details]
Bug 1475899: Part 5 - Add thread stack memory reporter for Windows.

https://reviewboard.mozilla.org/r/257116/#review264210

::: xpcom/threads/nsThread.cpp:480
(Diff revision 1)
>      madvise(self->mStackBase, stackSize, MADV_NOHUGEPAGE);
>  
>      pthread_attr_destroy(&attr);
> -  }
> +#elif defined(XP_WIN)
> +    uintptr_t stackBottom, stackTop;
> +    GetCurrentThreadStackLimits(&stackBottom, &stackTop);

You're going to need to do something about the Windows 7 case here; at the very least you'll need to use dynamic linking to obtain a pointer to this function.

Comment 24

[Kris Maglione [:kmag]]

Comment on attachment 8992235 [details]
Bug 1475899: Part 5 - Add thread stack memory reporter for Windows.

https://reviewboard.mozilla.org/r/257116/#review264210

> You're going to need to do something about the Windows 7 case here; at the very least you'll need to use dynamic linking to obtain a pointer to this function.

Hm. I guess I got my Windows version numbers mixed up. I thought 6.2 was Windows 7. :(

Comment 25

[Eric Rahm [:erahm]]

Comment on attachment 8992234 [details]
Bug 1475899: Part 4 - Add memory reporter for committed thread stack sizes on Linux.

https://reviewboard.mozilla.org/r/257114/#review264220

Looks good, I'm just concerned about reporting while holding the thread list lock.

::: xpcom/base/nsMemoryReporterManager.cpp:1406
(Diff revision 1)
>    }
>  };
>  NS_IMPL_ISUPPORTS(AtomTablesReporter, nsIMemoryReporter)
>  
> +#ifdef XP_LINUX
> +class ThreadStacksReporter final : public nsIMemoryReporter

Can you file a follow up to a) convert things to use MemoryMappings if it makes sense and b) merge reporters so that we only parse smaps once.

::: xpcom/base/nsMemoryReporterManager.cpp:1421
(Diff revision 1)
> +                            nsISupports* aData, bool aAnonymize) override
> +  {
> +    nsTArray<MemoryMapping> mappings(1024);
> +    MOZ_TRY(GetMemoryMappings(mappings));
> +
> +    for (auto* thread : nsThread::Enumerate()) {

I'm a little concerned about locking the thread list for the duration of looping and hitting the callback (which probably goes through JS) while locked. Do you think it would make sense to build up a list of {name, size} entries and then report outside of the thread enumeration loop?

::: xpcom/base/nsMemoryReporterManager.cpp:1433
(Diff revision 1)
> +        continue;
> +      }
> +      size_t privateSize = mappings[idx].Referenced();
> +
> +      nsAutoCString path("explicit/thread-stacks/");
> +      path.AppendASCII(PR_GetThreadName(thread->GetPRThread()));

Should we append the `tid` as well to differentiate threads with the same name? It looks like our threadpool adds a unique identifier to each thread so this might not be an issue in practice.

If you do add it we'll want to update the about:memory diff tool to normalize out the `tid`.

Comment 26

[Kris Maglione [:kmag]]

Comment on attachment 8992234 [details]
Bug 1475899: Part 4 - Add memory reporter for committed thread stack sizes on Linux.

https://reviewboard.mozilla.org/r/257114/#review264220

> I'm a little concerned about locking the thread list for the duration of looping and hitting the callback (which probably goes through JS) while locked. Do you think it would make sense to build up a list of {name, size} entries and then report outside of the thread enumeration loop?

Oh, yeah, that's a good point. I don't think it will be an issue in practice, but it's probably better to try to do as little as possible while holding the lock, anyway.

> Should we append the `tid` as well to differentiate threads with the same name? It looks like our threadpool adds a unique identifier to each thread so this might not be an issue in practice.
> 
> If you do add it we'll want to update the about:memory diff tool to normalize out the `tid`.

I'm kind of on the fence about that. Thread pools do get their own numbers, yeah, which is part of the reason I decided to ignore name clashes at the start. But we wind up with multiple threads with certain names, like "DOM Worker", which get big, and just wind up with [2] after their entry right now... So it would probably be a good idea, yeah.

Comment 27

[Eric Rahm [:erahm]]

Comment on attachment 8992235 [details]
Bug 1475899: Part 5 - Add thread stack memory reporter for Windows.

https://reviewboard.mozilla.org/r/257116/#review264230

Logically looks good, I'll defer to the Windows experts on the finer details.

::: xpcom/base/MemoryInfo.cpp:26
(Diff revision 1)
> +  const char* ptr = reinterpret_cast<const char*>(aPtr);
> +  const char* end = ptr + aSize;
> +  DebugOnly<void*> base = nullptr;
> +  while (ptr < end) {
> +    MEMORY_BASIC_INFORMATION basicInfo;
> +    if (!VirtualQuery(ptr, &basicInfo, sizeof(basicInfo))) {

If this fails on the first loop we're going to be in a weird spot where `mStart` == `mEnd`. Maybe that's okay, but we might want to handle that and indicate there's an error.

::: xpcom/threads/nsThread.cpp:1
(Diff revision 1)
> -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* -*- Mode: C++; tab-with: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

nit: stray removal

Comment 28

[Eric Rahm [:erahm]]

Comment on attachment 8992234 [details]
Bug 1475899: Part 4 - Add memory reporter for committed thread stack sizes on Linux.

https://reviewboard.mozilla.org/r/257114/#review264220

> Oh, yeah, that's a good point. I don't think it will be an issue in practice, but it's probably better to try to do as little as possible while holding the lock, anyway.

The normalization stuff lives in [aboutMemory.js](https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/toolkit/components/aboutmemory/content/aboutMemory.js#797-830). Doing something like `<thread_name> (tid=NNN)` would probably be easiest.

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8992234 [details]
Bug 1475899: Part 4 - Add memory reporter for committed thread stack sizes on Linux.

https://reviewboard.mozilla.org/r/257114/#review264220

> Can you file a follow up to a) convert things to use MemoryMappings if it makes sense and b) merge reporters so that we only parse smaps once.

Filed bug 1476168

Comment 30

[Kris Maglione [:kmag]]

Comment on attachment 8992232 [details]
Bug 1475899: Part 2 - Store stack base pointer for nsThreads on Linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257110/diff/1-2/

Comment 31

[Kris Maglione [:kmag]]

Comment on attachment 8992233 [details]
Bug 1475899: Part 3 - Add helper for parsing memory mappings on Linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257112/diff/1-2/

Comment 32

[Kris Maglione [:kmag]]

Comment on attachment 8992234 [details]
Bug 1475899: Part 4 - Add memory reporter for committed thread stack sizes on Linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257114/diff/1-2/

Comment 33

[Kris Maglione [:kmag]]

Comment on attachment 8992235 [details]
Bug 1475899: Part 5 - Add thread stack memory reporter for Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257116/diff/1-2/

Comment 34

[(Away)]

Sad to see so much duplication between this and `WindowsAddressSpaceReporter::CollectReports`, but what to do...

Comment 35

[Eric Rahm [:erahm]]

Comment on attachment 8992234 [details]
Bug 1475899: Part 4 - Add memory reporter for committed thread stack sizes on Linux.

https://reviewboard.mozilla.org/r/257114/#review264444

Looks good, thanks for the update. Can you please file a follow up for updating about:memory's diff normalization to handle thread tid's?

::: xpcom/base/nsMemoryReporterManager.cpp:1439
(Diff revision 2)
> +
> +      int idx = mappings.BinaryIndexOf(thread->StackBase());
> +      if (idx < 0) {
> +        continue;
> +      }
> +      size_t privateSize = mappings[idx].Referenced();

Can you add a comment here about this logic (what's Referenced, why do we do the min of it and StackSize).

Comment 36

[Aaron Klotz [:aklotz]]

Comment on attachment 8992235 [details]
Bug 1475899: Part 5 - Add thread stack memory reporter for Windows.

https://reviewboard.mozilla.org/r/257116/#review264438

r=me when issues are addressed, but please resubmit for review if you implement MemoryInfo::Dump

::: xpcom/base/MemoryInfo.h:59
(Diff revision 2)
> +  size_t Reserved() const { return mReserved; }
> +  size_t Committed() const { return mCommitted; }
> +  size_t Free() const { return mFree; }
> +  size_t Size() const { return mSize; }
> +
> +  void Dump(nsACString& aOut) const;

I don't see a definition for this. Either remove the declaration or please implement it and resubmit for review so that I can go over that code.

::: xpcom/base/MemoryInfo.h:80
(Diff revision 2)
> +  size_t mFree = 0;
> +  size_t mSize = 0;
> +
> +  PageTypeSet mType;
> +
> +  PermSet mPerms{};

Nit: mPerms has {} but mType does not.

::: xpcom/base/MemoryInfo.cpp:59
(Diff revision 2)
> +      }
> +      if (basicInfo.Type & MEM_PRIVATE) {
> +        result.mType += PageType::Private;
> +      }
> +
> +      switch (basicInfo.AllocationProtect & 0xff) {

Please add a comment to explain the bitwise AND. I get it, but others may not.

::: xpcom/base/MemoryInfo.cpp:60
(Diff revision 2)
> +      if (basicInfo.Type & MEM_PRIVATE) {
> +        result.mType += PageType::Private;
> +      }
> +
> +      switch (basicInfo.AllocationProtect & 0xff) {
> +      case PAGE_EXECUTE_WRITECOPY:

Nit: Can you shift the indentation once to the right inside the switch?

::: xpcom/base/MemoryInfo.cpp:61
(Diff revision 2)
> +        result.mType += PageType::Private;
> +      }
> +
> +      switch (basicInfo.AllocationProtect & 0xff) {
> +      case PAGE_EXECUTE_WRITECOPY:
> +        result.mPerms += Perm::CopyOnWrite;

Please add MOZ_FALLTHROUGH annotations to these `PAGE_EXECUTE_*` cases so that:

* It is more clear to the reader;
* To suppress any compiler warnings.

::: xpcom/base/MemoryInfo.cpp:71
(Diff revision 2)
> +      case PAGE_EXECUTE:
> +        result.mPerms += Perm::Execute;
> +        break;
> +
> +      case PAGE_WRITECOPY:
> +        result.mPerms += Perm::CopyOnWrite;

Same here re: MOZ_FALLTHROUGH.

::: xpcom/threads/nsThread.cpp:486
(Diff revision 2)
>      // consumption of our allocated stacks.
>      madvise(self->mStackBase, stackSize, MADV_NOHUGEPAGE);
>  
>      pthread_attr_destroy(&attr);
> +#elif defined(XP_WIN)
> +    static const DynamicallyLinkedFunctionPtr<GetCurrentThreadStackLimitsFn>

Thanks for using this!

Comment 37

[Kris Maglione [:kmag]]

Comment on attachment 8992235 [details]
Bug 1475899: Part 5 - Add thread stack memory reporter for Windows.

https://reviewboard.mozilla.org/r/257116/#review264438

> I don't see a definition for this. Either remove the declaration or please implement it and resubmit for review so that I can go over that code.

Oh, yeah. Sorry. I accidentally copied that from the Linux MemoryMapping class. I'll remove it for now. I may implement it in the future if we wind up needing it for debugging.

> Please add MOZ_FALLTHROUGH annotations to these `PAGE_EXECUTE_*` cases so that:
> 
> * It is more clear to the reader;
> * To suppress any compiler warnings.

I didn't get any compiler warnings, which surprised me, but is why I left it out. I'll add it for clarity/future warning changes, though.

Comment 38

[Aaron Klotz [:aklotz]]

Comment on attachment 8992235 [details]
Bug 1475899: Part 5 - Add thread stack memory reporter for Windows.

https://reviewboard.mozilla.org/r/257116/#review264458

::: xpcom/base/MemoryInfo.cpp:77
(Diff revision 2)
> +      case PAGE_READWRITE:
> +        result.mPerms += Perm::Write;
> +      case PAGE_READONLY:
> +        result.mPerms += Perm::Read;
> +        break;
> +      }

Also: Please add a default case.

Comment 39

[Kris Maglione [:kmag]]

Comment on attachment 8992235 [details]
Bug 1475899: Part 5 - Add thread stack memory reporter for Windows.

https://reviewboard.mozilla.org/r/257116/#review264230

> If this fails on the first loop we're going to be in a weird spot where `mStart` == `mEnd`. Maybe that's okay, but we might want to handle that and indicate there's an error.

The shouldn't actually happen, in practice, unless the pointer is invalid. I've been considering `Size() < aSize` to be the error case, since the caller is expected to know the actual size of the mapping.

Comment 40

[Kris Maglione [:kmag]]

Comment on attachment 8992234 [details]
Bug 1475899: Part 4 - Add memory reporter for committed thread stack sizes on Linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257114/diff/2-3/

Comment 41

[Kris Maglione [:kmag]]

Comment on attachment 8992235 [details]
Bug 1475899: Part 5 - Add thread stack memory reporter for Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257116/diff/2-3/

Comment 42

[Eric Rahm [:erahm]]

Comment on attachment 8992234 [details]
Bug 1475899: Part 4 - Add memory reporter for committed thread stack sizes on Linux.

https://reviewboard.mozilla.org/r/257114/#review264480

Thanks for the thorough comment, looks good.

Comment 43

[Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧]

Comment on attachment 8992233 [details]
Bug 1475899: Part 3 - Add helper for parsing memory mappings on Linux.

https://reviewboard.mozilla.org/r/257112/#review264552

LGTM.  (Also, njn wrote most of the old system memory reporter; I mostly just added support for Android things like tagged anonymous memory and `pmem`.)

::: xpcom/base/MemoryMapping.cpp:123
(Diff revision 2)
> +  // getline() will reallocate the buffer if it isn't large enough, and
> +  // replace the value in our pointer. In the end, we need to free whatever its
> +  // final value is. UniquePtr does not give us a way to handle this. Scoped
> +  // does.
> +  size_t bufferSize = 1024;
> +  ScopedCharArray buffer(new char[bufferSize]);

I was going to point out that this mixes `new[]` with realloc and free, but comment #16 already flagged this as a nit, and it looks like there's stuff in `memory/` that at least makes it not unsafe.

Comment 44

[Kris Maglione [:kmag]]

Comment on attachment 8992233 [details]
Bug 1475899: Part 3 - Add helper for parsing memory mappings on Linux.

https://reviewboard.mozilla.org/r/257112/#review264552

> I was going to point out that this mixes `new[]` with realloc and free, but comment #16 already flagged this as a nit, and it looks like there's stuff in `memory/` that at least makes it not unsafe.

Yeah, we override `operator new` to use moz_xalloc, so for types without destructors (e.g., char arrays), it's safe to mix with realloc and free.

Comment 45

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2916b5e05b6d100e91448f21cb4082e41a86e87c
Bug 1475899: Part 1 - Track live nsThreads and add a method of enumerating them. r=erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/391b97f0e5c029880d4a1ae697463cfcb8839116
Bug 1475899: Part 2 - Store stack base pointer for nsThreads on Linux. r=erahm,jld

https://hg.mozilla.org/integration/mozilla-inbound/rev/e89ebe1f22f28d2b667514cb66d39606136a2f58
Bug 1475899: Part 3 - Add helper for parsing memory mappings on Linux. r=erahm,jld

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4394660fde2b05e972c491246570d8f79d8a7c6
Bug 1475899: Part 4 - Add memory reporter for committed thread stack sizes on Linux. r=erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbf0e4b12c8e05cabca321d352df32735b8baec9
Bug 1475899: Part 5 - Add thread stack memory reporter for Windows. r=erahm,aklotz

Comment 46

[Cristian Brindusan [:cbrindusan]]

Backed out 5 changesets (bug 1475899) for build bustages on /workspace/build/src/xpcom/base/MemoryMapping.cpp. CLOSED TREE

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/30dae375573fce73ed4d137c9806a358b011ca8a

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fbf0e4b12c8e05cabca321d352df32735b8baec9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&selectedJob=188838635 

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=188838635&repo=mozilla-inbound&lineNumber=999

Comment 47

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9daa194d334fd4179899677cfcba2ba5df9d0ee0
Bug 1475899: Part 1 - Track live nsThreads and add a method of enumerating them. r=erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8438eed61d07e3d99aa4f64de7d677d9b6e5d81
Bug 1475899: Part 2 - Store stack base pointer for nsThreads on Linux. r=erahm,jld

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4cc057e04bd0e1658236df1e059807296cf0f0a
Bug 1475899: Part 3 - Add helper for parsing memory mappings on Linux. r=erahm,jld

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dbddd89571a35cf12b88e0f8fb27d588c14b210
Bug 1475899: Part 4 - Add memory reporter for committed thread stack sizes on Linux. r=erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/26b6d8585579e0a1839230d67b58c58e7d9406c2
Bug 1475899: Part 5 - Add thread stack memory reporter for Windows. r=erahm,aklotz

Comment 48

[Kris Maglione [:kmag]]

Note for performance sheriffs: This push is going to result in a major increase in the Base Content Explicit numbers. That is expected, and not a real regression. It's the result of better reporting of our explicit allocations, and isn't a problem unless our Base Content Resident Unique numbers also go up.

Comment 49

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a79aaf6a5ce27eb5a70a08d943b585cf3885673
Bug 1475899: Follow-up: Temporarily disable thread stack size assertion on Android. r=bustage

Comment 50

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9daa194d334f
https://hg.mozilla.org/mozilla-central/rev/d8438eed61d0
https://hg.mozilla.org/mozilla-central/rev/c4cc057e04bd
https://hg.mozilla.org/mozilla-central/rev/8dbddd89571a
https://hg.mozilla.org/mozilla-central/rev/26b6d8585579
https://hg.mozilla.org/mozilla-central/rev/5a79aaf6a5ce

Comment 51

[Ionuț Goldan [:igoldan]]

(In reply to Kris Maglione [:kmag] from comment #48)
> Note for performance sheriffs: This push is going to result in a major
> increase in the Base Content Explicit numbers. That is expected, and not a
> real regression. It's the result of better reporting of our explicit
> allocations, and isn't a problem unless our Base Content Resident Unique
> numbers also go up.

Thanks for the heads up!