Bug 142941 - GCTimer should work correctly when invoked multiple times during the same collection
Summary: GCTimer should work correctly when invoked multiple times during the same col...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks: 140774
  Show dependency treegraph
 
Reported: 2015-03-21 17:24 PDT by Mark Hahnenberg
Modified: 2015-03-31 10:12 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.19 KB, patch)
2015-03-21 17:40 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (10.80 KB, patch)
2015-03-22 13:38 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (32.09 KB, patch)
2015-03-28 17:02 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (32.14 KB, patch)
2015-03-28 17:22 PDT, Mark Hahnenberg
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2015-03-21 17:24:28 PDT
It would be nice to be able to record more fine-grained GC phases during phases that can occur multiple times per collection. For example, visiting live weak handles consists of a number of sub-phases that can be repeated until nothing changes. The current GCTimer implementation doesn't handle this correctly.
Comment 1 Mark Hahnenberg 2015-03-21 17:40:32 PDT
Created attachment 249181 [details]
Patch
Comment 2 Mark Hahnenberg 2015-03-22 13:38:54 PDT
Created attachment 249207 [details]
Patch
Comment 3 Geoffrey Garen 2015-03-23 11:00:41 PDT
Comment on attachment 249207 [details]
Patch

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

r=me

> Source/JavaScriptCore/heap/Heap.cpp:83
> +struct TimeRecord {

Can you make this a real class in its own file?

I think we're supposed to use m_ naming for fields.
Comment 4 Mark Hahnenberg 2015-03-23 11:30:52 PDT
(In reply to comment #3)
> Comment on attachment 249207 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249207&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/heap/Heap.cpp:83
> > +struct TimeRecord {
> 
> Can you make this a real class in its own file?
Sure, will do.

> 
> I think we're supposed to use m_ naming for fields.

Darin requested no "m_" for struct members on a previous patch dealing with GCTimer. Who do I believe!? :-)
Comment 5 Geoffrey Garen 2015-03-23 13:33:11 PDT
I don't know what the common style is for structs, but if you make it a class, I know what the style is :).
Comment 6 Mark Hahnenberg 2015-03-28 17:02:32 PDT
Created attachment 249674 [details]
Patch
Comment 7 Mark Hahnenberg 2015-03-28 17:03:18 PDT
Split GCTimer and friends out into separate files and moved them over to normal classes w/ accessors.
Comment 8 Mark Hahnenberg 2015-03-28 17:22:13 PDT
Created attachment 249675 [details]
Patch
Comment 9 Mark Lam 2015-03-31 10:12:34 PDT
Comment on attachment 249675 [details]
Patch

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

r=me

> Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:1806
> -</Project>
>  \ No newline at end of file
> +</Project>

Can you fix this?  Seems like an unintended change.

> Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:4433
> -</Project>
>  \ No newline at end of file
> +</Project>

Another seemingly unintentional change.