Summary: | Make JSCells have 32-bit Structure pointers | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, bunhere, commit-queue, eflews.bot, fpizlo, ggaren, gtk-ews, gyuyoung.kim, ossy, philn, rakuco, rego+ews, rniwa, sergio, xan.lopez | ||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
Bug Depends on: | 124540 | ||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Mark Hahnenberg
2013-10-22 20:10:21 PDT
Created attachment 214921 [details]
Patch
Comment on attachment 214921 [details] Patch Attachment 214921 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/7468047 Created attachment 214923 [details]
Patch
Comment on attachment 214923 [details] Patch Attachment 214923 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/7808091 Comment on attachment 214923 [details] Patch Attachment 214923 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/5628083 Comment on attachment 214923 [details] Patch Attachment 214923 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/9228019 Comment on attachment 214923 [details] Patch Attachment 214923 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/7678064 Created attachment 215198 [details]
Patch
Comment on attachment 215198 [details] Patch Attachment 215198 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/12988001 Comment on attachment 215198 [details] Patch Attachment 215198 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/13018001 Comment on attachment 215198 [details] Patch Attachment 215198 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/12978004 Comment on attachment 215198 [details] Patch Attachment 215198 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/12928017 Created attachment 215587 [details]
Patch
Comment on attachment 215587 [details] Patch Attachment 215587 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/17078370 Comment on attachment 215587 [details] Patch Attachment 215587 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/17108363 Created attachment 215592 [details]
Patch
Comment on attachment 215592 [details] Patch Attachment 215592 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/17078383 Created attachment 215594 [details]
Patch
Created attachment 215667 [details]
Patch
Comment on attachment 215667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215667&action=review > Source/JavaScriptCore/jit/JSInterfaceJIT.h:402 > - ALWAYS_INLINE JSInterfaceJIT::Jump JSInterfaceJIT::emitJumpIfNotType(RegisterID baseReg, RegisterID scratchReg, JSType type) > + ALWAYS_INLINE void JSInterfaceJIT::emitLoadStructure(RegisterID source, RegisterID dest, RegisterID scratch) > { > - loadPtr(Address(baseReg, JSCell::structureOffset()), scratchReg); > - return branch8(NotEqual, Address(scratchReg, Structure::typeInfoTypeOffset()), TrustedImm32(type)); > +#if USE(JSVALUE64) > + load32(MacroAssembler::Address(source, JSCell::structureIDOffset()), dest); > + loadPtr(vm()->heap.structureIDTable().base(), scratch); > + loadPtr(BaseIndex(scratch, dest, TimesEight), dest); > +#else > + UNUSED_PARAM(scratch); > + loadPtr(MacroAssembler::Address(source, JSCell::structureIDOffset()), dest); > +#endif > + } You could put this into AssemblyHelpers. > Source/JavaScriptCore/jit/UnusedPointer.h:35 > +#if USE(JSVALUE64) > +static const uintptr_t unusedPointer = 0x0; > +#else > static const uintptr_t unusedPointer = 0xd1e7beef; > +#endif Kill if possible. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:304 > +# loadb Structure::m_indexingType[structure], scratch > +# storeb scratch, JSCell::m_indexingType[cell] > +# > +# loadb Structure::m_typeInfo + TypeInfo::m_flags[structure], scratch > +# storeb scratch, JSCell::m_flags[cell] > +# > +# loadb Structure::m_typeInfo + TypeInfo::m_type[structure], scratch > +# storeb scratch, JSCell::m_type[cell] > +# > +# storep structure, JSCell::m_structureID[cell] Delete. > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:229 > +# loadb Structure::m_indexingType[structure], scratch > +# storeb scratch, JSCell::m_indexingType[cell] > +# > +# loadb Structure::m_typeInfo + TypeInfo::m_flags[structure], scratch > +# storeb scratch, JSCell::m_flags[cell] > +# > +# loadb Structure::m_typeInfo + TypeInfo::m_type[structure], scratch > +# storeb scratch, JSCell::m_type[cell] > +# > +# loadi Structure::m_structureID[structure], scratch > +# storei scratch, JSCell::m_structureID[cell] Srsly bra Created attachment 217203 [details]
Patch
Created attachment 217204 [details]
jsc benchmark results
These are the benchmark results for this latest patch. Overall a slight slowdown. Sunspider and kraken are both 1-2% slower, v8v7 is ~0.5% faster, octane is neutral. 3d-cube seems to be the largest overall contributor to the slowdown in sunspider and especially in longspider (~30% slowdown).
Created attachment 217205 [details]
rebase
Comment on attachment 217205 [details] rebase Attachment 217205 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/27738047 Comment on attachment 217205 [details] rebase Attachment 217205 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/27718058 Comment on attachment 217205 [details] rebase View in context: https://bugs.webkit.org/attachment.cgi?id=217205&action=review > Source/JavaScriptCore/runtime/VM.h:535 > + inline VM* VM::currentVM() > + { > +#if USE(PTHREAD_GETSPECIFIC_DIRECT) > + return reinterpret_cast<VM*>(pthread_getspecific(s_currentVM)); > +#else > + return reinterpret_cast<VM*>(WTF::threadSpecificGet(s_currentVM)); > +#endif > + } > + > + inline void VM::setCurrentVM(VM* vm) > + { > +#if USE(PTHREAD_GETSPECIFIC_DIRECT) > + pthread_setspecific(s_currentVM, reinterpret_cast<void*>(vm)); > +#else > + WTF::threadSpecificSet(s_currentVM, reinterpret_cast<void*>(vm)); > +#endif > + } You want the _direct variant of pthread_getspecific and pthread_setspecific. That's the inlined version. > Overall a slight slowdown. Sunspider and kraken are both 1-2% slower....
What's your plan for this?
Created attachment 225174 [details]
Patch
Comment on attachment 225174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225174&action=review LGTM, but make those changes. > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2228 > + ALWAYS_INLINE Jump branchPtrWithPatch(RelationalCondition cond, Address left, DataLabel32& dataLabel, TrustedImm32 initialRightValue = TrustedImm32(0)) > + { > + dataLabel = DataLabel32(this); > + moveWithPatch(initialRightValue, getCachedDataTempRegisterIDAndInvalidate()); > + return branch32(cond, left, dataTempRegister); > + } Rename to branch32WithPatch > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4019 > - m_out.storePtr(structure, result, m_heaps.JSCell_structure); > + LValue structureID = m_out.load32(structure, m_heaps.Structure_structureID); > + LValue indexingType = m_out.load8(structure, m_heaps.Structure_indexingType); > + LValue typeInfoType = m_out.load8(structure, m_heaps.Structure_typeInfoType); > + LValue typeInfoFlags = m_out.load8(structure, m_heaps.Structure_typeInfoFlags); > + > + m_out.store32(structureID, result, m_heaps.JSCell_structureID); > + m_out.store8(indexingType, result, m_heaps.JSCell_indexingType); > + m_out.store8(typeInfoType, result, m_heaps.JSCell_typeInfoType); > + m_out.store8(typeInfoFlags, result, m_heaps.JSCell_typeInfoFlags); > + m_out.store8(m_out.constInt8(0), result, m_heaps.JSCell_gcData); The common case is that we're passing a constant structure. Factor this in such a way that if structure is a constant, this doesn't emit loads. > Source/JavaScriptCore/jit/UnusedPointer.h:35 > +#if USE(JSVALUE64) > +static const uintptr_t unusedPointer = 0x0; > +#else > static const uintptr_t unusedPointer = 0xd1e7beef; > +#endif Can we arrange to keep diet beef as the unused pointer? > Source/JavaScriptCore/runtime/JSCellInlines.h:95 > +inline IndexingType JSCell::indexingType() const > +{ > + return (*reinterpret_cast<const uint64_t*>(this) & 0x000000ff00000000) >> 32; > } Web template framework? > Source/JavaScriptCore/runtime/Structure.h:510 > + // These need to be properly aligned at the beginning of the 'Structure' > + // part of the object. > + StructureID m_structureID; > + IndexingType m_indexingType; > + TypeInfo m_typeInfo; Unionify. Created attachment 225214 [details]
Patch
(In reply to comment #30) > Created an attachment (id=225214) [details] > Patch Can has description of what changed? /me doesn't want to read the whole patch again. (In reply to comment #31) > (In reply to comment #30) > > Created an attachment (id=225214) [details] [details] > > Patch > > Can has description of what changed? > > /me doesn't want to read the whole patch again. The biggest change is that I added a StructureIDBlob class to encapsulate most of the crazy related to loading the blob out of the Structure during object initialization. It basically splits the TypeInfo in half (inlineTypeFlags and outOfLineTypeFlags), which you can recombine at any point to get a TypeInfo back. Also fixing some of your other comments, and trying to get all the ports building :-) Comment on attachment 225214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225214&action=review > Source/JavaScriptCore/runtime/JSCellInlines.h:112 > + Structure* structure = cell->structure(); > + visitor.appendUnbarrieredPointer(&structure); Seems unfortunate to have to do the expensive JS::structure(), could we pull the VM& from the SlotVisitor? > Source/JavaScriptCore/runtime/JSDestructibleObject.h:37 > if (MarkedBlock::blockFor(this)->destructorType() == MarkedBlock::Normal) > return static_cast<const JSDestructibleObject*>(this)->classInfo(); > -#if ENABLE(GC_VALIDATION) > - return m_structure.unvalidatedGet()->classInfo(); > -#else > - return m_structure->classInfo(); > -#endif > + return structure()->classInfo(); > } Since you are already doing MarkedBlock::blockFor(this), can you pull the Heap (and then the VM&) out of it instead of repeating the same work in structure()? > Source/JavaScriptCore/runtime/JSFunction.cpp:396 > + thisObject->methodTable(exec->vm())->getOwnPropertySlot(thisObject, exec, exec->propertyNames().prototype, slot); You could probably reduce the generated code size here by pulling the VM& into a local and using it for things like propertyNames(), which otherwise will probably repeat the load. > Source/JavaScriptCore/runtime/JSObject.cpp:657 > + Structure* structure = this->structure(); You've got the VM, why not use it? Comment on attachment 225214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225214&action=review > Source/JavaScriptCore/assembler/X86Assembler.h:2079 > + const int instructionSize = 6; // REX MOV IMM32 It should be unsigned to fix the EFL build. It is enough to make EFL build happy. Created attachment 225275 [details]
Patch
(In reply to comment #35) > Created an attachment (id=225275) [details] > Patch No major changes, just fixing builds and review feedback. Comment on attachment 225275 [details]
Patch
R=me
Created attachment 225288 [details]
Patch
Committed r164764: <http://trac.webkit.org/changeset/164764> Looks like this change introduced a crash inside Indentifier::add. I'm hitting it on a few hundred tests now. identifierTable is null inside JSC::Identifier::add. (In reply to comment #41) > identifierTable is null inside JSC::Identifier::add. Odd. I was passing all tests prior to landing. Bug? Backtrace? (In reply to comment #42) > (In reply to comment #41) > > identifierTable is null inside JSC::Identifier::add. > > Odd. I was passing all tests prior to landing. Bug? Backtrace? Sam suggested I might be hitting a stale build file issue so I'm doing a full clean build now. Will report back traces when that's done. (In reply to comment #43) > (In reply to comment #42) > > (In reply to comment #41) > > > identifierTable is null inside JSC::Identifier::add. > > > > Odd. I was passing all tests prior to landing. Bug? Backtrace? > > Sam suggested I might be hitting a stale build file issue so I'm doing a full clean build now. Will report back traces when that's done. Looks like this was it. Sorry about the false positive :( (In reply to comment #44) > (In reply to comment #43) > > (In reply to comment #42) > > > (In reply to comment #41) > > > > identifierTable is null inside JSC::Identifier::add. > > > > > > Odd. I was passing all tests prior to landing. Bug? Backtrace? > > > > Sam suggested I might be hitting a stale build file issue so I'm doing a full clean build now. Will report back traces when that's done. > > Looks like this was it. Sorry about the false positive :( Whew! :-) |