986302 - add memory reporter for HPACK tables

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[u408661]

Just like the summary says - we should keep track of the memory used by HPACK.

Comment 1

[u408661]

Assigning to Nate for a decent first bug. We'll discuss more details in person :)

Comment 2

[nhughes]

Attachment: HPACK Table Memory Reporting

After hitting "measure" on the about:memory page, the EXC_BAD_ACCESS exception is thrown before the program eventually crashes.

Comment 3

[u408661]

Comment on attachment 8617406 [details] [diff] [review]
HPACK Table Memory Reporting

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

So there's a lot of other things in this patch that I want to comment on, but I'll save these for later (no biggie - this IS your first patch, after all!) I took a look at the code and immediately had a thought about the issue, and then running a build with this under lldb proved me correct. The interesting bit of output is:

Hit MOZ_CRASH(MyHpackTableReporter not thread-safe) at /Users/nwgh/src/mozilla/gecko/netwerk/protocol/http/Http2Compression.cpp:59

Note the bit about "not thread-safe". Try changing NS_DECL_ISUPPORTS to NS_DECL_THREADSAFE_ISUPPORTS and see what happens. It could be that we can't have memory reporters that measure non-main-thread memory usage (or we may have to be more careful with them) but see if that at least gets you past this first hurdle (it's possible it won't, and we'll have to re-think anyway).

Comment 4

[nhughes]

Attachment: MozReview Request: Bug 986302: Implement memory reporter for HPACK tables

Bug 986302: Implement memory reporter for HPACK tables

Comment 5

[nhughes]

Attachment: MozReview Request: Bug 986302: Add memory reporter for HPACK tables

Bug 986302: Add memory reporter for HPACK tables
Remove print statements used for debugging
Add Http2BaseCompressor member to MyHpackTableReporter class
Update MyHpackTableReporter constructor to accept Http2BaseCompressor parameter
Change NS_DECL_ISUPPORTS macro to NS_DECL_THREADSAFE_ISUPPORTS
Report aggregate Hpack table memory usage
Add const qualifier to memory measurement functions (Http2BaseCompressor)
Inline Http2BaseCompressor::SizeOfExcludingThis()

Comment 6

[u408661]

Comment on attachment 8620462 [details]
MozReview Request: Bug 986302: Add memory reporter for HPACK tables

https://reviewboard.mozilla.org/r/10771/#review9557

::: netwerk/protocol/http/Http2Compression.cpp:54
(Diff revision 1)
> -      "TREE-PATH", KIND_HEAP, UNITS_BYTES,
> +      "explicit/network/HPACK-tables", KIND_HEAP, UNITS_BYTES,

lowercase hpack

::: netwerk/protocol/http/Http2Compression.cpp:56
(Diff revision 1)
> -      "Description");
> +      "Aggregate HPACK table memory usage");       

Might be better to describe it as "Aggregate memory usage of HPACK dynamic tables" to separate it from the static table (which you'll be adding a separate reporter for)

::: netwerk/protocol/http/Http2Compression.cpp:31
(Diff revision 1)
>    MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)

Don't rely on the implicit-private rule of C++ classes, it makes the code harder to read. Additionally, our convention is to put public fields first, protected fields second, and private fields last.

Comment 7

[u408661]

Comment on attachment 8620461 [details]
MozReview Request: Bug 986302: Implement memory reporter for HPACK tables

https://reviewboard.mozilla.org/r/10511/#review9555

::: netwerk/protocol/http/Http2Compression.h:63
(Diff revision 1)
> +  // HPACK table memory report functions

Don't need this comment, SizeOf{In,Ex}cludingThis is common enough to be well known.

::: netwerk/protocol/http/Http2Compression.cpp:20
(Diff revision 1)
> +#include "nsISupports.h"

Don't need to include nsISupports.h, nsIMemoryReporter.h will include it as necessary.

::: netwerk/protocol/http/Http2Compression.cpp:34
(Diff revision 1)
> +  

Your editor seems to be leaving trailing whitespace on lines. Please configure it to not do that :)

::: netwerk/protocol/http/Http2Compression.cpp:27
(Diff revision 1)
> +// HPACK table report class

Again, this comment isn't necessary - the class name is self-explanatory.

::: netwerk/protocol/http/Http2Compression.cpp:29
(Diff revision 1)
> +class MyHpackTableReporter final : public nsIMemoryReporter

Get rid of the "My" here.

::: netwerk/protocol/http/Http2Compression.cpp:44
(Diff revision 1)
> +    fprintf(stderr, "collecting hpack reports\n");

No fprintfs in the final commit.

::: netwerk/protocol/http/Http2Compression.cpp:49
(Diff revision 1)
> +      "TREE-PATH", KIND_HEAP, UNITS_BYTES,

Need a real path here (same with description a few lines down)

::: netwerk/protocol/http/Http2Compression.cpp:53
(Diff revision 1)
> +    NS_ENSURE_SUCCESS(rv, rv);

This isn't necessary, since you're returning after this point anyway. Just "return rv" and be done.

::: netwerk/protocol/http/Http2Compression.cpp:59
(Diff revision 1)
> +NS_IMPL_ISUPPORTS(MyHpackTableReporter, nsIMemoryReporter) // exception here

Blank line before this.

::: netwerk/protocol/http/Http2Compression.cpp:243
(Diff revision 1)
> +  // Construct the memory reporter

