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+

Description Filip Pizlo 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.
Comment 1 Filip Pizlo 2016-12-15 12:28:23 PST 
Created attachment 297209 [details]
it's a start
Comment 2 Filip Pizlo 2016-12-15 13:28:31 PST 
Created attachment 297215 [details]
a bit more
Comment 3 Filip Pizlo 2016-12-15 14:19:32 PST 
Created attachment 297223 [details]
more
Comment 4 Filip Pizlo 2016-12-15 15:08:23 PST 
Created attachment 297229 [details]
the patch
Comment 5 WebKit Commit Bot 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`)
Comment 6 WebKit Commit Bot 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.
Comment 7 Geoffrey Garen 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"?
Comment 8 Filip Pizlo 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?
Comment 9 BJ Burg 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
Comment 10 Filip Pizlo 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!
Comment 11 Filip Pizlo 2016-12-15 18:17:25 PST 
Landed in https://trac.webkit.org/changeset/209897
Comment 12 Csaba Osztrogonác 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.
Comment 13 Ryan Haddad 2016-12-16 10:52:17 PST 
Rebaselined bindings tests in http://trac.webkit.org/projects/webkit/changeset/209925
Comment 14 Filip Pizlo 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!