Bug 165911

Summary: Get rid of HeapRootVisitor and make SlotVisitor less painful to use
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, cdumez, commit-queue, keith_miller, mark.lam, msaboff, ossy, ryanhaddad, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 165910    
Attachments:
Description Flags
it's a start
none
a bit more
none
more
none
the patch ggaren: review+

Filip Pizlo
Reported 2016-12-15 12:21:09 PST
SlotVisitor has a bunch of stuff left over from ancient attempts to make our GC be fully copying. We're all-in with the non-copying religion now, so we might as well get rid of the heresy.
Attachments
it's a start (19.65 KB, patch)
2016-12-15 12:28 PST, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
a bit more (111.58 KB, patch)
2016-12-15 13:28 PST, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
more (132.73 KB, patch)
2016-12-15 14:19 PST, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (133.24 KB, patch)
2016-12-15 15:08 PST, Filip Pizlo 		ggaren: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2016-12-15 12:28:23 PST
Created attachment 297209 [details] it's a start
Filip Pizlo
Comment 2 2016-12-15 13:28:31 PST
Created attachment 297215 [details] a bit more
Filip Pizlo
Comment 3 2016-12-15 14:19:32 PST
Created attachment 297223 [details] more
Filip Pizlo
Comment 4 2016-12-15 15:08:23 PST
Created attachment 297229 [details] the patch
WebKit Commit Bot
Comment 5 2016-12-15 15:10:58 PST
This patch modifies the JS builtins code generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-builtins-generator-tests --reset-results`)
WebKit Commit Bot
Comment 6 2016-12-15 15:11:18 PST
Attachment 297229 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/SlotVisitor.h:92: The parameter name "weak" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 113 files If any of these errors are false positives, please file a bug against check-webkit-style.
Geoffrey Garen
Comment 7 2016-12-15 15:17:51 PST
Comment on attachment 297229 [details] the patch r=me I don't really love "appendTrusted". We trust all our pointers to be valid pointers. Maybe "appendWithoutBarrierCheck"?
Filip Pizlo
Comment 8 2016-12-15 15:26:19 PST
(In reply to comment #7) > Comment on attachment 297229 [details] > the patch > > r=me > > I don't really love "appendTrusted". We trust all our pointers to be valid > pointers. > > Maybe "appendWithoutBarrierCheck"? We call this a lot so I was hoping for something short. appendUnbarriered?
Blaze Burg
Comment 9 2016-12-15 16:01:29 PST
Comment on attachment 297229 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=297229&action=review > Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:208 > +#define VISIT_FUNCTION(name) visitor.append(m_##name##Function); You'll need to rebaseline, run-builtins-generator-tests --reset-results
Filip Pizlo
Comment 10 2016-12-15 18:15:09 PST
(In reply to comment #9) > Comment on attachment 297229 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297229&action=review > > > Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:208 > > +#define VISIT_FUNCTION(name) visitor.append(m_##name##Function); > > You'll need to rebaseline, run-builtins-generator-tests --reset-results Done!
Filip Pizlo
Comment 11 2016-12-15 18:17:25 PST
Landed in https://trac.webkit.org/changeset/209897
Csaba Osztrogonác
Comment 12 2016-12-16 03:26:50 PST
(In reply to comment #11) > Landed in https://trac.webkit.org/changeset/209897 It broke the bindings generation tests, see build.webkit.org for details.
Ryan Haddad
Comment 13 2016-12-16 10:52:17 PST
Rebaselined bindings tests in http://trac.webkit.org/projects/webkit/changeset/209925
Filip Pizlo
Comment 14 2016-12-16 11:06:31 PST
(In reply to comment #13) > Rebaselined bindings tests in > http://trac.webkit.org/projects/webkit/changeset/209925 Thank you!
Note You need to log in before you can comment on or make changes to this bug.