Bug 22269 – Reduce PropertyMap usage

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Sam Weinig]

Patch forthcoming.

[Sam Weinig]

Created attachment 25174 [details]
patch

[Darin Adler]

Comment on attachment 25174 [details]
patch

> +    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

[Sam Weinig]

Landed in r38407.