Nathan Froyd [:froydnj] Reporter
	Description • 8 years ago

DMD says this for my current browser session:

  45,718 blocks in heap block record 1 of 14,432
  187,243,072 bytes (94,037,318 requested / 93,205,754 slop)
  Individual block sizes: 8,192 x 30; 4,096 x 45,651; 1,024; 272 x 36
  19.73% of the heap (19.73% cumulative)
  30.72% of unreported (30.72% cumulative)
  Allocated at {
    #01: replace_malloc (/home/froydnj/firefox/libdmd.so)
    #02: malloc[firefox/firefox +0x131ee]
    #03: nsStringBuffer::Alloc(unsigned long) (/home/froydnj/firefox/libxul.so)
    #04: nsACString_internal::MutatePrep(unsigned int, char**, unsigned int*) (/home/froydnj/firefox/libxul.so)
    #05: nsACString_internal::SetCapacity(unsigned int, mozilla::fallible_t const&) (/home/froydnj/firefox/libxul.so)
    #06: nsACString_internal::SetCapacity(unsigned int) (/home/froydnj/firefox/libxul.so)
    #07: nsACString_internal::SetLength(unsigned int) (/home/froydnj/firefox/libxul.so)
    #08: IPC::ParamTraits<nsACString_internal>::Read(IPC::Message const*, PickleIterator*, nsACString_internal*) (/home/froydnj/firefox/libxul.so)
    #09: mozilla::dom::PContentParent::Read(mozilla::ipc::StringInputStreamParams*, IPC::Message const*, PickleIterator*) (/home/froydnj/firefox/libxul.so)
    #10: mozilla::dom::PContentParent::Read(mozilla::ipc::InputStreamParams*, IPC::Message const*, PickleIterator*) (/home/froydnj/firefox/libxul.so)
    #11: mozilla::dom::PContentParent::Read(mozilla::ipc::InputStreamParamsWithFds*, IPC::Message const*, PickleIterator*) (/home/froydnj/firefox/libxul.so)
    #12: mozilla::dom::PContentParent::Read(mozilla::ipc::IPCStream*, IPC::Message const*, PickleIterator*) (/home/froydnj/firefox/libxul.so)
    #13: mozilla::dom::PContentParent::Read(mozilla::dom::BlobDataStream*, IPC::Message const*, PickleIterator*) (/home/froydnj/firefox/libxul.so)
    #14: mozilla::dom::PContentParent::Read(mozilla::dom::BlobData*, IPC::Message const*, PickleIterator*) (/home/froydnj/firefox/libxul.so)
    #15: mozilla::dom::PContentParent::Read(mozilla::dom::OptionalBlobData*, IPC::Message const*, PickleIterator*) (/home/froydnj/firefox/libxul.so)
    #16: mozilla::dom::PContentParent::Read(mozilla::dom::NormalBlobConstructorParams*, IPC::Message const*, PickleIterator*) (/home/froydnj/firefox/libxul.so)
    #17: mozilla::dom::PContentParent::Read(mozilla::dom::AnyBlobConstructorParams*, IPC::Message const*, PickleIterator*) (/home/froydnj/firefox/libxul.so)
    #18: mozilla::dom::PContentParent::Read(mozilla::dom::ParentBlobConstructorParams*, IPC::Message const*, PickleIterator*) (/home/froydnj/firefox/libxul.so)
    #19: mozilla::dom::PContentParent::Read(mozilla::dom::BlobConstructorParams*, IPC::Message const*, PickleIterator*) (/home/froydnj/firefox/libxul.so)
    #20: mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) (/home/froydnj/firefox/libxul.so)
    #21: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (/home/froydnj/firefox/libxul.so)
    #22: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (/home/froydnj/firefox/libxul.so)
    #23: mozilla::ipc::MessageChannel::OnMaybeDequeueOne() (/home/froydnj/firefox/libxul.so)
    #24: mozilla::detail::RunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run() (/home/froydnj/firefox/libxul.so)
  }