Unnecessary comment. Your comments should be explaining "why", not "what" (and then pretty much only when there's something non-obvious going on). Otherwise, comments are just noise in the source.

::: netwerk/protocol/http/Http2Compression.cpp:292
(Diff revision 1)
> +inline size_t 

Don't inline this, it won't be called often enough to make inlining necessary.

::: netwerk/protocol/http/Http2Compression.cpp:302
(Diff revision 1)
> +  return aMallocSizeOf(&mHeaderTable) + SizeOfExcludingThis(aMallocSizeOf);

This ends up double-counting mHeaderTable. Rather, do "return SizeOfExcludingThis(aMallocSizeOf) + aMallocSizeOf(this);"

::: netwerk/protocol/http/Http2Compression.cpp:295
(Diff revision 1)
> +  return aMallocSizeOf(&mHeaderTable);

So this won't actually count everything - you're going to have to iterate over the table, and measure each entry individually (since each entry is what's allocated on the heap, and mHeaderTable is not heap-allocated).

It actually gets slightly more complex than that, even, as the strings within each entry are also heap-allocated on their own. So you need to measure the nvPair + nvPair::name + nvPair::value

Lastly, to make it even more complex, the static table (indices one through sixty-something) are shared across all instances of Http2BaseCompressor, and are heap-allocated. So, to make sure you count those, but don't double (or triple, or quadruple, or ...) count them, you'll have to add yet another memory reporter just for that.

::: netwerk/protocol/http/Http2Compression.cpp:245
(Diff revision 1)
> +  MyHpackTableReporter *reporter = new MyHpackTableReporter();

You should have reporter be a member of the class instead, like:

nsRefPtr<HpackTableReporter> reporter;

As you need to unregister the reporter in the Http2BaseCompressor::~Http2BaseCompressor to avoid memory leaks and crashes when an h2 session gets closed down (and the compressor goes away).

Comment 8

[u408661]

OK, there's your first set of reviews. For the future, since these are both working on the same bits, only submit one patch in the review - makes it easier to see the final product (unlike middle school math class, I don't need to see your work :)

Comment 9

[nhughes]

https://reviewboard.mozilla.org/r/10511/#review9665

> You should have reporter be a member of the class instead, like:
> 
> nsRefPtr<HpackTableReporter> reporter;
> 
> As you need to unregister the reporter in the Http2BaseCompressor::~Http2BaseCompressor to avoid memory leaks and crashes when an h2 session gets closed down (and the compressor goes away).

Is it still acceptable for the Reporter to have a pointer back to the Http2BaseCompressor object so that it can call Http2BaseCompressor::SizeOf{In,Ex}cludingThis when generating the report(s)?

Comment 10

[u408661]

(In reply to nhughes from comment #9)
> https://reviewboard.mozilla.org/r/10511/#review9665
> 
> > You should have reporter be a member of the class instead, like:
> > 
> > nsRefPtr<HpackTableReporter> reporter;
> > 
> > As you need to unregister the reporter in the Http2BaseCompressor::~Http2BaseCompressor to avoid memory leaks and crashes when an h2 session gets closed down (and the compressor goes away).
> 
> Is it still acceptable for the Reporter to have a pointer back to the
> Http2BaseCompressor object so that it can call
> Http2BaseCompressor::SizeOf{In,Ex}cludingThis when generating the report(s)?

Absolutely! Just don't make it a strong reference (irrelevant in this case, since Http2BaseCompressor and children aren't refcounted, but could cause issues if it were an nsIWhatever).

Comment 11

[nhughes]

https://reviewboard.mozilla.org/r/10511/#review9791

> So this won't actually count everything - you're going to have to iterate over the table, and measure each entry individually (since each entry is what's allocated on the heap, and mHeaderTable is not heap-allocated).
> 
> It actually gets slightly more complex than that, even, as the strings within each entry are also heap-allocated on their own. So you need to measure the nvPair + nvPair::name + nvPair::value
> 
> Lastly, to make it even more complex, the static table (indices one through sixty-something) are shared across all instances of Http2BaseCompressor, and are heap-allocated. So, to make sure you count those, but don't double (or triple, or quadruple, or ...) count them, you'll have to add yet another memory reporter just for that.

Should the static table reporter get registered as "strong"? My thinking is that it should because it's a "global" reporter that isn't tied to the life-span of a shorter-lived Http2BaseCompressor object. Furtheremore, should the HpackStaticTableReporter class be static?

Comment 12

[u408661]

https://reviewboard.mozilla.org/r/10511/#review9885

> Should the static table reporter get registered as "strong"? My thinking is that it should because it's a "global" reporter that isn't tied to the life-span of a shorter-lived Http2BaseCompressor object. Furtheremore, should the HpackStaticTableReporter class be static?

Yes, register it as strong. However, do not make the class static - static constructors are the devil (which is why there's those hoops around allocating and freeing the static table dynamically), just register it when the static table is constructed, and unregister it when the static table is destructed.

Comment 13

[nhughes]

Attachment: Hpack Table Memory Reporting Patch

It seems that the static table's memory usage isn't getting printed in about:memory (i.e. CollectReports() isn't being called) even though the reporter is being instantiated/registered.

Comment 14

[u408661]

Comment on attachment 8623453 [details] [diff] [review]
Hpack Table Memory Reporting Patch

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

::: netwerk/protocol/http/Http2Compression.cpp
@@ +23,5 @@
>  namespace mozilla {
>  namespace net {
>  
> +class
> +HpackDynamicTableReporter final : public nsIMemoryReporter

"class" on the same line as the name, please

@@ +30,3 @@
>  public:
>  
> +HpackDynamicTableReporter(Http2BaseCompressor *target)

This line needs indenting

@@ +39,2 @@
>    NS_IMETHOD
>    CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, bool anonymize)

I believe this needs to be declared override

@@ +42,3 @@
>      nsresult rv;
>  
>      rv = MOZ_COLLECT_REPORT(

Don't save the rv here just to not use it. Just "return MOZ_COLLECT_REPORT..."

@@ +42,4 @@
>      nsresult rv;
>  
>      rv = MOZ_COLLECT_REPORT(
> +      "explicit/network/hpack-tables", KIND_HEAP, UNITS_BYTES,

call this explicit/network/hpack/dynamic-tables

@@ +42,5 @@
>      nsresult rv;
>  
>      rv = MOZ_COLLECT_REPORT(
> +      "explicit/network/hpack-tables", KIND_HEAP, UNITS_BYTES,
> +      mCompressor->SizeOfIncludingThis(MallocSizeOf),

So we probably don't want to use (or even have) SizeOfIncludingThis, as the compressors aren't (directly) heap-allocated - they're created as members of an Http2Session, so the count for them should come as part of the session (which we aren't counting either, but minor details...)

@@ +63,5 @@
>  
>  static nsDeque *gStaticHeaders = nullptr;
>  
> +class
> +HpackStaticTableReporter final : public nsIMemoryReporter

"class" on the same line as the name, please

@@ +68,5 @@
> +{
> +
> +public:
> +
> +HpackStaticTableReporter()

this line needs indented

@@ +70,5 @@
> +public:
> +
> +HpackStaticTableReporter()
> +  {
> +    RegisterStrongMemoryReporter(this);

Don't register here, register where you call new (in InitializeStaticHeaders)

@@ +76,5 @@
> +
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +
> +  NS_IMETHOD
> +  CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, bool anonymize)

I believe this needs to be declared override

@@ +80,5 @@
> +  CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, bool anonymize)
> +  {
> +    nsresult rv;
> +
> +    rv = MOZ_COLLECT_REPORT(

Don't save the rv here just to not use it. Just "return MOZ_COLLECT_REPORT..."

@@ +81,5 @@
> +  {
> +    nsresult rv;
> +
> +    rv = MOZ_COLLECT_REPORT(
> +      "explicit/network/hpack-static-table", KIND_HEAP, UNITS_BYTES,

call this explicit/network/hpack/static-table

@@ +82,5 @@
> +    nsresult rv;
> +
> +    rv = MOZ_COLLECT_REPORT(
> +      "explicit/network/hpack-static-table", KIND_HEAP, UNITS_BYTES,
> +      gStaticHeaders->SizeOfIncludingThis(MallocSizeOf),

Are you sure this is the patch you tested? Because, unless I'm crazy, this shouldn't compile - gStaticHeaders doesn't have a SizeOfIncludingThis anywhere in it.

@@ +98,5 @@
> +};
> +
> +NS_IMPL_ISUPPORTS(HpackStaticTableReporter, nsIMemoryReporter)
> +
> +HpackStaticTableReporter *staticReporter = new HpackStaticTableReporter();

This is a static constructor. Please create the static reporter in the same place that gStaticTable gets created. Also, call it gStaticReporter to follow naming conventions. (Similarly, you'll need to delete gStaticReporter in the same place that gStaticHeaders is deleted.) DO make gStaticReporter a static variable, though (static pointer does not create a static constructor, as long as you don't new in the initialization of said static pointer).

@@ +280,5 @@
>    : mOutput(nullptr)
>    , mMaxBuffer(kDefaultMaxBuffer)
>  {
> +  dynamicReporter = new HpackDynamicTableReporter(this);
> +  RegisterWeakMemoryReporter(dynamicReporter);

Why is this one weak?

@@ +286,5 @@
> +
> +Http2BaseCompressor::~Http2BaseCompressor()
> +{
> +  UnregisterWeakMemoryReporter(dynamicReporter);
> +  //UnregisterWeakMemoryReporter(staticReporter);

This will go in Http2CompressionCleanup

@@ +337,3 @@
>  {
> +  size_t size = 0;
> +  for(uint32_t i=0; i<mHeaderTable.Length(); i++)

spaces around "=" and "<"

@@ +337,5 @@
>  {
> +  size_t size = 0;
> +  for(uint32_t i=0; i<mHeaderTable.Length(); i++)
> +  {
> +    void *name = (void*)&(mHeaderTable[i]->mName);

No need for these casts to void*, just do "&(mHeaderTable[i]->mName)" in your calls to aMallocSizeOf below - all pointers degrade to a void* just fine.

If you want to shorten the "size += ..." line (which is good!), instead have nvPair pair = mHeaderTable[i]; and then operate on "pair" in your loop.

Also, I could swear there's a better way of measuring the memory usage of strings (since this won't give you what you want), but I'm having trouble finding it. Perhaps ask in #developers on irc

@@ +347,5 @@
>  }
>  
>  
> +size_t
> +Http2BaseCompressor::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const

See my note above about using SizeOfExcludingThis instead of this one

@@ +354,4 @@
>  }
> +/*
> +size_t
> +Http2BaseCompressor::SizeOfStaticTable(mozilla::MallocSizeOf aMallocSizeOf) const

This isn't used, get rid of it.

::: netwerk/protocol/http/Http2Compression.h
@@ +61,5 @@
>  {
>  public:
>    Http2BaseCompressor();
> +  virtual ~Http2BaseCompressor();
> +  void operator=(const Http2BaseCompressor &compressor);

This should not be necessary, get rid of it.

Comment 15

[nhughes]

I'm getting the following without explicitly casting to type void*:

error: no viable conversion from 'const nsCString' to 'const void *'

Comment 16

[nhughes]

Attachment: hpackTableReporting.patch

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 8628396 [details] [diff] [review]
hpackTableReporting.patch

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

I have lots of comments, but I want to emphasise that you're heading in the right general direction. Memory reporting code has a bunch of non-obvious idioms. These are important, because as you've already seen, a mis-written reporter will cause crashes. It also means this is not an easy first bug! It might also explain why some of my suggestions are slightly at cross-purpose with some of hurley's suggestions.

I'm giving f+ because you're making good progress, but I'd like to see another version with my comments addressed. Keep perservering, the first bug is always the hardest :)

::: netwerk/protocol/http/HpackTableReporting.cpp
@@ +5,5 @@
> +namespace net {
> +
> +NS_IMPL_ISUPPORTS(HpackDynamicTableReporter, nsIMemoryReporter)
> +
> +HpackDynamicTableReporter::HpackDynamicTableReporter(Http2BaseCompressor *target)

Nit: args have |aFoo| form. Here I would use |aCompressor| to match |mCompressor|.

@@ +7,5 @@
> +NS_IMPL_ISUPPORTS(HpackDynamicTableReporter, nsIMemoryReporter)
> +
> +HpackDynamicTableReporter::HpackDynamicTableReporter(Http2BaseCompressor *target)
> +{
> +  mCompressor = target;

Nit: the use of C++ initializer lists is generally preferred to the use of assignment in constructors.

@@ +13,5 @@
> +
> +HpackDynamicTableReporter::~HpackDynamicTableReporter() {}
> +
> +NS_IMETHODIMP
> +HpackDynamicTableReporter::CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, bool anonymize)

Should be |aAnonymize|.

@@ +16,5 @@
> +NS_IMETHODIMP
> +HpackDynamicTableReporter::CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, bool anonymize)
> +{
> +  return MOZ_COLLECT_REPORT(
> +    "explicit/network/dynamic-tables", KIND_HEAP, UNITS_BYTES,

Use "explicit/network/hpack/dynamic-tables" as hurley suggested.

@@ +25,5 @@
> +NS_IMPL_ISUPPORTS(HpackStaticTableReporter, nsIMemoryReporter)
> +
> +HpackStaticTableReporter::HpackStaticTableReporter() {}
> +
> +HpackStaticTableReporter::~HpackStaticTableReporter() {}

Nit: for methods with very small bodies I would usually inline the body into the class declaration. Some people might not, though.

@@ +28,5 @@
> +
> +HpackStaticTableReporter::~HpackStaticTableReporter() {}
> +
> +NS_IMETHODIMP
> +HpackStaticTableReporter::CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, bool anonymize)

Should be |aAnonymize|.

@@ +31,5 @@
> +NS_IMETHODIMP
> +HpackStaticTableReporter::CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, bool anonymize)
> +{
> +  return MOZ_COLLECT_REPORT(
> +    "explicit/network/hpack-static-table", KIND_HEAP, UNITS_BYTES,

Use "explicit/network/hpack/static-table" as hurley suggested.

::: netwerk/protocol/http/HpackTableReporting.h
@@ +18,5 @@
> +
> +class HpackDynamicTableReporter final : public nsIMemoryReporter
> +{
> +
> +public:

Nit: no need for the blank line before |public|. Ditto in the class below.

@@ +24,5 @@
> +
> +  HpackDynamicTableReporter(Http2BaseCompressor *target);
> +
> +  NS_IMETHODIMP
> +  CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, bool anonymize);

Nit: should be |aAnonymize|. Ditto in the class below.

@@ +28,5 @@
> +  CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, bool anonymize);
> +
> +private:
> +
> +  MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)

Nit: no need for the blank line after |private|. Ditto in the class below.

@@ +34,5 @@
> +  ~HpackDynamicTableReporter();
> +
> +  Http2BaseCompressor *mCompressor; // the compressor being reported on
> +
> +};

Nit: no need for blank line before closing brace. Ditto in the class below.

::: netwerk/protocol/http/Http2Compression.cpp
@@ +22,5 @@
>  namespace mozilla {
>  namespace net {
>  
> +nsDeque *gStaticHeaders = nullptr;
> +static HpackStaticTableReporter *gStaticReporter;

First, |gStaticReporter| needs to be a ref-counted pointer. You should use StaticRefPtr<>. Look at |sWindowReporter| in dom/base/nsWindowMemoryReporter.cpp as an example, but ignore the use of ClearOnShutdown() there.

Second, I appreciate that you're going for modularity, but I strongly suggest moving the code from the new files into this file. Because...

- The added reporter classes are small enough that having them in a separate file isn't much of a win.

- That will make the patch smaller because you don't need all the boilerplate that comes with a new file.

- Finally, |gStaticHeaders| won't need to be made visible outside this file; it can remain |static|.

@@ +30,5 @@
>  {
>    // this happens after the socket thread has been destroyed
>    delete gStaticHeaders;
>    gStaticHeaders = nullptr;
> +  gStaticReporter = nullptr;

Before nulling gStaticReporter, you should call UnregisterStrongReporter() to be safe. We don't want the memory reporter manager to call CollectReports() when gStaticHeaders is null.

And once you've made gStaticReporter a ref-counted pointer (as I mentioned above), nulling it will reduce the ref-count to zero and it will be freed.

@@ +52,5 @@
>  {
>    MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
>    if (!gStaticHeaders) {
>      gStaticHeaders = new nsDeque();
> +    gStaticReporter = new HpackStaticTableReporter();

You're never registering the static table reporter, so it won't ever be called. You probably want this:

> gStaticReporter = new HpackStaticTableReporter();
> RegisterStrongReporter(gStaticReporter);

@@ +206,5 @@
>    : mOutput(nullptr)
>    , mMaxBuffer(kDefaultMaxBuffer)
>  {
> +  mDynamicReporter = new HpackDynamicTableReporter(this);
> +  RegisterStrongMemoryReporter(mDynamicReporter);

You're never unregistering the reporter. So once this object is destroyed, the memory reporter manager will call the still-registered reporter, which will try to measure the (destroyed) object via a dangling pointer and you'll probably get a crash.

So you'll want to call UnregisterStrongMemoryReporter() in the destructor.

@@ +257,5 @@
> +{
> +  size_t size = 0;
> +  void *name;
> +  void *value;
> +  const nvPair *pair;

Nit: the '*' in pointers types goes on the left, e.g. |void* name|. The full Mozilla style guide is at https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style.

Also, I would move the declarations of |name|, |value| and |pair| into the loop, to the point of their first assignment. Having said that, you won't need them once you make the changes I suggest below...

@@ +259,5 @@
> +  void *name;
> +  void *value;
> +  const nvPair *pair;
> +  for(uint32_t i = 0; i < mHeaderTable.Length(); i++)
> +  {

Nit: brace on the same line as "for".

@@ +264,5 @@
> +    pair = mHeaderTable[i];
> +    name = (void*)(&(pair->mName));
> +    value = (void*)(&(pair->mValue));
> +    size += (aMallocSizeOf(pair) + aMallocSizeOf(name)
> +      + aMallocSizeOf(value));

This looks dangerous. You're calling aMallocSizeOf() on the addresses of the mName and mValue. The latter in particular is in the middle of the struct, so it may cause aMallocSizeOf() to crash. (It's only safe to call aMallocSizeOf() on heap pointers.)

There's a solution: mName and mValue are both nsCString, which has its own measurement functions. (This isn't obvious because our string class hierarchy is a mess.) These functions are SizeOfExcludingThisMustBeUnshared() and SizeOfExcludingThisIfUnshared(). Call the former if you know the strings are unshared, otherwise call the latter.

Furthermore, it would be a good idea here to add a SizeOfExcludingThis() method (and/or SizeOfIncludingThis()? not sure) to nvPair, which would in turn measure the mName and mValue members. You could also do a similar thing with nvFIFO.

Comment 18

[nhughes]

Thanks for the feedback!

I've been trying to unregister that memory reporter, but here is the build error I'm getting:

0:30.44 /Users/nhughes/Desktop/firefox/netwerk/protocol/http/Http2Compression.cpp:35:3: error: use of undeclared identifier 'UnregisterStrongReporter'
0:30.44   UnregisterStrongReporter(gStaticReporter);
0:30.44   ^
0:30.44 1 error generated.

Comment 19

[Nicholas Nethercote [inactive]]

UnregisterStrongReporter is a method within nsIMemoryReporterManager, so you'd call it more like this:

> mgr->UnregisterStrongReporter();

This requires having |mgr| in the first place. There are some convenience functions that will first get |mgr| for you: RegisterStrongMemoryReporter(), RegisterWeakMemoryReporter(), UnregisterWeakMemoryReporter()... but UnregisterStrongMemoryReporter() is missing. You can add it to xpcom/base/nsMemoryReporterManager.cpp, it will be very similar.

Comment 20

[u408661]

Comment on attachment 8628396 [details] [diff] [review]
hpackTableReporting.patch

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

I just want to give a big "+1" to pretty much everything njn said - especially the part about keeping this all in Http2Compression.{cpp,h} - there is really no need for separate files here. The one thing I might disagree with is argument naming - we don't follow the "aFoo" convention in Http2Compression, so let's not muddle things up by having it in some places but not others.

Comment 21

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Hurley [:hurley, :nwgh] from comment #20)
>
> we don't follow the "aFoo" convention in Http2Compression

That's a real shame given that it's a file that's relatively new, by Mozilla standards :(

Comment 22

[nhughes]

Attachment: hpack_table_reporting.diff

Thanks for the feedback on the previous version of this patch. I've addressed the issues you pointed out and I was hoping to get some more feedback on this iteration when you have the time. Specifically, I'd appreciate your thoughts on the way in which I am measuring memory usage of the mName and mValue nsCStrings. I am going to move the memory reporting code from the new files I created; I'm just keeping it separate for the time being because it's a little easier to navigate. Thanks!

Comment 23

[Nicholas Nethercote [inactive]]

The mName and mValue code looks fine; thank you for fixing that.

I started looking at the rest of the patch but stopped when I saw you haven't addressed a few of my previous comments, excluding the don't-create-new-files suggestions. (E.g. you didn't rename "target" as "aCompressor", and you haven't updated the memory report paths as suggested.)

It's not a good use of reviewer time to look at half-fixed patches. I'll be happy to take another look once you've addressed them all, including getting rid of the new files.

Thanks.

Comment 24

[nhughes]

Nick (njn) - Sorry, some of the changes I made (the ones you mentioned) weren't included in that patch. I should have made certain all of my updates were present. I'll be more careful in the future. Thanks.

Comment 25

[nhughes]

Attachment: hpack_table_reporting.diff

Nick (njn) - this version of the patch addresses all of the suggestions you made in your feedback. Please take a look when you have the time. It's not urgent. I really appreciate all of your input. Thanks!

Comment 26

[Nicholas Nethercote [inactive]]

Comment on attachment 8631205 [details] [diff] [review]
hpack_table_reporting.diff

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

Great progress! :)

Most of the comments are for minor formatting issues now. I'm giving f+ again because, given that this is your first patch, it makes sense to have someone check it a final time before landing. I'm happy to look again when you're ready.

I'd be interested to see example output. Can you copy and paste the relevant lines from about:memory here?

::: netwerk/protocol/http/Http2Compression.cpp
@@ +33,5 @@
> +{
> +  return MOZ_COLLECT_REPORT(
> +    "explicit/network/hpack/dynamic-tables", KIND_HEAP, UNITS_BYTES,
> +    mCompressor->SizeOfExcludingThis(MallocSizeOf),
> +    "Aggregate memory usage of HPACK dynamic tables");

'.' at end of the description.

@@ +44,5 @@
> +{
> +  return MOZ_COLLECT_REPORT(
> +    "explicit/network/hpack/static-table", KIND_HEAP, UNITS_BYTES,
> +    gStaticHeaders->SizeOfIncludingThis(MallocSizeOf),
> +    "Memory usage of HPACK static table");

Ditto.

@@ +237,5 @@
>    : mOutput(nullptr)
>    , mMaxBuffer(kDefaultMaxBuffer)
>  {
> +  mDynamicReporter = new HpackDynamicTableReporter(this);
> +  RegisterStrongMemoryReporter(mDynamicReporter);

You added the UnregisterStrongReporter() function, but you forgot to use it :)  Call it in ~Http2BaseCompressor().

(My comment from last time still applies: "once this object is destroyed, the memory reporter manager will call the still-registered reporter, which will try to measure the (destroyed) object via a dangling pointer and you'll probably get a crash." Is the code running without crashing now? How many live Http2BaseCompressors do we have in practice?)

@@ +250,5 @@
> +size_t
> +Http2BaseCompressor::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t size = 0;
> +  for(uint32_t i = 0; i < mHeaderTable.Length(); i++) {

Nit: space after |for|.

::: netwerk/protocol/http/Http2Compression.h
@@ +23,5 @@
> +{
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +
> +  HpackDynamicTableReporter(Http2BaseCompressor *aCompressor) : mCompressor(aCompressor) {}

Nit: exceeds 80 chars. Do this instead:

> HpackDynamicTableReporter(Http2BaseCompressor* aCompressor)
>   : mCompressor(aCompressor)
> {}

And note that the '*' should be on the left.

@@ +26,5 @@
> +
> +  HpackDynamicTableReporter(Http2BaseCompressor *aCompressor) : mCompressor(aCompressor) {}
> +
> +  NS_IMETHODIMP
> +  CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, bool anonymize);

Nit: exceeds 80 chars. I'd probably do this:

> NS_IMETHODIMP 
> CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, 
>                bool aAnonymize);

Note the 'a' prefix on |aAnonymize|.

@@ +31,5 @@
> +
> +private:
> +  MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)
> +
> +  ~HpackDynamicTableReporter() {}

A private destructor is unusual. Given that it's empty, I think just omitting it would be better.

@@ +33,5 @@
> +  MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)
> +
> +  ~HpackDynamicTableReporter() {}
> +
> +  Http2BaseCompressor *mCompressor; // the compressor being reported on

Nit: '*' on the left.

And the comment probably isn't necessary. It's the only field in a nsIMemoryReporter class, so it's pretty obvious it's being reported.

@@ +44,5 @@
> +
> +  HpackStaticTableReporter() {}
> +
> +  NS_IMETHODIMP
> +  CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, bool anonymize);

Same comment about line length and |anonymize| lacking the 'a' prefix.

@@ +49,5 @@
> +
> +private:
> +  MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)
> +
> +  ~HpackStaticTableReporter() {}

Same comment about the empty private destructor.

@@ +97,5 @@
>  class Http2BaseCompressor
>  {
>  public:
>    Http2BaseCompressor();
> +  virtual ~Http2BaseCompressor() { UnregisterStrongMemoryReporter(mDynamicReporter); };

Exceeds 80 chars. Do this instead.

> virtual ~Http2BaseCompressor()
> {
>   UnregisterStrongMemoryReporter(mDynamicReporter);
> }

And you can remove the trailing semi-colon (which I see was already present; you might as well remove it while you're modifying the line).

::: xpcom/base/nsIMemoryReporter.idl
@@ +460,5 @@
>  // reference to this reporter.
>  XPCOM_API(nsresult) RegisterStrongMemoryReporter(nsIMemoryReporter* aReporter);
>  
> +// Unregister a strong memory reporter
> +XPCOM_API(nsresult) UnregisterStrongMemoryReporter(nsIMemoryReporter* aReporter);

Can you please move it after RegisterWeakMemoryReporter(). That way all the register functions come before all the unregister functions.

Also, please add '.' at the end of the comment.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +2295,5 @@
>  }
>  
>  nsresult
> +UnregisterStrongMemoryReporter(nsIMemoryReporter* aReporter)
> +{

Ditto.

Comment 27

[nhughes]

(In reply to Nicholas Nethercote [:njn] from comment #26)
> Comment on attachment 8631205 [details] [diff] [review]
> hpack_table_reporting.diff
> 
> Review of attachment 8631205 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> @@ +237,5 @@
> >    : mOutput(nullptr)
> >    , mMaxBuffer(kDefaultMaxBuffer)
> >  {
> > +  mDynamicReporter = new HpackDynamicTableReporter(this);
> > +  RegisterStrongMemoryReporter(mDynamicReporter);
> 
> You added the UnregisterStrongReporter() function, but you forgot to use it
> :)  Call it in ~Http2BaseCompressor().
> 
> (My comment from last time still applies: "once this object is destroyed,
> the memory reporter manager will call the still-registered reporter, which
> will try to measure the (destroyed) object via a dangling pointer and you'll
> probably get a crash." Is the code running without crashing now? How many
> live Http2BaseCompressors do we have in practice?)

I haven't had any crashes. There seem to be anywhere between 0 and 20 Http2BaseCompressors, from what I've observed. 

I'm calling UnregisterStrongMemoryReporter() from ~Http2BaseCompressor() -- which in turn calls UnregisterStrongReporter(). Is that OK? I believe that's similar to the registration process.

Comment 28

[nhughes]

Here is the relevant output from about:memory for a test run:

15,632 B (00.01%) -- hpack
│      │   ├──14,512 B (00.01%) ── dynamic-tables [12]
│      │   └───1,120 B (00.00%) ── static-table

Conditions:
2 tabs open (in addition to about:memory)
- https://www.google.com/?gws_rd=ssl
- https://twitter.com/search?q=hey&src=typd

Comment 29

[nhughes]

(In reply to Nicholas Nethercote [:njn] from comment #26)
> Comment on attachment 8631205 [details] [diff] [review]
> hpack_table_reporting.diff
> 
> Review of attachment 8631205 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +31,5 @@
> > +
> > +private:
> > +  MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)
> > +
> > +  ~HpackDynamicTableReporter() {}
> 
> A private destructor is unusual. Given that it's empty, I think just
> omitting it would be better.

I agree; that's what I was attempting initially. Here is the build output I get when omitting the private destructor for the dynamic reporter (the same error occurs from ommiting the static reporter's destructor):

 0:16.68 In file included from /Users/nhughes/Desktop/firefox/obj-firefox/netwerk/protocol/http/Unified_cpp_protocol_http0.cpp:11:
 0:16.68 /Users/nhughes/Desktop/firefox/netwerk/protocol/http/Http2Compression.cpp:29:1: error: static_assert failed "Reference-counted class HpackDynamicTableReporter should not have a public destructor. Try to make this class's destructor non-public. If that is really not possible, you can whitelist this class by providing a HasDangerousPublicDestructor specialization for it."
 0:16.68 NS_IMPL_ISUPPORTS(HpackDynamicTableReporter, nsIMemoryReporter)
 0:16.69 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:16.69 ../../../dist/include/nsISupportsImpl.h:1049:3: note: expanded from macro 'NS_IMPL_ISUPPORTS'
 0:16.69   NS_IMPL_ADDREF(aClass)                                                      \
 0:16.69   ^~~~~~~~~~~~~~~~~~~~~~
 0:16.69 ../../../dist/include/nsISupportsImpl.h:583:3: note: expanded from macro 'NS_IMPL_ADDREF'
 0:16.69   MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING(_class)                                  \
 0:16.69   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:16.69 ../../../dist/include/nsISupportsImpl.h:90:3: note: expanded from macro 'MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING'
 0:16.69   static_assert(!MOZ_IS_DESTRUCTIBLE(X) || \
 0:16.69   ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Comment 30

[Nicholas Nethercote [inactive]]

> I'm calling UnregisterStrongMemoryReporter() from ~Http2BaseCompressor() --
> which in turn calls UnregisterStrongReporter(). Is that OK? I believe that's
> similar to the registration process.

Oh, I managed to completely miss that. Sorry! It looks good.

> "Reference-counted class HpackDynamicTableReporter should not have a public destructor. Try to make this class's destructor non-public. If that is really not possible, you can whitelist this class by providing a HasDangerousPublicDestructor specialization for it."

Fair enough! That's a very clear message. I wonder if this would work...

> ~HpackDynamicTableReporter() = delete;

If it does, that's arguably better, because it's clearer that we don't want a destructor. I'll let you decide which way to go on that one.

The lesson here is: reviewers are fallible. Because they haven't been writing and compiling the code they won't have as detailed an understanding of certain things. So push back and/or explain if they've said something is wrong. As you did in both cases above :)

Comment 31

[Nicholas Nethercote [inactive]]

(In reply to nhughes from comment #28)
> Here is the relevant output from about:memory for a test run:
> 
> 15,632 B (00.01%) -- hpack
> │      │   ├──14,512 B (00.01%) ── dynamic-tables [12]
> │      │   └───1,120 B (00.00%) ── static-table
> 
> Conditions:
> 2 tabs open (in addition to about:memory)
> - https://www.google.com/?gws_rd=ssl
> - https://twitter.com/search?q=hey&src=typd

Two things.

- The sizes here are small. Are there workloads where these sizes are likely to get higher?

- Is there any way to distinguish dynamic tables? E.g. a URL? (I don't see anything like that in Http2BaseCompressor.) If so, is it worth doing that? Maybe not, I don't know.

hurley, what do you think?

Comment 32

[Nicholas Nethercote [inactive]]

Thinking some more: I think the definitions of HpackDynamicTableReporter and HpackStaticTableReporter can be moved from Http2Compression.h to Http2Compression.cpp -- no point exposing more declarations than you need to. You'll need a forward declaration for HpackDynamicTableReporter but I think that will suffice.

Comment 33

[nhughes]

(In reply to Nicholas Nethercote [:njn] from comment #32)
> Thinking some more: I think the definitions of HpackDynamicTableReporter and
> HpackStaticTableReporter can be moved from Http2Compression.h to
> Http2Compression.cpp -- no point exposing more declarations than you need
> to. You'll need a forward declaration for HpackDynamicTableReporter but I
> think that will suffice.

I think HpackDynamicTableReporter has to be defined before Http2BaseCompressor. I thought that a forward declaration would suffice, but the compiler doesn't agree. Moving the declaration of HpackStaticTableReporter into Http2Compression.cpp, however, does not cause an issue, since Http2Base compressor doesn't contain an HpackStaticTableReporter reference.

Comment 34

[nhughes]

Attachment: hpack_table_reporting.diff

Comment 35

[Nicholas Nethercote [inactive]]

Comment on attachment 8632995 [details] [diff] [review]
hpack_table_reporting.diff

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

Looking good. Getting really close now. One more time should do it :)

::: netwerk/protocol/http/Http2Compression.cpp
@@ +45,5 @@
> +
> +NS_IMPL_ISUPPORTS(HpackDynamicTableReporter, nsIMemoryReporter)
> +
> +NS_IMETHODIMP
> +HpackDynamicTableReporter::CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, bool aAnonymize)

Nit: line exceeds 80 chars. Please break it.

@@ +50,5 @@
> +{
> +  return MOZ_COLLECT_REPORT(
> +    "explicit/network/hpack/dynamic-tables", KIND_HEAP, UNITS_BYTES,
> +    mCompressor->SizeOfExcludingThis(MallocSizeOf),
> +    "Aggregate memory usage of HPACK dynamic tables.");

When you move HpackDynamicTableReporter into this file, please inline this function definition into the class declaration.

@@ +53,5 @@
> +    mCompressor->SizeOfExcludingThis(MallocSizeOf),
> +    "Aggregate memory usage of HPACK dynamic tables.");
> +}
> +
> +NS_IMPL_ISUPPORTS(HpackStaticTableReporter, nsIMemoryReporter)

Nit: I'd move this to just after the declaration of the HpackStaticTableReporter class.

@@ +57,5 @@
> +NS_IMPL_ISUPPORTS(HpackStaticTableReporter, nsIMemoryReporter)
> +
> +NS_IMETHODIMP
> +HpackStaticTableReporter::CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData, bool aAnonymize)
> +{

This method is short. Inline it into the class declaration.

::: netwerk/protocol/http/Http2Compression.h
@@ +21,5 @@
> +
> +class HpackDynamicTableReporter final : public nsIMemoryReporter
> +{
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

I just tried moving this class into the .cpp file. The only other change you need to make is to move the definition of ~Http2BaseCompressor() into the .cpp file. I think this is worth doing.

Comment 36

[nhughes]

Attachment: hpack_table_reporting.diff

Comment 37

[Nicholas Nethercote [inactive]]

Comment on attachment 8633611 [details] [diff] [review]
hpack_table_reporting.diff

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

Lovely! One tiny nit but I don't need to see the patch again; r=me. Thank you for your perserverance.

::: netwerk/protocol/http/Http2Compression.cpp
@@ +275,5 @@
> +}
> +
> +Http2BaseCompressor::~Http2BaseCompressor()
> +{
> +    UnregisterStrongMemoryReporter(mDynamicReporter);

Nit: two-space indent here.

Comment 38

[nhughes]

Attachment: hpack_table_reporting.diff

Comment 39

[u408661]

(In reply to Nicholas Nethercote [:njn] from comment #31)
> (In reply to nhughes from comment #28)
> > Here is the relevant output from about:memory for a test run:
> > 
> > 15,632 B (00.01%) -- hpack
> > │      │   ├──14,512 B (00.01%) ── dynamic-tables [12]
> > │      │   └───1,120 B (00.00%) ── static-table
> > 
> > Conditions:
> > 2 tabs open (in addition to about:memory)
> > - https://www.google.com/?gws_rd=ssl
> > - https://twitter.com/search?q=hey&src=typd
> 
> Two things.
> 
> - The sizes here are small. Are there workloads where these sizes are likely
> to get higher?

It's certainly possible, we don't really know (not a whole lot of h2 in the wild yet - at least in terms of connection count, Google and Twitter do cause a lot of h2 traffic in terms of bytes, though).

> - Is there any way to distinguish dynamic tables? E.g. a URL? (I don't see
> anything like that in Http2BaseCompressor.) If so, is it worth doing that?
> Maybe not, I don't know.

I'm sure it's possible, though I haven't thought through the best way we can do that. nhughes, care to file a follow-up bug to split it out (not by URI, but by something else - that's part of the "best way" I haven't figured out). No need to work on it yourself (unless you have some free cycles), but just to have it for future work.

Comment 40

[u408661]

(In reply to nhughes from comment #28)
> Here is the relevant output from about:memory for a test run:
> 
> 15,632 B (00.01%) -- hpack
> │      │   ├──14,512 B (00.01%) ── dynamic-tables [12]
> │      │   └───1,120 B (00.00%) ── static-table
> 
> Conditions:
> 2 tabs open (in addition to about:memory)
> - https://www.google.com/?gws_rd=ssl
> - https://twitter.com/search?q=hey&src=typd

So you say there are 2 tabs open, but (if I'm reading the output correctly) it looks like there are 12 dynamic tables? I would expect 4 at a first glance - compressor and decompressor for each of the tabs. Do we know offhand where those other 4 connections/8 tables are coming from? I'm not saying they shouldn't be there, but it does seem like there could be some missed connection coalescing opportunities going on that we might want to look into.

Comment 41

[u408661]

Comment on attachment 8634225 [details] [diff] [review]
hpack_table_reporting.diff

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

Really close! I think the next round will be it.

Also, a note for your final commit message - when you add the reviewers to the commit message, you'll have "r=hurley r=njn", since you needed r+ from both of us to land this (me for the necko bits, and njn for the memory reporter bits).

::: netwerk/protocol/http/Http2Compression.cpp
@@ +24,5 @@
>  namespace net {
>  
>  static nsDeque *gStaticHeaders = nullptr;
>  
> +class HpackStaticTableReporter final : public nsIMemoryReporter

Let's move the definitions for both this and HpackDynamicTableReporter above gStaticHeaders (but keep the StaticRefPtr below gStaticHeaders) - that way we don't have variable declarations mixed in with class definitions (which I find a bit harder to read)

@@ +288,5 @@
> +size_t
> +Http2BaseCompressor::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t size = 0;
> +  for (uint32_t i = 0; i < mHeaderTable.Length(); i++) {

This will multiply-count the static table (once for each dynamic table you report), you want something like:

for (uint32_t i = mHeaderTable.StaticLength(); i < mHeaderTable.Length(); ++i)

to count only the dynamic entries.

@@ +289,5 @@
> +Http2BaseCompressor::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t size = 0;
> +  for (uint32_t i = 0; i < mHeaderTable.Length(); i++) {
> +    size += mHeaderTable[i]->SizeOfExcludingThis(aMallocSizeOf);

You either need to make (and use) nvPair::SizeOfIncludingThis (which would return aMallocSizeOf(this) + what you have now), or add size += aMallocSizeOf(mHeaderTable[i]), as the nvPairs are dynamically allocated, and you're not counting that space anywhere.

Comment 42

[nhughes]

(In reply to Nicholas Hurley [:hurley, :nwgh] from comment #39)
> (In reply to Nicholas Nethercote [:njn] from comment #31)
> > (In reply to nhughes from comment #28)
> > > Here is the relevant output from about:memory for a test run:
> > > 
> > > 15,632 B (00.01%) -- hpack
> > > │      │   ├──14,512 B (00.01%) ── dynamic-tables [12]
> > > │      │   └───1,120 B (00.00%) ── static-table
> > > 
> > > Conditions:
> > > 2 tabs open (in addition to about:memory)
> > > - https://www.google.com/?gws_rd=ssl
> > > - https://twitter.com/search?q=hey&src=typd
> > 
> > Two things.
> > 
> > - The sizes here are small. Are there workloads where these sizes are likely
> > to get higher?
> 
> It's certainly possible, we don't really know (not a whole lot of h2 in the
> wild yet - at least in terms of connection count, Google and Twitter do
> cause a lot of h2 traffic in terms of bytes, though).
> 
> > - Is there any way to distinguish dynamic tables? E.g. a URL? (I don't see
> > anything like that in Http2BaseCompressor.) If so, is it worth doing that?
> > Maybe not, I don't know.
> 
> I'm sure it's possible, though I haven't thought through the best way we can
> do that. nhughes, care to file a follow-up bug to split it out (not by URI,
> but by something else - that's part of the "best way" I haven't figured
> out). No need to work on it yourself (unless you have some free cycles), but
> just to have it for future work.

Done. Pleat let me know if I should add any additional info about the bug (filed here):
https://bugzilla.mozilla.org/show_bug.cgi?id=1185078

Comment 43

[nhughes]

(In reply to Nicholas Hurley [:hurley, :nwgh] from comment #40)
> (In reply to nhughes from comment #28)
> > Here is the relevant output from about:memory for a test run:
> > 
> > 15,632 B (00.01%) -- hpack
> > │      │   ├──14,512 B (00.01%) ── dynamic-tables [12]
> > │      │   └───1,120 B (00.00%) ── static-table
> > 
> > Conditions:
> > 2 tabs open (in addition to about:memory)
> > - https://www.google.com/?gws_rd=ssl
> > - https://twitter.com/search?q=hey&src=typd
> 
> So you say there are 2 tabs open, but (if I'm reading the output correctly)
> it looks like there are 12 dynamic tables? I would expect 4 at a first
> glance - compressor and decompressor for each of the tabs. Do we know
> offhand where those other 4 connections/8 tables are coming from? I'm not
> saying they shouldn't be there, but it does seem like there could be some
> missed connection coalescing opportunities going on that we might want to
> look into.

Hm, I'm not sure offhand, but I'll investigate that and report back here.

Comment 44

[nhughes]

(In reply to Nicholas Hurley [:hurley, :nwgh] from comment #41)
> Comment on attachment 8634225 [details] [diff] [review]
> hpack_table_reporting.diff
> 
> Review of attachment 8634225 [details] [diff] [review]:
> -----------------------------------------------------------------
> Let's move the definitions for both this and HpackDynamicTableReporter above
> gStaticHeaders (but keep the StaticRefPtr below gStaticHeaders) - that way
> we don't have variable declarations mixed in with class definitions (which I
> find a bit harder to read)

HpackStaticTableReporter::CollectReports() references gStaticHeaders so I can't define HpackStaticTableReporter above that pointer. I moved the StaticRefPtr under the definition of HpackDynamicTableReporter -- I think that makes it a bit easier to read (at least the memory reporting classes are defined in succession). Please let me know if there's anything else you think I can do to make the code more readable :)

Comment 45

[nhughes]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32225ff53f7c

Comment 46

[nhughes]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7eea94915ed

Comment 47

[nhughes]

Attachment: hpack_table_reporting.diff

Nick - I was hoping you could review when you have a minute. Thanks!

Comment 48

[nhughes]

Attachment: hpack_table_reporting.diff

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b90b21e0c64

Comment 50

[Nicholas Nethercote [inactive]]

Congratulations, nhughes! :)

Comment 51

[Carsten Book [:Tomcat]]

sorry had to back this out for static build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11933698&repo=mozilla-inbound

Comment 52

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9039d4bdd28c

Comment 53

[nhughes]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c90f9d5c7a6

Comment 54

[nhughes]

Attachment: hpack_table_reporting.diff

Comment 55

[nhughes]

Attachment: hpack_table_reporting.diff

Comment 56

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/794dbff8502b

Comment 57

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/794dbff8502b