tracing: Report ResourceClients and the reason for not deletable for web-caches

tracing: Report ResourceClients and the reason for not deletable for web-caches

This CL adds
- Resource::reasonNotDeletable() (mostly corresponds to canDelete())
- ResourceClient::debugName() and its overrides
that are used in Resource::onMemoryDump().

This CL utilizes already-defined LayoutObject::debugName(), and makes it to be
an override of ResourceClient::debugName().

BUG=549445
Committed: https://crrev.com/2a3f374c912c0ea77d488821d7b67b50a1fe84a7
Cr-Commit-Position: refs/heads/master@{#358735}

[hiroshige, 2015-10-29 09:37:01]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-29 09:37:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413233005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413233005/80001

[commit-bot: I haz the power, 2015-10-29 09:40:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-29 09:40:24]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hiroshige, 2015-10-30 05:51:12]

Patchset #6 (id:100001) has been deleted

[hiroshige, 2015-10-30 05:54:48]

Patchset #6 (id:120001) has been deleted

[hiroshige, 2015-10-30 05:54:59]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-30 05:55:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413233005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413233005/140001

[hiroshige, 2015-10-30 05:58:03]

Description was changed from

==========
Report why canDelete() returns false and its ResourceClients
==========

to

==========
Show Resource's ResourceClients and the reason for not deletable in memory-infra
==========

[hiroshige, 2015-10-30 06:10:04]

Description was changed from

==========
Show Resource's ResourceClients and the reason for not deletable in memory-infra
==========

to

==========
tracing: Report ResourceClients and the reason for not deletable for web-caches

BUG=549445
==========

[hiroshige, 2015-10-30 06:15:32]

Description was changed from

==========
tracing: Report ResourceClients and the reason for not deletable for web-caches

BUG=549445
==========

to

==========
tracing: Report ResourceClients and the reason for not deletable for web-caches

This CL adds
- Resource::reasonNotDeletable() (mostly corresponds to canDelete())
- ResourceClient::debugName() and its overrides
that are used in Resource::onMemoryDump().

BUG=549445
==========

[hiroshige, 2015-10-30 06:30:46]

hiroshige@chromium.org changed reviewers:
+ bashi@chromium.org, haraken@chromium.org, ssid@chromium.org

[hiroshige, 2015-10-30 06:30:46]

PTAL.

Changes other than in Resource.h/Resource.cpp are adding debugName() overrides.

Screenshot is included in:
https://drive.google.com/a/google.com/file/d/0BzNRZJOK3W0HRVFZZmUxc20wcXc/vie...

[bashi, 2015-10-30 06:40:06]

bashi@chromium.org changed reviewers:
+ primiano@chromium.org

[bashi, 2015-10-30 06:45:49]

Thanks for the work, hiroshige-san!

+primiano

I'm not sure putting this kind of detailed information in allocator detailed
pane. I'd like to have some feedback from memory-infra team before moving
forward.

[bashi, 2015-10-30 06:48:05]

On 2015/10/30 06:45:49, bashi1 wrote:
> Thanks for the work, hiroshige-san!
> 
> +primiano
> 
> I'm not sure putting this kind of detailed information in allocator detailed
^^
I wanted to say "I'm not sure it's a good idea to put..."

> pane. I'd like to have some feedback from memory-infra team before moving
> forward.

[ssid, 2015-10-30 07:00:31]

I think it is fine to have this information if it is useful. It could be added
in json format to be consistent with the tracing. But I am not sure if inserting
json string in dump provider causes other issues. 
Also, the strings should be made more meaningful for people who read the trace
but do not know about memoryCache. 
Lets see what primiano@ thinks.

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:543:
builder.append("m_loader");
Can we get rid of m_ and display more useful info for common user. Maybe
"loader_exists"?

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:548:
builder.append("m_preloadCount(");
preloaded_with_count or just preloaded, and similar names below..

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:569:
builder.append("memoryCache");
reason_not_deletable : memoryCache -> in_memory_cache?

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:815:
dump->addString("not_deletable_because", "", reasonNotDeletable());
I think it is better to show "reason_not_deletable" or something like that,
instead of open phrase "not_deletable_because". What if that column is empty?

[hiroshige, 2015-10-30 07:34:32]

hiroshige@chromium.org changed reviewers:
+ loading-reviews@chromium.org, tyoshino@chromium.org

[hiroshige, 2015-10-30 07:34:33]

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:543:
builder.append("m_loader");
On 2015/10/30 07:00:31, ssid wrote:
> Can we get rid of m_ and display more useful info for common user. Maybe
> "loader_exists"?

I made the names to directly corresponding to the variable names and
canDelete(), because I think
- it is clearer for Chromium developers
- it is hard to make the description fully understandable for "common users" --
Resource lifetime management is quite complex, and anyway requires reading
source code to understand it.

