Bug 65556 - JSC GC uses dummy cells to avoid having to remember which cells it has already destroyed
Summary: JSC GC uses dummy cells to avoid having to remember which cells it has alread...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-02 11:57 PDT by Filip Pizlo
Modified: 2011-08-02 14:22 PDT (History)
3 users (show)

See Also:


Attachments
the patch (19.81 KB, patch)
2011-08-02 12:04 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (19.73 KB, patch)
2011-08-02 12:52 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (19.85 KB, patch)
2011-08-02 13:17 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.