Bug 123195

Summary: Make JSCells have 32-bit Structure pointers
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
jsc benchmark results
none
rebase
none
Patch
none
Patch
none
Patch
none
Patch none

Description Mark Hahnenberg 2013-10-22 20:10:21 PDT
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.
Comment 1 Mark Hahnenberg 2013-10-22 20:21:24 PDT
Created attachment 214921 [details]
Patch
Comment 2 EFL EWS Bot 2013-10-22 20:31:43 PDT
Comment on attachment 214921 [details]
Patch

Attachment 214921 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/7468047
Comment 3 Mark Hahnenberg 2013-10-22 20:44:52 PDT
Created attachment 214923 [details]
Patch
Comment 4 EFL EWS Bot 2013-10-22 20:52:26 PDT
Comment on attachment 214923 [details]
Patch

Attachment 214923 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/7808091
Comment 5 Build Bot 2013-10-22 21:25:05 PDT
Comment on attachment 214923 [details]
Patch

Attachment 214923 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/5628083
Comment 6 EFL EWS Bot 2013-10-23 01:13:35 PDT
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 7 kov's GTK+ EWS bot 2013-10-23 01:16:33 PDT
Comment on attachment 214923 [details]
Patch

Attachment 214923 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/7678064
Comment 8 Mark Hahnenberg 2013-10-25 12:31:31 PDT
Created attachment 215198 [details]
Patch
Comment 9 EFL EWS Bot 2013-10-25 12:41:42 PDT
Comment on attachment 215198 [details]
Patch

Attachment 215198 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/12988001
Comment 10 EFL EWS Bot 2013-10-25 12:49:19 PDT
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 11 Build Bot 2013-10-25 13:14:24 PDT
Comment on attachment 215198 [details]
Patch

Attachment 215198 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/12978004
Comment 12 kov's GTK+ EWS bot 2013-10-25 13:30:11 PDT
Comment on attachment 215198 [details]
Patch

Attachment 215198 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/12928017
Comment 13 Mark Hahnenberg 2013-10-30 18:03:35 PDT
Created attachment 215587 [details]
Patch
Comment 14 EFL EWS Bot 2013-10-30 18:14:06 PDT
Comment on attachment 215587 [details]
Patch

Attachment 215587 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/17078370
Comment 15 Build Bot 2013-10-30 18:45:52 PDT
Comment on attachment 215587 [details]
Patch

Attachment 215587 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/17108363
Comment 16 Mark Hahnenberg 2013-10-30 18:56:16 PDT
Created attachment 215592 [details]
Patch
Comment 17 EFL EWS Bot 2013-10-30 19:05:20 PDT
Comment on attachment 215592 [details]
Patch

Attachment 215592 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/17078383
Comment 18 Mark Hahnenberg 2013-10-30 19:06:35 PDT
Created attachment 215594 [details]
Patch
Comment 19 Mark Hahnenberg 2013-10-31 13:01:29 PDT
Created attachment 215667 [details]
Patch
Comment 20 Filip Pizlo 2013-10-31 13:45:02 PDT
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
Comment 21 Mark Hahnenberg 2013-11-18 09:27:33 PST
Created attachment 217203 [details]
Patch
Comment 22 Mark Hahnenberg 2013-11-18 09:32:46 PST
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).
Comment 23 Mark Hahnenberg 2013-11-18 09:38:44 PST
Created attachment 217205 [details]
rebase
Comment 24 Build Bot 2013-11-18 10:11:08 PST
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 25 Build Bot 2013-11-18 10:16:34 PST
Comment on attachment 217205 [details]
rebase

Attachment 217205 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/27718058
Comment 26 Geoffrey Garen 2013-11-18 10:59:18 PST
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.
Comment 27 Geoffrey Garen 2013-11-18 11:00:15 PST
> Overall a slight slowdown. Sunspider and kraken are both 1-2% slower....

What's your plan for this?
Comment 28 Mark Hahnenberg 2014-02-25 12:58:48 PST
Created attachment 225174 [details]
Patch
Comment 29 Filip Pizlo 2014-02-25 13:38:43 PST
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.
Comment 30 Mark Hahnenberg 2014-02-25 19:25:58 PST
Created attachment 225214 [details]
Patch
Comment 31 Filip Pizlo 2014-02-25 19:31:34 PST
(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.
Comment 32 Mark Hahnenberg 2014-02-25 19:35:33 PST
(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 33 Sam Weinig 2014-02-26 09:15:35 PST
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 34 Csaba Osztrogonác 2014-02-26 09:58:55 PST
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.
Comment 35 Mark Hahnenberg 2014-02-26 12:06:33 PST
Created attachment 225275 [details]
Patch
Comment 36 Mark Hahnenberg 2014-02-26 12:07:26 PST
(In reply to comment #35)
> Created an attachment (id=225275) [details]
> Patch

No major changes, just fixing builds and review feedback.
Comment 37 Filip Pizlo 2014-02-26 12:12:04 PST
Comment on attachment 225275 [details]
Patch

R=me
Comment 38 Mark Hahnenberg 2014-02-26 13:26:27 PST
Created attachment 225288 [details]
Patch
Comment 39 Mark Hahnenberg 2014-02-26 17:22:02 PST
Committed r164764: <http://trac.webkit.org/changeset/164764>
Comment 40 Ryosuke Niwa 2014-02-26 18:32:52 PST
Looks like this change introduced a crash inside Indentifier::add.  I'm hitting it on a few hundred tests now.
Comment 41 Ryosuke Niwa 2014-02-26 18:33:53 PST
identifierTable is null inside JSC::Identifier::add.
Comment 42 Mark Hahnenberg 2014-02-26 18:35:00 PST
(In reply to comment #41)
> identifierTable is null inside JSC::Identifier::add.

Odd. I was passing all tests prior to landing. Bug? Backtrace?
Comment 43 Ryosuke Niwa 2014-02-26 18:44:05 PST
(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.
Comment 44 Ryosuke Niwa 2014-02-26 18:53:59 PST
(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 :(
Comment 45 Mark Hahnenberg 2014-02-26 18:54:47 PST
(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! :-)