972799 - Show file-base and memory-base blob-url report in about:memory

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

In bug 970801, People may be concerned about the memory-base blob. We need to separate the file-base and memory-base blob in about:memory.

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: show file-base and memory-base blob in report

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Kyle, I still show both file-base and memory-base blob in the report.
I think we need to know all type of blob in a app.

Does the nsDOMMemoryFile be the only one type of memory-base blob?
http://dxr.mozilla.org/mozilla-central/source/content/base/public/nsDOMFile.h#357

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8376158 [details] [diff] [review]
show file-base and memory-base blob in report

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

This looks good, but it doesn't fix the issue of counting the size of file-based blobs towards memory usage.

::: content/base/public/nsDOMFile.h
@@ +382,5 @@
>  
> +  NS_IMETHOD_(bool) IsMemoryFile(void) MOZ_OVERRIDE
> +  {
> +    return true;
> +  }

Move the definition into the C++ file too.

::: content/base/public/nsHostObjectProtocolHandler.h
@@ +56,5 @@
>    static void Traverse(const nsACString& aUri, nsCycleCollectionTraversalCallback& aCallback);
> +
> +private:
> +  static void Init(void);
> +  static bool initialized;

sInitialized please.

::: content/base/public/nsIDOMFile.idl
@@ +63,1 @@
>  };

You need to change the uuid for nsIDOMBlob and everything that inherits from it (nsIDOMFile in this case) because you changed the interface.

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +177,5 @@
>        if (NS_FAILED(blob->GetSize(&size))) {
>          size = 0;
>        }
>  
> +      path = (blob->IsMemoryFile()) ? "memory-blob-urls/" : "file-blob-urls/";

This is fine, but you still haven't fixed the size issue.

@@ +233,5 @@
>  NS_IMPL_ISUPPORTS1(BlobURLsReporter, nsIMemoryReporter)
>  
>  }
>  
> +bool nsHostObjectProtocolHandler::initialized = false;

No need to do this, static data is initialized to zero.

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)
> Comment on attachment 8376158 [details] [diff] [review]
> show file-base and memory-base blob in report
> 
> Review of attachment 8376158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, but it doesn't fix the issue of counting the size of
> file-based blobs towards memory usage.
> 

The report will like:

9.92 MB (100.0%) -- file-blob-urls
└──9.92 MB (100.0%) ++ owner(app://gallery.gaiamobile.org/index.html)
   ├──0.00 MB (00.05%) ── blob:0027984b-4209-4198-9cc6-b1aa3954c2d5
   ├──0.00 MB (00.05%) ── blob:0070362d-3c69-4762-91a9-0c1d2424db60
   ...............
   ├──0.00 MB (00.05%) ── blob:03f49825-b5ee-4e6f-9b9b-f686e2ea0373


1.92 MB (100.0%) -- memory-blob-urls
└──1.92 MB (100.0%) ++ owner(app://gallery.gaiamobile.org/index.html)
   ├──0.00 MB (00.05%) ── blob:0027984b-4209-4198-9cc6-b1aa3954c2d5
   ├──0.00 MB (00.05%) ── blob:0070362d-3c69-4762-91a9-0c1d2424db60
   ...............
   ├──0.00 MB (00.05%) ── blob:03f49825-b5ee-4e6f-9b9b-f686e2ea0373


I separate file-based blobs into different sheet.
Do we also count the blob size in other position?

Comment 5

[Nicholas Nethercote [inactive]]

> 9.92 MB (100.0%) -- file-blob-urls
> └──9.92 MB (100.0%) ++ owner(app://gallery.gaiamobile.org/index.html)
>    ├──0.00 MB (00.05%) ── blob:0027984b-4209-4198-9cc6-b1aa3954c2d5
>    ├──0.00 MB (00.05%) ── blob:0070362d-3c69-4762-91a9-0c1d2424db60
>    ...............
>    ├──0.00 MB (00.05%) ── blob:03f49825-b5ee-4e6f-9b9b-f686e2ea0373

Looks like you  must have lots of entries for them to add up to 9.92 MB. We don't really want to clog up about:memory with thousands of tiny, uninteresting entries. Something done in various other places is to have some kind of threshold -- things larger than that threshold get reported individually, and then all the things smaller than the threshold get aggregated into a single entry. For example, see the "sundries" values for JS code, and "non-notable" strings.

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: show file-base and memory-base blob in report. v2

The report will like:

9.92 MB (100.0%) -- file-blob-urls
└──9.92 MB (100.0%) ++ owner(app://gallery.gaiamobile.org/index.html)
   ├──0.00 MB (00.05%) ── blob:0027984b-4209-4198-9cc6-b1aa3954c2d5
   ├──0.00 MB (00.05%) ── blob:0070362d-3c69-4762-91a9-0c1d2424db60
   ...............
   ├──0.00 MB (00.05%) ── blob:03f49825-b5ee-4e6f-9b9b-f686e2ea0373

3 (100.0%) -- file-blob-urls
└──3 (100.0%) -- owner(app://system.gaiamobile.org/index.html)
   ├──1 (33.33%) ── blob:6007bccc-85a2-4049-aa1e-1aa23bc2c4a4
   ├──1 (33.33%) ── blob:b795f8d4-ce7d-4d9c-bc3c-0f4ed15afdc2
   └──1 (33.33%) ── blob:d8d73bad-03b2-42f6-8368-4af547121c32

The file-blob-urls will only show the count number, not size.

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)
> @@ +233,5 @@
> >  NS_IMPL_ISUPPORTS1(BlobURLsReporter, nsIMemoryReporter)
> >  
> >  }
> >  
> > +bool nsHostObjectProtocolHandler::initialized = false;
> 
> No need to do this, static data is initialized to zero.

I still prefer to set the init value precisely.
Like http://dxr.mozilla.org/mozilla-central/source/accessible/src/base/nsEventShell.cpp#66

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8382179 [details] [diff] [review]
show file-base and memory-base blob in report.  v2

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

> 9.92 MB (100.0%) -- file-blob-urls
> └──9.92 MB (100.0%) ++ owner(app://gallery.gaiamobile.org/index.html)
>    ├──0.00 MB (00.05%) ── blob:0027984b-4209-4198-9cc6-b1aa3954c2d5
>    ├──0.00 MB (00.05%) ── blob:0070362d-3c69-4762-91a9-0c1d2424db60
>    ...............
>    ├──0.00 MB (00.05%) ── blob:03f49825-b5ee-4e6f-9b9b-f686e2ea0373

From the code it looks like this should be the "memory-blob-urls" tree, not the "file-blob-urls" tree. Something's weird.

Also, like I said in comment 5, spamming about:memory with trees containing 2000+ uninteresting entries isn't good. Please introduce a threshold (I've used 16 KiB elsewhere) and aggregate entries below that threshold. I'm giving r- based on that alone.

Comment 9

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> Comment on attachment 8382179 [details] [diff] [review]
> show file-base and memory-base blob in report.  v2
> 
> Review of attachment 8382179 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > 9.92 MB (100.0%) -- file-blob-urls
> > └──9.92 MB (100.0%) ++ owner(app://gallery.gaiamobile.org/index.html)
> >    ├──0.00 MB (00.05%) ── blob:0027984b-4209-4198-9cc6-b1aa3954c2d5
> >    ├──0.00 MB (00.05%) ── blob:0070362d-3c69-4762-91a9-0c1d2424db60
> >    ...............
> >    ├──0.00 MB (00.05%) ── blob:03f49825-b5ee-4e6f-9b9b-f686e2ea0373
> 
> From the code it looks like this should be the "memory-blob-urls" tree, not
> the "file-blob-urls" tree. Something's weird.

It just a example, not a real memory report. This section will be memory-blob-urls tree.
This is my mistake for text copy and paste.

> Also, like I said in comment 5, spamming about:memory with trees containing
> 2000+ uninteresting entries isn't good. Please introduce a threshold (I've
> used 16 KiB elsewhere) and aggregate entries below that threshold. I'm
> giving r- based on that alone.

Why we don't show all blob entries? If we have a lot of blob; 2000 blob in this case; we want to make sure that each blob memory usage is correct. The report will not unroll until we click the item(like the "tiny" item below). The memory report's page will not be very long until we unroll it.

20.13 MB (100.0%) -- memory-blob-urls
├──19.62 MB (97.46%) ── test1 [230]
└───0.51 MB (02.54%) ++ (6 tiny)

If we aggregate the blobs which size are under the threshold, the report will like:

20.13 MB (100.0%) -- memory-blob-urls
├──19.62 MB (97.46%) ── sundries(2000 items)
└───0.51 MB (02.54%) ++ (6 tiny)
    ├──0.09 MB (00.42%) ── test2
    ├──0.09 MB (00.42%) ── test3
    ├──0.09 MB (00.42%) ── test4
    ├──0.09 MB (00.42%) ── test5
    ├──0.09 MB (00.42%) ── test6
    └──0.09 MB (00.42%) ── test7

Even though each entry is small, but the total sundries are very large.
We can't get more info about it in this case.
We can't know the blob address, and it's hard to debug in gaia if we don't have the blob address.

I use this blob address info to reduce the gallery memory usage.
These blob info have used in a real case, and we also list all blob(no size threshold) in current implementation. Should we change it?

Comment 10

[Nicholas Nethercote [inactive]]

Did you need to see every blob individually to fix the gallery? Or did you just need to know that there were a lot of them, and their total size was significant?

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> Did you need to see every blob individually to fix the gallery? Or did you
> just need to know that there were a lot of them, and their total size was
> significant?

I have used the individual infos.
This is how I found the gallery memory problem:
1. we see a lot of blob entries in memory report. It is not normal.
2. choose 1 blob's address and find where we load this blob in gaia.
3. then, we see that we have some problem for blob usage.

So, I prefer to show all info.

Comment 12

[Nicholas Nethercote [inactive]]

I sometimes use verbose debugging printfs to identify problems, but I don't commit them.

But fine, do the ultra-verbose option. If it turns out to be a problem, we can always modify it again later.

Comment 13

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Nicholas, could you review this patch again?

Comment 14

[Nicholas Nethercote [inactive]]

Comment on attachment 8382179 [details] [diff] [review]
show file-base and memory-base blob in report.  v2

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

::: content/base/public/nsDOMFile.h
@@ +379,5 @@
>    }
>  
>    NS_IMETHOD GetInternalStream(nsIInputStream**) MOZ_OVERRIDE;
>  
> +  NS_IMETHOD_(bool) IsMemoryFile(void) MOZ_OVERRIDE;

Does this need to be declared in the IDL interface, or could it just be done at the C++ level? The latter would be preferable, if it's possible.

::: content/base/public/nsIDOMFile.idl
@@ +57,5 @@
>    // Called before the blob is stored in a database to decide if it can be
>    // shared or needs to be copied. It can be called on any thread.
>    [notxpcom] FileInfo getFileInfo(in FileManager aFileManager);
> +
> +  // Return true if this blob is memory file.

Change "is" to "is a".

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +183,3 @@
>        }
>  
> +      path = (isMemoryFile) ? "memory-blob-urls/" : "file-blob-urls/";

No need for parens around isMemoryFile.

@@ +229,5 @@
> +            UNITS_BYTES,
> +            size / refCount,
> +            (specialDesc.IsEmpty()
> +             ? static_cast<const nsACString&>(desc)
> +             : static_cast<const nsACString&>(specialDesc)),

Hoist this ternary expression outside the |if| so it doesn't need to be repeated.

@@ +260,3 @@
>  {
> +  if (!sInitialized) {
> +    sInitialized = true;

sInitialized doesn't need to be a class member. You can revert it to being a local variable within this function.

Comment 15

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Nicholas Nethercote [:njn] from comment #14)
> Comment on attachment 8382179 [details] [diff] [review]
> show file-base and memory-base blob in report.  v2
> 
> Review of attachment 8382179 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/public/nsDOMFile.h
> @@ +379,5 @@
> >    }
> >  
> >    NS_IMETHOD GetInternalStream(nsIInputStream**) MOZ_OVERRIDE;
> >  
> > +  NS_IMETHOD_(bool) IsMemoryFile(void) MOZ_OVERRIDE;
> 
> Does this need to be declared in the IDL interface, or could it just be done
> at the C++ level? The latter would be preferable, if it's possible.

The interface "isMemoryFile()" is just called by c++ level, so I will change the interface to:
[noscript, notxpcom] bool isMemoryFile();

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

notxpcom implies noscript.  You don't have to specify it explicitly.

Comment 17

[Nicholas Nethercote [inactive]]

> The interface "isMemoryFile()" is just called by c++ level

My question is: do you need to modify the IDL? No point exposing that function if it's not necessary.

Comment 18

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Nicholas Nethercote [:njn] from comment #17)
> > The interface "isMemoryFile()" is just called by c++ level
> 
> My question is: do you need to modify the IDL? No point exposing that
> function if it's not necessary.

We get the blob object here:
http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsHostObjectProtocolHandler.cpp#161

The type is nsIDOMBlob, so I change the nsIDOMBlob idl interface.

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: show file-base and memory-base blob in report. v3. r=n.nethercote

update patch for comment 14 and comment 16.

Comment 20

[Nicholas Nethercote [inactive]]

Comment on attachment 8384444 [details] [diff] [review]
show file-base and memory-base blob in report. v3. r=n.nethercote

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

::: content/base/public/nsIDOMFile.idl
@@ +63,3 @@
>  };
>  
> +[scriptable, builtinclass, uuid(4e7d1a8b-e2d5-4304-a753-4affb731660c)]

You don't need to change the uuid of this interface, because it's unchanged in this patch.

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Nicholas Nethercote [:njn] from comment #20)
> Comment on attachment 8384444 [details] [diff] [review]
> show file-base and memory-base blob in report. v3. r=n.nethercote
> Review of attachment 8384444 [details] [diff] [review]:
> ----------------------------------------------------------------
> ::: content/base/public/nsIDOMFile.idl
> @@ +63,3 @@
> >  };
> >  
> > +[scriptable, builtinclass, uuid(4e7d1a8b-e2d5-4304-a753-4affb731660c)]
> 
> You don't need to change the uuid of this interface, because it's unchanged
> in this patch.

The nsIDOMFile inherits from nsIDOMBlob. I change the nsIDOMBlob interface.
Don't we need to change the uuid for nsIDOMFile? See comment 3.

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)
> ::: content/base/public/nsIDOMFile.idl
> @@ +63,1 @@
> >  };
> You need to change the uuid for nsIDOMBlob and everything that inherits from
> it (nsIDOMFile in this case) because you changed the interface.

Comment 22

[Nicholas Nethercote [inactive]]

Ok, fine, ignore my comment.

Comment 23

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try result:
https://tbpl.mozilla.org/?tree=Try&rev=59d823831c52

Please land the attachment 8384444 [details] [diff] [review] to mozilla-central. Thanks.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06eb9d02fd2a

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/06eb9d02fd2a