Not sure who's holding on to these blobs, but it would be great if they were reported somehow.

	Nathan Froyd [:froydnj] Reporter
	Comment 1 • 8 years ago

I don't see any Blobs in the GC or CC logs, either. :(

	Andrea Marchesini [:baku] Assignee
	Comment 2 • 8 years ago

With this patch we have a memory reporter for BlobImplStream.
I'm going to file a bug for having the same for BlobImplString as well.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 8 years ago

Comment on attachment 8805916 [details] [diff] [review]
blob_memory_report.patch


>+    MOZ_COLLECT_REPORT(
>+      "explicit/dom/memory-file-data/small",
>+      KIND_HEAP, UNITS_BYTES, size,
>+      "Memory used to back a File/Blob based on an input stream. "
>+      "Probably this is a Memory Blob sent by the child process to "
>+      "the parent process.");
The latter sentence look wrong. This particular case is more common with WebSocket created blobs, as far as I see.
So, just drop the "Probably this is..." sentence.
But I'm not familiar enough with memory reporting. I have no idea whether the same path can be used for many things.
We already do have some use for explicit/dom/memory-file-data/small, but with different description.
To be more clear what is using the memory, perhaps all the explicit/dom/memory-file-data/small should have some similar stuff as what 'large' has.
Some (type=<something telling about the type of the blobimpl>).  Or just use different path here?


>+
>+    return NS_OK;
>+  }
>+
>+private:
>+  ~MemoryReporter()
>+  {}
>+
>+  // Raw pointer because this class is kept alive by the BlobImplStream.
>+  BlobImplStream* mBlobImpl;
Please add a Disconnect() method or some such which explicitly clears this pointer when 
BlobImplStream::~BlobImplStream() is called and mMemoryReporter is set.