One possible way is to use class names where applicable:
  hasClients -> ResourceClient
  m_handleCount -> ResourcePtr
  m_protectorCount -> InternalResourcePtr
which will make the names and the column name of "ResourceClient" (L840)
consistent.

Added +loading-reviews@.
How do you think? Any good names?

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:569:
builder.append("memoryCache");
On 2015/10/30 07:00:31, ssid wrote:
> reason_not_deletable : memoryCache -> in_memory_cache?

Acknowledged.

[commit-bot: I haz the power, 2015-10-30 08:19:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-30 08:19:05]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2015-10-30 10:15:52]

primiano@chromium.org changed reviewers:
+ petrcermak@chromium.org

[Primiano Tucci (use gerrit), 2015-10-30 10:15:53]

On 2015/10/30 08:19:05, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Let me take a look applying this patch locally.
Starts looking like what you really want is a TRACE_EVENT_OBJECT_SNAPSHOT where
that structure is a JSON and a small UI support in trace viewer to interpret
this.
Let me take a look with our TraceViewer UI guru (+petrcermak@) and see what we
can take out of the hat.

[kouhei (in TOK), 2015-11-02 04:16:28]

kouhei@chromium.org changed reviewers:
+ kouhei@chromium.org

[kouhei (in TOK), 2015-11-02 04:16:28]

lgtm % nits

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp (right):

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp:139: String
debugName() const override { return "TestScriptResourceClient"; }
Maybe const char*

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClient.h (right):

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClient.h:57: virtual String
debugName() const = 0;
resourceClientDebugName?

[hiroshige, 2015-11-02 05:21:29]

Description was changed from

==========
tracing: Report ResourceClients and the reason for not deletable for web-caches

This CL adds
- Resource::reasonNotDeletable() (mostly corresponds to canDelete())
- ResourceClient::debugName() and its overrides
that are used in Resource::onMemoryDump().

BUG=549445
==========

to

==========
tracing: Report ResourceClients and the reason for not deletable for web-caches

This CL adds
- Resource::reasonNotDeletable() (mostly corresponds to canDelete())
- ResourceClient::debugName() and its overrides
that are used in Resource::onMemoryDump().

This CL utilizes already-defined LayoutObject::debugName(), and makes it to be
an override of ResourceClient::debugName().

BUG=549445
==========

[hiroshige, 2015-11-02 05:22:07]

Updated CL description to mention LayoutObject::debugName().

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp (right):

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp:139: String
debugName() const override { return "TestScriptResourceClient"; }
On 2015/11/02 04:16:28, kouhei wrote:
> Maybe const char*

Mostly const char* but LayoutObject::debugName() need to be String.

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClient.h (right):

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClient.h:57: virtual String
debugName() const = 0;
On 2015/11/02 04:16:28, kouhei wrote:
> resourceClientDebugName?
String LayoutObject::debugName() const is already defined and this CL uses it
for LayoutObject and its subclasses.
So naming this debugName() is more consistent I think.

[kouhei (in TOK), 2015-11-02 07:05:03]

On 2015/11/02 05:22:07, hiroshige wrote:
> Updated CL description to mention LayoutObject::debugName().
> 
>
https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp
(right):
> 
>
https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp:139: String
> debugName() const override { return "TestScriptResourceClient"; }
> On 2015/11/02 04:16:28, kouhei wrote:
> > Maybe const char*
> 
> Mostly const char* but LayoutObject::debugName() need to be String.
> 
>
https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/fetch/ResourceClient.h (right):
> 
>
https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/fetch/ResourceClient.h:57: virtual String
> debugName() const = 0;
> On 2015/11/02 04:16:28, kouhei wrote:
> > resourceClientDebugName?
> String LayoutObject::debugName() const is already defined and this CL uses it
> for LayoutObject and its subclasses.
> So naming this debugName() is more consistent I think.

Fair enough.

[kinuko, 2015-11-02 07:41:57]

Non-owner fyi comment

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:543:
builder.append("m_loader");
On 2015/10/30 07:34:33, hiroshige wrote:
> On 2015/10/30 07:00:31, ssid wrote:
> > Can we get rid of m_ and display more useful info for common user. Maybe
> > "loader_exists"?
> 
> I made the names to directly corresponding to the variable names and
> canDelete(), because I think
> - it is clearer for Chromium developers
> - it is hard to make the description fully understandable for "common users"
--
> Resource lifetime management is quite complex, and anyway requires reading
> source code to understand it.
> 
> One possible way is to use class names where applicable:
>   hasClients -> ResourceClient
>   m_handleCount -> ResourcePtr
>   m_protectorCount -> InternalResourcePtr
> which will make the names and the column name of "ResourceClient" (L840)
> consistent.
> 
> Added +loading-reviews@.
> How do you think? Any good names?

Either looks fine with me given that it seems difficult anyway for 'common
users' to understand the meaning of these values. If we're going to show this as
something like TRACE_EVENT_OBJECT_SNAPSHOT with json UI support that sounds good
to me too.

