Summary: | fourthTier: Create an equivalent of Structure::get() that can work from a compilation thread | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | barraclough, ggaren, mark.lam, mhahnenberg, msaboff, oliver, sam | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 112839 | ||||||||||||||
Attachments: |
|
Description
Filip Pizlo
2013-04-22 13:03:03 PDT
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 |