Bug 65556

Summary: JSC GC uses dummy cells to avoid having to remember which cells it has already destroyed
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, oliver, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch
none
the patch
none
the patch none

Description Filip Pizlo 2011-08-02 11:57:52 PDT
The JSC GC attempts to maintain the invariant that every cell within a marked block has a C++ object that inherits from JSCell, so that anytime a sweep is performed, destructors can be called on all free cells even if this was done already.  This requires infrastructure for "dummy" JSCells, with "dummy" structures, just to allow the garbage collector to get around having to do bookkeeping about which parts of memory have already been swept.  Dummy cells should be replaced with a collector-internal mechanism for tracking which memory has already been swept.
Comment 1 Filip Pizlo 2011-08-02 12:04:46 PDT
Created attachment 102674 [details]
the patch
Comment 2 WebKit Review Bot 2011-08-02 12:07:35 PDT
Attachment 102674 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/heap/MarkedBlock.cpp:217:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/heap/MarkedBlock.h:173:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Filip Pizlo 2011-08-02 12:52:18 PDT
Created attachment 102685 [details]
the patch
Comment 4 Oliver Hunt 2011-08-02 13:06:52 PDT
Comment on attachment 102685 [details]
the patch

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

> Source/JavaScriptCore/heap/MarkedBlock.h:77
>              FreeCell* next;
> +            
> +            void setNoObject()
> +            {
> +                *reinterpret_cast<void**>(this) = 0;
> +            }

Why this cast void**?  FreeCell doesn't have a vtable pointer, you're simply assigning null to this->next?
Comment 5 Filip Pizlo 2011-08-02 13:12:26 PDT
(In reply to comment #4)
> (From update of attachment 102685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102685&action=review
> 
> > Source/JavaScriptCore/heap/MarkedBlock.h:77
> >              FreeCell* next;
> > +            
> > +            void setNoObject()
> > +            {
> > +                *reinterpret_cast<void**>(this) = 0;
> > +            }
> 
> Why this cast void**?  FreeCell doesn't have a vtable pointer, you're simply assigning null to this->next?

I did that to emphasize the fact that I'm setting what-would-have-been-a-vtable to 0.  I suppose a comment would do the same job, so I'll post a fix.
Comment 6 Filip Pizlo 2011-08-02 13:17:53 PDT
Created attachment 102690 [details]
the patch
Comment 7 WebKit Review Bot 2011-08-02 14:22:51 PDT
Comment on attachment 102690 [details]
the patch

Clearing flags on attachment: 102690

Committed r92233: <http://trac.webkit.org/changeset/92233>
Comment 8 WebKit Review Bot 2011-08-02 14:22:56 PDT
All reviewed patches have been landed.  Closing bug.