Bug 55423 – Clean up property tables in Structure

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Barraclough]

Encapsulate, reduce duplication of table search code, and reduce the size of the tables (remove the index, just maintain the tables in the correct order).

[Gavin Barraclough]

Created attachment 84145 [details]
The patch

[WebKit Review Bot]

Attachment 84145 [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/Structure.cpp:67:  Should have a space between // and comment  [whitespace/comments] [4]
Source/JavaScriptCore/runtime/Structure.cpp:71:  Should have a space between // and comment  [whitespace/comments] [4]
Source/JavaScriptCore/runtime/Structure.cpp:170:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/PropertyMapHashTable.h:417:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 84145 [details]
The patch

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

Please consider moving implementations to inline functions out of the class definition and moving the power of 2 math to StdLibExtras.h in a follow up patch.

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:177
> +

An assertion that initialCapacity is >= other.size would be good.

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:373
> +    // we knoe capacity to be available.

Typo. Knoe!

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:467
> +    Vector<unsigned>* m_deletedOffsets;

Can this be an OwnPtr?

>> Source/JavaScriptCore/runtime/Structure.cpp:67
>> +//static const int smallMapThreshold = 1024;
> 
> Should have a space between // and comment  [whitespace/comments] [4]

Please delete.

>> Source/JavaScriptCore/runtime/Structure.cpp:71
>> +//static const unsigned tinyMapThreshold = 20;
> 
> Should have a space between // and comment  [whitespace/comments] [4]

Please delete.

[Darin Adler]

Comment on attachment 84145 [details]
The patch

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

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:44
> +#define PROPERTY_MAP_DELETED_ENTRY_KEY ((StringImpl*)1) 

Does this really need to be a macro instead of a const?

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:136
> +    PropertyTable(unsigned initialCapacity)

I think this class would be easier to read if the code wasn’t all inside the class definition. It would be nice to have a class definition with just function declarations, and all the function definitions separate after the class.

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:464
> +    unsigned* m_index;

Can we use OwnArrayPtr and new for this instead of fastZeroedMalloc?

>> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:467
>> +    Vector<unsigned>* m_deletedOffsets;
> 
> Can this be an OwnPtr?

Can we use OwnPtr for this?

>>> Source/JavaScriptCore/runtime/Structure.cpp:67
>>> +//static const int smallMapThreshold = 1024;
>> 
>> Should have a space between // and comment  [whitespace/comments] [4]
> 
> Please delete.

You should remove these commented-out constants unless there is some reason to keep them.

>>> Source/JavaScriptCore/runtime/Structure.cpp:71
>>> +//static const unsigned tinyMapThreshold = 20;
>> 
>> Should have a space between // and comment  [whitespace/comments] [4]
> 
> Please delete.

Like the stylebot says!

> Source/JavaScriptCore/runtime/Structure.h:107
> +        size_t get(StringImpl* rep, unsigned& attributes, JSCell*& specificValue);

I don’t think the argument name “rep” here helps at all. Should it be named propertyName instead?

> Source/JavaScriptCore/runtime/Structure.h:161
> +        PropertyTable* copyPropertyTable();

Can this return a PassOwnPtr?

> Source/JavaScriptCore/runtime/Structure.h:199
> +        PropertyTable* m_propertyTable;

Can this be an OwnPtr?

> Source/JavaScriptCore/runtime/Structure.h:231
> +        return entry ? entry->offset : WTF::notFound;

Do you really need the WTF prefix here?

[Build Bot]

Attachment 84145 [details] did not build on win:
Build output: http://queues.webkit.org/results/8073522

[Gavin Barraclough]

(In reply to comment #4)
> (From update of attachment 84145 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84145&action=review
> 
> > Source/JavaScriptCore/runtime/PropertyMapHashTable.h:44
> > +#define PROPERTY_MAP_DELETED_ENTRY_KEY ((StringImpl*)1) 

I don't think we can initialize a static const of a non-integral type in this fashion - would have to go in the cpp.

> > Source/JavaScriptCore/runtime/PropertyMapHashTable.h:464
> > +    unsigned* m_index;
> 
> Can we use OwnArrayPtr and new for this instead of fastZeroedMalloc?

This was going to be a bit weird - bear in mind the this allocation isn't just an array of unsigneds, the same allocation holds the value array.  Depending on the size of the values an packing this might theoretically not even have a size that is a multiple of sizeof(unsigned)! - so I've decided to hold off on this for now, though it might be doable.

All other changes have been made. Fixed in r79963.