Bug 159569 - Dumping the object graph doesn't work with verbose GC logging
Summary: Dumping the object graph doesn't work with verbose GC logging
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-07-08 11:13 PDT by Michael Saboff
Modified: 2016-07-08 11:57 PDT (History)
5 users (show)

See Also:

Attachments
Patch (4.13 KB, patch)
2016-07-08 11:21 PDT, Michael Saboff 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2016-07-08 11:13:10 PDT 
When the logGC option is set to verbose (2), the logging code attempts to dump the object graph.  That code doesn't work.  It is built to visit the object graph again and that method it fraught with difficulties.  Therefore the object graph dumping part of verbose GC logging should be removed.
Comment 1 Michael Saboff 2016-07-08 11:21:05 PDT 
Created attachment 283184 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2016-07-08 11:21:51 PDT 
<rdar://problem/27249686>
Comment 3 Mark Lam 2016-07-08 11:30:02 PDT 
Comment on attachment 283184 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283184&action=review

> Source/JavaScriptCore/ChangeLog:11
> +        The current object graph logging code tries to revisits the graph.  This doesn't work
> +        correctly and asking around it isn't used.  The only way to dump the true object graph
> +        is to log while we GC and that has obvious performance implications.
> +        Therefore I eliminated GCLogging::dumpObjectGraph() and related code.  

Then, you should also remove the verbose option from Options::logGC.
Comment 4 Michael Saboff 2016-07-08 11:32:43 PDT 
(In reply to comment #3)
> Comment on attachment 283184 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283184&action=review
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        The current object graph logging code tries to revisits the graph.  This doesn't work
> > +        correctly and asking around it isn't used.  The only way to dump the true object graph
> > +        is to log while we GC and that has obvious performance implications.
> > +        Therefore I eliminated GCLogging::dumpObjectGraph() and related code.  
> 
> Then, you should also remove the verbose option from Options::logGC.

We still need the verbose option, which will show all of the roots.  This patch only removes the dump object graph code.  I still think that logging the roots is useful.
Comment 5 Mark Lam 2016-07-08 11:33:00 PDT 
Comment on attachment 283184 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283184&action=review

r=me

>> Source/JavaScriptCore/ChangeLog:11
>> +        Therefore I eliminated GCLogging::dumpObjectGraph() and related code.  
> 
> Then, you should also remove the verbose option from Options::logGC.

My mistake: the verbose option is still in use.
Comment 6 WebKit Commit Bot 2016-07-08 11:57:10 PDT 
Comment on attachment 283184 [details]
Patch

Clearing flags on attachment: 283184

Committed r203000: <http://trac.webkit.org/changeset/203000>
Comment 7 WebKit Commit Bot 2016-07-08 11:57:15 PDT 
All reviewed patches have been landed.  Closing bug.