> BlobImplStream::~BlobImplStream()
>-{}
>+{
>+  if (mMemoryReporter) {
>+    UnregisterStrongMemoryReporter(mMemoryReporter);
so you should also call Disconnect() here.


f+ with those fixed, but this must get a review from someone familiar with memory reporter. njn perhaps?

	Nicholas Nethercote [inactive]
	Comment 5 • 8 years ago

Comment on attachment 8805934 [details] [diff] [review]
blob_memory_report.patch

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

Thank you for writing this new reporter! You're heading in the right direction but I have some comments below and I'd like to see a revised patch before landing.

::: dom/base/File.cpp
@@ +1249,5 @@
> +      return NS_OK;
> +    }
> +
> +    uint64_t size;
> +    nsresult rv = mBlobImpl->mInputStream->Available(&size);

How does Available() work -- does it end up using nsIInputStream::available()? Generally it's best in memory reporters to measure things with a function generated with the MOZ_DEFINE_MALLOC_SIZE_OF macro -- it's better to measure the memory usage directly rather than computing its size analytically.
(https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Memory_reporting has some explanation why.)

Is it possible to do that in this case?

@@ +1257,5 @@
> +
> +    MOZ_COLLECT_REPORT(
> +      "explicit/dom/memory-file-data/small",
> +      KIND_HEAP, UNITS_BYTES, size,
> +      "Memory used to back a File/Blob based on an input stream.");

We already have a reporter that uses the path "explicit/dom/memory-file-data/small" but it has a different description. So we could end up with multiple measurements with the same path, which about:memory handles (it just adds them together). That is reasonable when they all have the same description but seems odd when they don't. I don't know much about blobs so I can't tell if the new and old measurements should just share the same description, or if the new measurements should get a different path.

Also, that existing path is paired with "explicit/dom/memory-file-data/large/*", which explicitly lists large blobs. Can the blobs you are measuring in this patch be large? If so, a small/large split like the existing one would be useful.

::: dom/base/File.h
@@ +864,4 @@
>    nsCOMPtr<nsIInputStream> mInputStream;
> +
> +  class MemoryReporter;
> +  RefPtr<MemoryReporter> mMemoryReporter;

Instead of BlobImplStream having an nsIMemoryReporter, can it implement the nsIMemoryReporter interface itself and (weakly) register/unregister itself? Experience has shown that is a less error-prone pattern because you don't have to worry about the mismatches between the lifetime of the reporter and the lifetime of the object being reported. (No need for null checks, either.)

See nsScriptNameSpaceManager for an existing example.

	Andrea Marchesini [:baku] Assignee
	Comment 6 • 8 years ago

> Is it possible to do that in this case?

At the moment, this is not possible because nsIStringInputStream is not a builtinclass.
We can change that, probably.

	Andrea Marchesini [:baku] Assignee
	Comment 7 • 8 years ago

All the other comments applied.

	Andrea Marchesini [:baku] Assignee
	Comment 9 • 8 years ago

Comment on attachment 8806241 [details] [diff] [review]
blob_memory_report.patch

Actually this approach doesn't work because I cannot register the object in the CTOR: the refcount value is == 0 and we have CrashIfRefcountIsZero() to prevent this.

I think the separate class sounds good. Feedback?

	Nicholas Nethercote [inactive]
	Comment 10 • 8 years ago

Comment on attachment 8806241 [details] [diff] [review]
blob_memory_report.patch

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

> Actually this approach doesn't work because I cannot register the object in the CTOR: the refcount value is == 0 and we have CrashIfRefcountIsZero() to prevent this.
> 
> I think the separate class sounds good. Feedback?

Looking at other examples, they all use an Init() function and register within that. I'd prefer doing that -- it's simpler than having a separate class. You could rename MaybeRegisterMemoryReporter() as Init().

::: dom/base/File.cpp
@@ +1240,3 @@
>  {
>    mImmutable = true;
> +  MaybeRegisterMemoryReporter();

With the separate Init() function, you wouldn't call MaybeRegisterMemoryReporter() here.

@@ +1267,5 @@
>  BlobImplStream::~BlobImplStream()
> +{
> +  if (mMemoryReporterRegistered) {
> +    UnregisterWeakMemoryReporter(this);
> +  }

You can just unconditionally call UnregisterWeakMemoryReporter() because nothing bad happens if it fails. And then you can remove mMemoryReporterRegistered.

@@ +1322,5 @@
> +BlobImplStream::CollectReports(nsIHandleReportCallback* aHandleReport,
> +                               nsISupports* aData, bool aAnonymize)
> +{
> +  uint64_t size;
> +  nsresult rv = mInputStream->Available(&size);

You didn't answer my question from the previous review about Available().

	Nicholas Nethercote [inactive]
	Comment 12 • 8 years ago

Comment on attachment 8806609 [details] [diff] [review]
blob_memory_report.patch

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

> Looking at other examples, they all use an Init() function and register
> within that. I'd prefer doing that -- it's simpler than having a separate
> class. You could rename MaybeRegisterMemoryReporter() as Init().

Thank you for addressing this comment via the new Create() functions.
 
> You can just unconditionally call UnregisterWeakMemoryReporter() because
> nothing bad happens if it fails. And then you can remove
> mMemoryReporterRegistered.

You haven't addressed this comment.
 
> You didn't answer my question from the previous review about Available().

You still haven't answered my question.

	Andrea Marchesini [:baku] Assignee
	Comment 13 • 8 years ago

> > You didn't answer my question from the previous review about Available().
> 
> You still haven't answered my question.

I answered in comment 6. nsIStringInputStream is not a builtinclass and it doesn't expose the internal string member.
I don't think we can do much more than Available.

We could make nsIStringInputStream a builtinclass as a follow up: it seems that no addons implement this interface.

	Nicholas Nethercote [inactive]
	Comment 14 • 8 years ago

Comment on attachment 8806609 [details] [diff] [review]
blob_memory_report.patch

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

Sorry I missed comment 6. That's unfortunate, because it means the memory is reported but DMD will still think it's unreported.

r=me with the unnecessary mMemoryReporterRegistered field removed.

Thanks again for adding this reporter.

	Pulsebot
	Comment 15 • 8 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/176e40639a6c
BlobImplStream implements nsIMemoryReporter, r=njn

	Phil Ringnalda (:philor)
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/176e40639a6c