Bug 142941

Summary:

GCTimer should work correctly when invoked multiple times during the same collection

Product:

WebKit

Reporter:

Mark Hahnenberg <mhahnenb>

Component:

JavaScriptCore

Assignee:

Mark Hahnenberg <mhahnenb>

Status:

NEW

Severity:

Normal

CC:

fpizlo, ggaren, kling, mark.lam

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

140774

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	mark.lam: review+

Mark Hahnenberg

Reported 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.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Mark Hahnenberg

Comment 1 2015-03-21 17:40:32 PDT

Created attachment 249181 [details] Patch

Mark Hahnenberg

Comment 2 2015-03-22 13:38:54 PDT

Created attachment 249207 [details] Patch

Geoffrey Garen

Comment 3 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.

Mark Hahnenberg

Comment 4 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!? :-)

Geoffrey Garen

Comment 5 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 :).

Mark Hahnenberg

Comment 6 2015-03-28 17:02:32 PDT

Created attachment 249674 [details] Patch

Mark Hahnenberg

Comment 7 2015-03-28 17:03:18 PDT

Split GCTimer and friends out into separate files and moved them over to normal classes w/ accessors.

Mark Hahnenberg

Comment 8 2015-03-28 17:22:13 PDT

Created attachment 249675 [details] Patch

Mark Lam

Comment 9 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.

Note You need to log in before you can comment on or make changes to this bug.