[Primiano Tucci (use gerrit), 2015-11-02 17:14:01]

OK I applied the patch and took a look.
This is effectively less invasive than what I was thinking.
If this is enough to cover what you need LGTM.
But if we want to do something fancier in the future (object relationships and
such) we should find a better way to formalize it.

On an unrelated note I've just realized two things on the URL (which is part of
a prior cl already landed).
- We could make the url to be type:"url" instead of type:"string" and tech the
UI to make the links clckable
- I see lot of data:font/open-type;base64,a1b2c3d4... I think we should truncate
the base64 here as IMHO that is just wasting trace space.

[Nate Chapin, 2015-11-02 18:36:50]

japhet@chromium.org changed reviewers:
+ japhet@chromium.org

[Nate Chapin, 2015-11-02 18:36:51]

FYI, I'm planning on substantially simplifying Resource lifetime soon. Once
https://chromium.googlesource.com/chromium/src/+/aadbd2624c5003698624cf2e21fd...
has baked for a little longer and I'm sure it's not going to be reverted, it
should be straightforward to remove ResourcePtr and the other special cases for
Resource lifetimes, including deleting canDelete().

[ssid, 2015-11-04 12:12:57]

LGTM, sorry for the delay. Thanks for adding this.

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:543:
builder.append("m_loader");
On 2015/11/02 07:41:57, kinuko wrote:
> On 2015/10/30 07:34:33, hiroshige wrote:
> > On 2015/10/30 07:00:31, ssid wrote:
> > > Can we get rid of m_ and display more useful info for common user. Maybe
> > > "loader_exists"?
> > 
> > I made the names to directly corresponding to the variable names and
> > canDelete(), because I think
> > - it is clearer for Chromium developers
> > - it is hard to make the description fully understandable for "common users"
> --
> > Resource lifetime management is quite complex, and anyway requires reading
> > source code to understand it.
> > 
> > One possible way is to use class names where applicable:
> >   hasClients -> ResourceClient
> >   m_handleCount -> ResourcePtr
> >   m_protectorCount -> InternalResourcePtr
> > which will make the names and the column name of "ResourceClient" (L840)
> > consistent.
> > 
> > Added +loading-reviews@.
> > How do you think? Any good names?
> 
> Either looks fine with me given that it seems difficult anyway for 'common
> users' to understand the meaning of these values. If we're going to show this
as
> something like TRACE_EVENT_OBJECT_SNAPSHOT with json UI support that sounds
good
> to me too.

Sorry for the late reply. It is fine as it is, since it is difficult for others
to understand anyway and more useful for developers.

[hiroshige, 2015-11-09 18:04:18]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-09 18:05:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413233005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413233005/160001

[commit-bot: I haz the power, 2015-11-09 20:45:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-09 20:45:04]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2015-11-09 20:53:58]

Reflected comments.

haraken@, could you take a look as a core/web/bindings OWNER?

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:569:
builder.append("memoryCache");
On 2015/10/30 07:00:31, ssid wrote:
> reason_not_deletable : memoryCache -> in_memory_cache?

Done.

https://codereview.chromium.org/1413233005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:815:
dump->addString("not_deletable_because", "", reasonNotDeletable());
On 2015/10/30 07:00:31, ssid wrote:
> I think it is better to show "reason_not_deletable" or something like that,
> instead of open phrase "not_deletable_because". What if that column is empty?

Done.

[haraken, 2015-11-09 23:30:20]

LGTM

https://codereview.chromium.org/1413233005/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1413233005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:809:
ResourceClientWalker<ResourceClient> w(m_clients);

w => walker

https://codereview.chromium.org/1413233005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:810: while (ResourceClient* c
= w.next())

c => client

[hiroshige, 2015-11-09 23:43:40]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-09 23:44:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413233005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413233005/180001

[hiroshige, 2015-11-10 00:11:53]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2015-11-10 00:12:11]

https://codereview.chromium.org/1413233005/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1413233005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:809:
ResourceClientWalker<ResourceClient> w(m_clients);
On 2015/11/09 23:30:20, haraken wrote:
> 
> w => walker

Done.

https://codereview.chromium.org/1413233005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:810: while (ResourceClient* c
= w.next())
On 2015/11/09 23:30:20, haraken wrote:
> 
> c => client

Done.

[hiroshige, 2015-11-10 00:12:20]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2015-11-10 00:12:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, ssid@chromium.org, kouhei@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1413233005/#ps180001
(title: "Reflected haraken's comments.")

[commit-bot: I haz the power, 2015-11-10 00:13:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413233005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413233005/180001

[commit-bot: I haz the power, 2015-11-10 01:47:52]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-11-10 01:48:58]

Patchset 8 (id:??) landed as
https://crrev.com/2a3f374c912c0ea77d488821d7b67b50a1fe84a7
Cr-Commit-Position: refs/heads/master@{#358735}