Created attachment 199095 [details] the patch

Comment on attachment 199095 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=199095&action=review > Source/JavaScriptCore/ChangeLog:73 > +2013-04-22 Filip Pizlo <fpizlo@apple.com> > + > + * bytecode/GetByIdStatus.cpp: > + (JSC::GetByIdStatus::computeFromLLInt): > + (JSC::GetByIdStatus::computeForChain): > + (JSC::GetByIdStatus::computeFor): > + * bytecode/PutByIdStatus.cpp: > + (JSC::PutByIdStatus::computeFromLLInt): > + (JSC::PutByIdStatus::computeFor): > + * runtime/PropertyMapHashTable.h: > + (PropertyTable): > + (JSC::PropertyTable::findConcurrently): > + (JSC): > + (JSC::PropertyTable::add): > + (JSC::PropertyTable::remove): > + (JSC::PropertyTable::reinsert): > + (JSC::PropertyTable::rehash): > + * runtime/PropertyTable.cpp: > + (JSC::PropertyTable::PropertyTable): > + * runtime/Structure.cpp: > + (JSC::Structure::findStructuresAndMapForMaterialization): > + (JSC::Structure::getConcurrently): > + * runtime/Structure.h: Duplicate changelog > Source/JavaScriptCore/runtime/PropertyMapHashTable.h:366 > + Locker locker(m_lock); So add() locks, what about delete? Do we need that to lock? > Source/WTF/wtf/Locker.h:43 > + Locker(AssumeLockedTag) > + : m_lockable(0) > + { > + } I wonder if there's someway we could add an assertion to make sure this is the case

Comment on attachment 199095 [details] the patch As discussed in person, there is a race here between the compiler doing a lookup and the mutator stealing a property table from one structure and giving it to another.

(In reply to comment #2) > (From update of attachment 199095 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=199095&action=review > > > Source/JavaScriptCore/ChangeLog:73 > > +2013-04-22 Filip Pizlo <fpizlo@apple.com> > > + > > + * bytecode/GetByIdStatus.cpp: > > + (JSC::GetByIdStatus::computeFromLLInt): > > + (JSC::GetByIdStatus::computeForChain): > > + (JSC::GetByIdStatus::computeFor): > > + * bytecode/PutByIdStatus.cpp: > > + (JSC::PutByIdStatus::computeFromLLInt): > > + (JSC::PutByIdStatus::computeFor): > > + * runtime/PropertyMapHashTable.h: > > + (PropertyTable): > > + (JSC::PropertyTable::findConcurrently): > > + (JSC): > > + (JSC::PropertyTable::add): > > + (JSC::PropertyTable::remove): > > + (JSC::PropertyTable::reinsert): > > + (JSC::PropertyTable::rehash): > > + * runtime/PropertyTable.cpp: > > + (JSC::PropertyTable::PropertyTable): > > + * runtime/Structure.cpp: > > + (JSC::Structure::findStructuresAndMapForMaterialization): > > + (JSC::Structure::getConcurrently): > > + * runtime/Structure.h: > > Duplicate changelog > > > Source/JavaScriptCore/runtime/PropertyMapHashTable.h:366 > > + Locker locker(m_lock); > > So add() locks, what about delete? Do we need that to lock? > > > Source/WTF/wtf/Locker.h:43 > > + Locker(AssumeLockedTag) > > + : m_lockable(0) > > + { > > + } > > I wonder if there's someway we could add an assertion to make sure this is the case I don't think there is. The point is that you would use this in constructors, for example: you just allocated an object and you want to tell it "no, I didn't grab a lock, and yes, that is correct" because you know that since you just allocated it, it could not have escaped.

Created attachment 199102 [details] the patch Not yet ready for review because I want to rerun perf tests and such.

Created attachment 199113 [details] the patch

Comment on attachment 199113 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=199113&action=review > Source/JavaScriptCore/runtime/Structure.cpp:274 > + createPropertyMap(AssumeLocked, vm, numberOfSlotsForLastOffset(m_offset, m_inlineCapacity)); You need to have a lock here through the end of the function in the create case.

(In reply to comment #7) > (From update of attachment 199113 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=199113&action=review > > > Source/JavaScriptCore/runtime/Structure.cpp:274 > > + createPropertyMap(AssumeLocked, vm, numberOfSlotsForLastOffset(m_offset, m_inlineCapacity)); > > You need to have a lock here through the end of the function in the create case. Threads, man. How do they work?

Created attachment 199118 [details] the patch

Comment on attachment 199118 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=199118&action=review If we use findStructuresAndMapForMaterialization in more places, let's add a helper object that unlocks automatically for you when it goes out of scope. > Source/JavaScriptCore/ChangeLog:45 > +2013-04-22 Filip Pizlo <fpizlo@apple.com> M-x Delete.

Created attachment 199127 [details] patch for landing Deleted the part of the ChangeLog that needed deleting. Still running LayoutTests.

Landed in http://trac.webkit.org/changeset/148936