Bug 54029 - Give each MarkedBlock enough mark bits to cover the whole block
Summary: Give each MarkedBlock enough mark bits to cover the whole block
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-08 13:16 PST by Geoffrey Garen
Modified: 2011-02-08 15:03 PST (History)
1 user (show)

See Also:


Attachments
Patch (6.88 KB, patch)
2011-02-08 13:22 PST, Geoffrey Garen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2011-02-08 13:16:50 PST
Give each MarkedBlock enough mark bits to cover the whole block
Comment 1 Geoffrey Garen 2011-02-08 13:22:28 PST
Created attachment 81681 [details]
Patch
Comment 2 WebKit Review Bot 2011-02-08 13:24:27 PST
Attachment 81681 [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/runtime/MarkedBlock.h:46:  BITS_PER_BLOCK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/runtime/MarkedBlock.h:47:  CELLS_PER_BLOCK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2011-02-08 13:29:48 PST
Comment on attachment 81681 [details]
Patch

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

>> Source/JavaScriptCore/runtime/MarkedBlock.h:46
>> +    const size_t BITS_PER_BLOCK = BLOCK_SIZE / CELL_SIZE;
> 
> BITS_PER_BLOCK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

Sure would be nice to fix these constants at some point.

> Source/JavaScriptCore/runtime/MarkedBlock.h:65
> -        size_t cellNumber(const JSCell*);
> -        bool isMarked(const JSCell*);
> -        bool testAndSetMarked(const JSCell*);
> -        void setMarked(const JSCell*);
> +        size_t cellNumber(const void*);
> +        bool isMarked(const void*);
> +        bool testAndSetMarked(const void*);
> +        void setMarked(const void*);

Why this change? Change log does not say why.

> Source/JavaScriptCore/runtime/MarkedBlock.h:112
>      inline bool MarkedBlock::isPossibleCell(const void* p)
>      {
> -        return isCellAligned(p) && p;
> +        return isCellAligned(p);
>      }

Do we even need an isPossibleCell function any more? Maybe calling isCellAligned directly would be better?
Comment 4 Geoffrey Garen 2011-02-08 14:56:42 PST
> >> Source/JavaScriptCore/runtime/MarkedBlock.h:46
> >> +    const size_t BITS_PER_BLOCK = BLOCK_SIZE / CELL_SIZE;
> > 
> > BITS_PER_BLOCK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> 
> Sure would be nice to fix these constants at some point.

Indeed. I've been ignoring them for now because in the near future most of these constants will be gone.

> > Source/JavaScriptCore/runtime/MarkedBlock.h:65
> > -        size_t cellNumber(const JSCell*);
> > -        bool isMarked(const JSCell*);
> > -        bool testAndSetMarked(const JSCell*);
> > -        void setMarked(const JSCell*);
> > +        size_t cellNumber(const void*);
> > +        bool isMarked(const void*);
> > +        bool testAndSetMarked(const void*);
> > +        void setMarked(const void*);
> 
> Why this change? Change log does not say why.

I'll add a comment.

> > Source/JavaScriptCore/runtime/MarkedBlock.h:112
> >      inline bool MarkedBlock::isPossibleCell(const void* p)
> >      {
> > -        return isCellAligned(p) && p;
> > +        return isCellAligned(p);
> >      }
> 
> Do we even need an isPossibleCell function any more? Maybe calling isCellAligned directly would be better?

Fixed in final patch.
Comment 5 Geoffrey Garen 2011-02-08 15:03:16 PST
Committed r77977: <http://trac.webkit.org/changeset/77977>