[webkit-reviews] review granted: [Bug 22269] Reduce PropertyMap usage : [Attachment 25174] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 22269: Reduce PropertyMap usage
https://bugs.webkit.org/show_bug.cgi?id=22269

Attachment 25174: patch
https://bugs.webkit.org/attachment.cgi?id=25174&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    RefPtr<StructureID> structureID =
StructureID::addPropertyTransitionToExistingStructure(m_structureID,
propertyName, attributes, offset);
> +    if (structureID) {

It would make the long line even longer, but this would be a great case for
putting the assignment into the if statement to make it clear you can't use
this outside the if statement. Also is this a rare enough case that it should
go into a separate function? Or maybe a separate inline function just for code
clarity?

> -    RefPtr<StructureID> structureID =
StructureID::addPropertyTransition(m_structureID, propertyName, attributes,
offset);
> +    structureID = StructureID::addPropertyTransition(m_structureID,
propertyName, attributes, offset);

If you put the structureID into the if then you wouldn't have to change this
line, and in fact it would be ever so slightly more efficient because you'd
save one branch and deref.

> +    // Taken  from http://www.cs.utk.edu/~vose/c-stuff/bithacks.html

Extra space here in the comment.

> +    // Search for the last StructureID with a property table 

Please use periods if you use capitals.

> +	       rehashPropertyMapHashTable(sizeForKeyCount(m_offset + 1)); //
This could be made more efficient by combinding with the copy above 

Ditto. Also misspells combining.

> +    for (int i = structures.size() - 2; i >= 0; --i) {

ptrdiff_t instead of int? I can never figure out what we should do about 64 vs.
32 bit.

> +	   structure->m_nameInPrevious->ref();
> +	   PropertyMapEntry entry(structure->m_nameInPrevious,
structure->m_offset, structure->m_attributesInPrevious,
++m_propertyTable->lastIndexUsed); 
> +	   insertIntoPropertyMapHashTable(entry);

I wish there was a way to put that ref call somewhere lower level so it was
more obvious it was balanced by deref.

> +    // deleted offsets vector) before transistioning from dictionary. 

Spelling error in the word "transitioning".

Does this get thoroughly enough tested by existing tests, or do we need new
tests?

r=me