Closed Bug 1475899 Opened Last year Closed Last year

Add memory reporter for committed size of thread stacks

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [overhead:noted])

Attachments

(5 files)

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 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 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;
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.
Flags: needinfo?(aklotz)
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 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.
Attachment #8992231 - Flags: review?(erahm) → review+
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 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.
Attachment #8992232 - Flags: review?(erahm) → review+
Attachment #8992232 - Flags: review?(jld)
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 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`?
Attachment #8992232 - Flags: review?(jld) → review+
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 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.
Attachment #8992233 - Flags: review?(erahm) → review+
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 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 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.
(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.
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?
Attachment #8992235 - Flags: review?(mhowell) → review?(dmajor)
(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 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.
Attachment #8992235 - Flags: review-
Flags: needinfo?(aklotz)
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 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`.
Attachment #8992234 - Flags: review?(erahm) → review-
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 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
Attachment #8992235 - Flags: review?(erahm) → review+
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 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
Sad to see so much duplication between this and `WindowsAddressSpaceReporter::CollectReports`, but what to do...
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).
Attachment #8992234 - Flags: review?(erahm) → review-
Blocks: 1476371
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!
Attachment #8992235 - Flags: review?(aklotz) → review+
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 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 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.
Blocks: 1476403
Blocks: 1476405
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.
Attachment #8992234 - Flags: review?(erahm) → review+
Blocks: 1476432
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.
Attachment #8992233 - Flags: review?(jld) → review+
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.
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
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
Whiteboard: [overhead:noted]
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.
Flags: needinfo?(kmaglione+bmo)
See Also: → 1476828
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a79aaf6a5ce27eb5a70a08d943b585cf3885673
Bug 1475899: Follow-up: Temporarily disable thread stack size assertion on Android. r=bustage
(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!
You need to log in before you can comment on or make changes to this bug.