Bug 123195 – Make JSCells have 32-bit Structure pointers

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

[Mark Hahnenberg]

This will free up 32 bits of space in JSCell headers, which we can use for other information like the IndexingType, JSType, JSTypeInfo type flags, and garbage collection metadata.

[Mark Hahnenberg]

Created attachment 214921 [details]
Patch

[EFL EWS Bot]

Comment on attachment 214921 [details]
Patch

Attachment 214921 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/7468047

[Mark Hahnenberg]

Created attachment 214923 [details]
Patch

[EFL EWS Bot]

Comment on attachment 214923 [details]
Patch

Attachment 214923 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/7808091

[Build Bot]

Comment on attachment 214923 [details]
Patch

Attachment 214923 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/5628083

[EFL EWS Bot]

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

[kov's GTK+ EWS bot]

Comment on attachment 214923 [details]
Patch

Attachment 214923 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/7678064

[Mark Hahnenberg]

Created attachment 215198 [details]
Patch

[EFL EWS Bot]

Comment on attachment 215198 [details]
Patch

Attachment 215198 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/12988001

[EFL EWS Bot]

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

[Build Bot]

Comment on attachment 215198 [details]
Patch

Attachment 215198 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/12978004

[kov's GTK+ EWS bot]

Comment on attachment 215198 [details]
Patch

Attachment 215198 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/12928017

[Mark Hahnenberg]

Created attachment 215587 [details]
Patch

[EFL EWS Bot]

Comment on attachment 215587 [details]
Patch

Attachment 215587 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/17078370

[Build Bot]

Comment on attachment 215587 [details]
Patch

Attachment 215587 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/17108363

[Mark Hahnenberg]

Created attachment 215592 [details]
Patch

[EFL EWS Bot]

Comment on attachment 215592 [details]
Patch

Attachment 215592 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/17078383

[Mark Hahnenberg]

Created attachment 215594 [details]
Patch

[Mark Hahnenberg]

Created attachment 215667 [details]
Patch

[Filip Pizlo]

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

[Mark Hahnenberg]

Created attachment 217203 [details]
Patch

[Mark Hahnenberg]

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).

[Mark Hahnenberg]

Created attachment 217205 [details]
rebase

[Build Bot]

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

[Build Bot]

Comment on attachment 217205 [details]
rebase

Attachment 217205 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/27718058

[Geoffrey Garen]

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.

[Geoffrey Garen]

> Overall a slight slowdown. Sunspider and kraken are both 1-2% slower....

What's your plan for this?

[Mark Hahnenberg]

Created attachment 225174 [details]
Patch

[Filip Pizlo]

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.

[Mark Hahnenberg]

Created attachment 225214 [details]
Patch

[Filip Pizlo]

(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.

[Mark Hahnenberg]

(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 :-)

[Sam Weinig]

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?

[Csaba Osztrogonác]

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.

[Mark Hahnenberg]

Created attachment 225275 [details]
Patch

[Mark Hahnenberg]

(In reply to comment #35)
> Created an attachment (id=225275) [details]
> Patch

No major changes, just fixing builds and review feedback.

[Filip Pizlo]

Comment on attachment 225275 [details]
Patch

R=me

[Mark Hahnenberg]

Created attachment 225288 [details]
Patch

[Mark Hahnenberg]

Committed r164764: <http://trac.webkit.org/changeset/164764>

[Ryosuke Niwa]

Looks like this change introduced a crash inside Indentifier::add.  I'm hitting it on a few hundred tests now.

[Ryosuke Niwa]

identifierTable is null inside JSC::Identifier::add.

[Mark Hahnenberg]

(In reply to comment #41)
> identifierTable is null inside JSC::Identifier::add.

Odd. I was passing all tests prior to landing. Bug? Backtrace?

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

(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 :(

[Mark Hahnenberg]

(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! :-)