Changeset 206314 in webkit


Ignore:
Timestamp:
Sep 23, 2016 11:09:44 AM (8 years ago)
Author:
fpizlo@apple.com
Message:

Source/JavaScriptCore:
Need a store-load fence between setting cell state and visiting the object in SlotVisitor
​https://bugs.webkit.org/show_bug.cgi?id=162354

Reviewed by Mark Lam.

This was meant to be a small change, but then it became bigger as I found small
opportunities for improving this code. This adds a store-load fence and is performance-
neutral. That's probably partly due to other optimizations that I did to visitChildren().

Initially, I found that adding an mfence as a store-load fence was terribly expensive. So,
I thought that I needed to buffer up a bunch of objects, set their states, do one mfence,
and then visit all of them. This seemed like a win, so I went with it. Unfortunately, this
made no sense for two reasons:

  • I shouldn't use mfence. I should use ortop (lock orl $0, (%rsp)) instead. Ortop is basically free, and it's what WTF now uses for storeLoadFence().


  • My data saying that buffering up objects was not a slow-down was wrong. That was actually almost as expensive as the mfence.


But in order to implement that, I made some other improvements that I think we should stick
with:

  • SlotVisitor::visitChildren() now uses a switch on type. This replaces what used to be some nasty ClassInfo look-ups.


  • We no longer save the object's old CellState. We would do that so that we would know what state the object had been before we blackened it. But I believe that the more logical solution is to have two kinds of black - one for black-for-the-first-time objects and one for repeat offenders. This is a lot easier to reason about, since you can now just figure this out by looking at the cell directly.


The latter change meant rewiring a bunch of barriers. It didn't make them any more
expensive.

  • ftl/FTLLowerDFGToB3.cpp:

(JSC::FTL::DFG::LowerDFGToB3::emitStoreBarrier):

  • heap/CellState.h:

(JSC::blacken):

  • heap/Heap.cpp:

(JSC::Heap::addToRememberedSet):

  • heap/Heap.h:
  • heap/HeapInlines.h:

(JSC::Heap::writeBarrier):
(JSC::Heap::reportExtraMemoryVisited):
(JSC::Heap::reportExternalMemoryVisited):

  • heap/MarkStack.cpp:
  • heap/MarkStack.h:
  • heap/SlotVisitor.cpp:

(JSC::SlotVisitor::visitChildren):

  • heap/SlotVisitor.h:
  • heap/SlotVisitorInlines.h:

(JSC::SlotVisitor::reportExtraMemoryVisited):
(JSC::SlotVisitor::reportExternalMemoryVisited):

  • jit/AssemblyHelpers.h:

(JSC::AssemblyHelpers::jumpIfIsRememberedOrInEden):

  • llint/LLIntData.cpp:

(JSC::LLInt::Data::performAssertions):

  • llint/LowLevelInterpreter.asm:
  • llint/LowLevelInterpreter32_64.asm:
  • llint/LowLevelInterpreter64.asm:
  • runtime/JSObject.h:

(JSC::isJSFinalObject):

Source/WTF:
REGRESSION(r194387): Crash on github.com in IntlDateTimeFormat::resolvedOptions in C locale
​https://bugs.webkit.org/show_bug.cgi?id=162139

Patch by Carlos Garcia Campos <​cgarcia@igalia.com> on 2016-09-23
Reviewed by Michael Catanzaro.

Handle the case of "C" or "POSIX" locale and use "en-US" as default. That matches what ICU and other ports do,
as well as what layout tests expect (some tests like js/intl-collator.html pass in the bots only because we use
en-US as system locale in those bots).

  • wtf/PlatformUserPreferredLanguagesUnix.cpp:

(WTF::platformLanguage):

Location:
trunk/Source
Files:
19 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r206297 r206314  
     12016-09-22  Filip Pizlo  <fpizlo@apple.com>
     2
     3        Need a store-load fence between setting cell state and visiting the object in SlotVisitor
     4        https://bugs.webkit.org/show_bug.cgi?id=162354
     5
     6        Reviewed by Mark Lam.
     7       
     8        This was meant to be a small change, but then it became bigger as I found small
     9        opportunities for improving this code. This adds a store-load fence and is performance-
     10        neutral. That's probably partly due to other optimizations that I did to visitChildren().
     11       
     12        Initially, I found that adding an mfence as a store-load fence was terribly expensive. So,
     13        I thought that I needed to buffer up a bunch of objects, set their states, do one mfence,
     14        and then visit all of them. This seemed like a win, so I went with it. Unfortunately, this
     15        made no sense for two reasons:
     16       
     17        - I shouldn't use mfence. I should use ortop (lock orl $0, (%rsp)) instead. Ortop is
     18          basically free, and it's what WTF now uses for storeLoadFence().
     19       
     20        - My data saying that buffering up objects was not a slow-down was wrong. That was actually
     21          almost as expensive as the mfence.
     22       
     23        But in order to implement that, I made some other improvements that I think we should stick
     24        with:
     25       
     26        - SlotVisitor::visitChildren() now uses a switch on type. This replaces what used to be
     27          some nasty ClassInfo look-ups.
     28       
     29        - We no longer save the object's old CellState. We would do that so that we would know what
     30          state the object had been before we blackened it. But I believe that the more logical
     31          solution is to have two kinds of black - one for black-for-the-first-time objects and one
     32          for repeat offenders. This is a lot easier to reason about, since you can now just figure
     33          this out by looking at the cell directly.
     34       
     35        The latter change meant rewiring a bunch of barriers. It didn't make them any more
     36        expensive.
     37
     38        * ftl/FTLLowerDFGToB3.cpp:
     39        (JSC::FTL::DFG::LowerDFGToB3::emitStoreBarrier):
     40        * heap/CellState.h:
     41        (JSC::blacken):
     42        * heap/Heap.cpp:
     43        (JSC::Heap::addToRememberedSet):
     44        * heap/Heap.h:
     45        * heap/HeapInlines.h:
     46        (JSC::Heap::writeBarrier):
     47        (JSC::Heap::reportExtraMemoryVisited):
     48        (JSC::Heap::reportExternalMemoryVisited):
     49        * heap/MarkStack.cpp:
     50        * heap/MarkStack.h:
     51        * heap/SlotVisitor.cpp:
     52        (JSC::SlotVisitor::visitChildren):
     53        * heap/SlotVisitor.h:
     54        * heap/SlotVisitorInlines.h:
     55        (JSC::SlotVisitor::reportExtraMemoryVisited):
     56        (JSC::SlotVisitor::reportExternalMemoryVisited):
     57        * jit/AssemblyHelpers.h:
     58        (JSC::AssemblyHelpers::jumpIfIsRememberedOrInEden):
     59        * llint/LLIntData.cpp:
     60        (JSC::LLInt::Data::performAssertions):
     61        * llint/LowLevelInterpreter.asm:
     62        * llint/LowLevelInterpreter32_64.asm:
     63        * llint/LowLevelInterpreter64.asm:
     64        * runtime/JSObject.h:
     65        (JSC::isJSFinalObject):
     66
    1672016-09-23  Csaba Osztrogonác  <ossy@webkit.org>
    268

  • trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

    r206289 r206314  
    1143611436
    1143711437        m_out.branch(
    11438             m_out.notZero32(loadCellState(base)), usually(continuation), rarely(slowPath));
     11438            m_out.above(loadCellState(base), m_out.constInt32(blackThreshold)),
     11439            usually(continuation), rarely(slowPath));
    1143911440
    1144011441        LBasicBlock lastNext = m_out.appendTo(slowPath, continuation);

  • trunk/Source/JavaScriptCore/heap/CellState.h

    r190569 r206314  
    11/*
    2  * Copyright (C) 2015 Apple Inc. All rights reserved.
     2 * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    2727#define CellState_h
    2828
     29#include <wtf/Assertions.h>
     30
    2931namespace JSC {
    3032
    3133enum class CellState : uint8_t {
    32     // The object is black as far as this GC is concerned. When not in GC, this just means that it's an
    33     // old gen object. Note that we deliberately arrange OldBlack to be zero, so that the store barrier on
    34     // a target object "from" is just:
    35     //
    36     // if (!from->cellState())
    37     //     slowPath(from);
    38     //
    39     // There is a bunch of code in the LLInt and JITs that rely on this being the case. You'd have to
    40     // change a lot of code if you ever wanted the store barrier to be anything but a non-zero check on
    41     // cellState.
    42     OldBlack = 0,
     34    // The object is black for the first time during this GC.
     35    NewBlack = 0,
     36   
     37    // The object is black for the Nth time during this full GC cycle (N > 1). An object may get to
     38    // this state if it transitions from black back to grey during a concurrent GC, or because it
     39    // wound up in the remembered set because of a generational barrier.
     40    OldBlack = 1,
    4341   
    4442    // The object is in eden. During GC, this means that the object has not been marked yet.
    45     NewWhite = 1,
     43    NewWhite = 2,
     44
     45    // The object is grey - i.e. it will be scanned - and this is the first time in this GC that we are
     46    // going to scan it. If this is an eden GC, this also means that the object is in eden.
     47    NewGrey = 3,
    4648
    4749    // The object is grey - i.e. it will be scanned - but it either belongs to old gen (if this is eden
    4850    // GC) or it is grey a second time in this current GC (because a concurrent store barrier requested
    4951    // re-greying).
    50     OldGrey = 2,
     52    OldGrey = 4
     53};
    5154
    52     // The object is grey - i.e. it will be scanned - and this is the first time in this GC that we are
    53     // going to scan it. If this is an eden GC, this also means that the object is in eden.
    54     NewGrey = 3
    55 };
     55static const unsigned blackThreshold = 1; // x <= blackThreshold means x is black.
     56
     57inline bool isBlack(CellState cellState)
     58{
     59    return static_cast<unsigned>(cellState) <= blackThreshold;
     60}
     61
     62inline CellState blacken(CellState cellState)
     63{
     64    if (cellState == CellState::NewGrey)
     65        return CellState::NewBlack;
     66    ASSERT(cellState == CellState::NewBlack || cellState == CellState::OldBlack || cellState == CellState::OldGrey);
     67    return CellState::OldBlack;
     68}
    5669
    5770} // namespace JSC

  • trunk/Source/JavaScriptCore/heap/Heap.cpp

    r206154 r206314  
    2929#include "GCActivityCallback.h"
    3030#include "GCIncomingRefCountedSetInlines.h"
     31#include "GCSegmentedArrayInlines.h"
    3132#include "GCTypeMap.h"
    3233#include "HasOwnPropertyCache.h"
    … …  
    915916    ASSERT(cell);
    916917    ASSERT(!Options::useConcurrentJIT() || !isCompilationThread());
    917     ASSERT(cell->cellState() == CellState::OldBlack);
     918    ASSERT(isBlack(cell->cellState()));
    918919    // Indicate that this object is grey and that it's one of the following:
    919920    // - A re-greyed object during a concurrent collection.

  • trunk/Source/JavaScriptCore/heap/Heap.h

    r206154 r206314  
    176176    // memory growth.
    177177    void reportExtraMemoryAllocated(size_t);
    178     void reportExtraMemoryVisited(CellState cellStateBeforeVisiting, size_t);
     178    void reportExtraMemoryVisited(JSCell*, size_t);
    179179
    180180#if ENABLE(RESOURCE_USAGE)
    181181    // Use this API to report the subset of extra memory that lives outside this process.
    182     void reportExternalMemoryVisited(CellState cellStateBeforeVisiting, size_t);
     182    void reportExternalMemoryVisited(JSCell*, size_t);
    183183    size_t externalMemorySize() { return m_externalMemorySize; }
    184184#endif

  • trunk/Source/JavaScriptCore/heap/HeapInlines.h

    r206172 r206314  
    126126    WriteBarrierCounters::countWriteBarrier();
    127127#endif
    128     if (!from || from->cellState() != CellState::OldBlack)
     128    if (!from || !isBlack(from->cellState()))
    129129        return;
    130130    if (!to || to->cellState() != CellState::NewWhite)
    … …  
    136136{
    137137    ASSERT_GC_OBJECT_LOOKS_VALID(const_cast<JSCell*>(from));
    138     if (!from || from->cellState() != CellState::OldBlack)
     138    if (!from || !isBlack(from->cellState()))
    139139        return;
    140140    addToRememberedSet(from);
    … …  
    147147}
    148148
    149 inline void Heap::reportExtraMemoryVisited(CellState dataBeforeVisiting, size_t size)
     149inline void Heap::reportExtraMemoryVisited(JSCell* cell, size_t size)
    150150{
    151151    // We don't want to double-count the extra memory that was reported in previous collections.
    152     if (operationInProgress() == EdenCollection && dataBeforeVisiting == CellState::OldGrey)
     152    if (operationInProgress() == EdenCollection && cell->cellState() == CellState::OldBlack)
    153153        return;
    154154
    … …  
    163163
    164164#if ENABLE(RESOURCE_USAGE)
    165 inline void Heap::reportExternalMemoryVisited(CellState dataBeforeVisiting, size_t size)
     165inline void Heap::reportExternalMemoryVisited(JSCell* cell, size_t size)
    166166{
    167167    // We don't want to double-count the external memory that was reported in previous collections.
    168     if (operationInProgress() == EdenCollection && dataBeforeVisiting == CellState::OldGrey)
     168    if (operationInProgress() == EdenCollection && cell->cellState() == CellState::OldBlack)
    169169        return;
    170170

  • trunk/Source/JavaScriptCore/heap/MarkStack.cpp

    r190267 r206314  
    2727#include "MarkStack.h"
    2828
     29#include "GCSegmentedArrayInlines.h"
    2930#include "JSCInlines.h"
    3031

  • trunk/Source/JavaScriptCore/heap/MarkStack.h

    r179348 r206314  
    2727#define MarkStack_h
    2828
    29 #include "GCSegmentedArrayInlines.h"
     29#include "GCSegmentedArray.h"
    3030
    3131namespace JSC {

  • trunk/Source/JavaScriptCore/heap/SlotVisitor.cpp

    r206172 r206314  
    2626#include "config.h"
    2727#include "SlotVisitor.h"
    28 #include "SlotVisitorInlines.h"
    2928
    3029#include "ConservativeRoots.h"
     30#include "GCSegmentedArrayInlines.h"
    3131#include "HeapCellInlines.h"
    3232#include "HeapProfiler.h"
    … …  
    3737#include "JSString.h"
    3838#include "JSCInlines.h"
     39#include "SlotVisitorInlines.h"
    3940#include "SuperSampler.h"
    4041#include "VM.h"
    … …  
    297298    SetCurrentCellScope currentCellScope(*this, cell);
    298299   
    299     m_currentObjectCellStateBeforeVisiting = cell->cellState();
    300     cell->setCellState(CellState::OldBlack);
    301    
    302     if (isJSString(cell)) {
     300    cell->setCellState(blacken(cell->cellState()));
     301   
     302    // FIXME: Make this work on ARM also.
     303    // https://bugs.webkit.org/show_bug.cgi?id=162461
     304    if (isX86())
     305        WTF::storeLoadFence();
     306   
     307    switch (cell->type()) {
     308    case StringType:
    303309        JSString::visitChildren(const_cast<JSCell*>(cell), *this);
    304         return;
    305     }
    306 
    307     if (isJSFinalObject(cell)) {
     310        break;
     311       
     312    case FinalObjectType:
    308313        JSFinalObject::visitChildren(const_cast<JSCell*>(cell), *this);
    309         return;
    310     }
    311 
    312     if (isJSArray(cell)) {
     314        break;
     315
     316    case ArrayType:
    313317        JSArray::visitChildren(const_cast<JSCell*>(cell), *this);
    314         return;
    315     }
    316 
    317     cell->methodTable()->visitChildren(const_cast<JSCell*>(cell), *this);
     318        break;
     319       
     320    default:
     321        // FIXME: This could be so much better.
     322        // https://bugs.webkit.org/show_bug.cgi?id=162462
     323        cell->methodTable()->visitChildren(const_cast<JSCell*>(cell), *this);
     324        break;
     325    }
    318326}
    319327

  • trunk/Source/JavaScriptCore/heap/SlotVisitor.h

    r206172 r206314  
    169169    JSCell* m_currentCell { nullptr };
    170170
    171     CellState m_currentObjectCellStateBeforeVisiting { CellState::NewWhite };
    172 
    173171public:
    174172#if !ASSERT_DISABLED

  • trunk/Source/JavaScriptCore/heap/SlotVisitorInlines.h

    r202394 r206314  
    107107inline void SlotVisitor::reportExtraMemoryVisited(size_t size)
    108108{
    109     heap()->reportExtraMemoryVisited(m_currentObjectCellStateBeforeVisiting, size);
     109    heap()->reportExtraMemoryVisited(m_currentCell, size);
    110110}
    111111
    … …  
    113113inline void SlotVisitor::reportExternalMemoryVisited(size_t size)
    114114{
    115     heap()->reportExternalMemoryVisited(m_currentObjectCellStateBeforeVisiting, size);
     115    heap()->reportExternalMemoryVisited(m_currentCell, size);
    116116}
    117117#endif

  • trunk/Source/JavaScriptCore/jit/AssemblyHelpers.h

    r205675 r206314  
    13091309    Jump jumpIfIsRememberedOrInEden(GPRReg cell)
    13101310    {
    1311         return branchTest8(MacroAssembler::NonZero, MacroAssembler::Address(cell, JSCell::cellStateOffset()));
     1311        return branch8(Above, Address(cell, JSCell::cellStateOffset()), TrustedImm32(blackThreshold));
    13121312    }
    13131313
    … …  
    13151315    {
    13161316        uint8_t* address = reinterpret_cast<uint8_t*>(cell) + JSCell::cellStateOffset();
    1317         return branchTest8(MacroAssembler::NonZero, MacroAssembler::AbsoluteAddress(address));
     1317        return branch8(Above, AbsoluteAddress(address), TrustedImm32(blackThreshold));
    13181318    }
    13191319   

  • trunk/Source/JavaScriptCore/llint/LLIntData.cpp

    r206098 r206314  
    215215
    216216    STATIC_ASSERT(MarkedBlock::blockSize == 16 * 1024);
     217    STATIC_ASSERT(blackThreshold == 1);
    217218
    218219    ASSERT(bitwise_cast<uintptr_t>(ShadowChicken::Packet::tailMarker()) == static_cast<uintptr_t>(0x7a11));

  • trunk/Source/JavaScriptCore/llint/LowLevelInterpreter.asm

    r206289 r206314  
    409409const MarkedBlockSize = 16 * 1024
    410410const MarkedBlockMask = ~(MarkedBlockSize - 1)
     411
     412const BlackThreshold = 1
    411413
    412414# Allocation constants
    … …  
    889891end
    890892
    891 macro skipIfIsRememberedOrInEden(cell, scratch1, scratch2, continuation)
    892     loadb JSCell::m_cellState[cell], scratch1
    893     continuation(scratch1)
     893macro skipIfIsRememberedOrInEden(cell, slowPath)
     894    bba JSCell::m_cellState[cell], BlackThreshold, .done
     895    slowPath()
     896.done:
    894897end
    895898

  • trunk/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm

    r206289 r206314  
    501501    loadisFromInstruction(cellOperand, t1)
    502502    loadConstantOrVariablePayload(t1, CellTag, t2, .writeBarrierDone)
    503     skipIfIsRememberedOrInEden(t2, t1, t3,
    504         macro(cellState)
    505             btbnz cellState, .writeBarrierDone
     503    skipIfIsRememberedOrInEden(
     504        t2,
     505        macro()
    506506            push cfr, PC
    507507            # We make two extra slots because cCall2 will poke.
    … …  
    512512            addp 8, sp
    513513            pop PC, cfr
    514         end
    515     )
     514        end)
    516515.writeBarrierDone:
    517516end
    … …  
    533532    loadHelper(t3)
    534533
    535     skipIfIsRememberedOrInEden(t3, t1, t2,
    536         macro(gcData)
    537             btbnz gcData, .writeBarrierDone
     534    skipIfIsRememberedOrInEden(
     535        t3,
     536        macro()
    538537            push cfr, PC
    539538            # We make two extra slots because cCall2 will poke.
    … …  
    544543            addp 8, sp
    545544            pop PC, cfr
    546         end
    547     )
     545        end)
    548546.writeBarrierDone:
    549547end

  • trunk/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm

    r206289 r206314  
    405405    loadisFromInstruction(cellOperand, t1)
    406406    loadConstantOrVariableCell(t1, t2, .writeBarrierDone)
    407     skipIfIsRememberedOrInEden(t2, t1, t3,
    408         macro(cellState)
    409             btbnz cellState, .writeBarrierDone
     407    skipIfIsRememberedOrInEden(
     408        t2,
     409        macro()
    410410            push PB, PC
    411411            move t2, a1 # t2 can be a0 (not on 64 bits, but better safe than sorry)
    … …  
    413413            cCall2Void(_llint_write_barrier_slow)
    414414            pop PC, PB
    415         end
    416     )
     415        end)
    417416.writeBarrierDone:
    418417end
    … …  
    433432
    434433    loadHelper(t3)
    435     skipIfIsRememberedOrInEden(t3, t1, t2,
    436         macro(gcData)
    437             btbnz gcData, .writeBarrierDone
     434    skipIfIsRememberedOrInEden(
     435        t3,
     436        macro()
    438437            push PB, PC
    439438            move cfr, a0

  • trunk/Source/JavaScriptCore/runtime/JSObject.h

    r206136 r206314  
    10941094inline bool isJSFinalObject(JSCell* cell)
    10951095{
    1096     return cell->classInfo() == JSFinalObject::info();
     1096    return cell->type() == FinalObjectType;
    10971097}
    10981098

  • trunk/Source/WTF/ChangeLog

    r206295 r206314  
    1212        * wtf/PlatformUserPreferredLanguagesUnix.cpp:
    1313        (WTF::platformLanguage):
     14
     152016-09-22  Filip Pizlo  <fpizlo@apple.com>
     16
     17        Need a store-load fence between setting cell state and visiting the object in SlotVisitor
     18        https://bugs.webkit.org/show_bug.cgi?id=162354
     19
     20        Reviewed by Mark Lam.
     21       
     22        Fix this on x86-32.
     23
     24        * wtf/Atomics.h:
     25        (WTF::x86_ortop):
    1426
    15272016-09-22  Filip Pizlo  <fpizlo@apple.com>

  • trunk/Source/WTF/wtf/Atomics.h

    r206274 r206314  
    176176    // investigate if that is actually better.
    177177    MemoryBarrier();
    178 #else
     178#elif CPU(X86_64)
    179179    // This has acqrel semantics and is much cheaper than mfence. For exampe, in the JSC GC, using
    180180    // mfence as a store-load fence was a 9% slow-down on Octane/splay while using this was neutral.
    181181    asm volatile("lock; orl $0, (%%rsp)" ::: "memory");
     182#else
     183    asm volatile("lock; orl $0, (%%esp)" ::: "memory");
    182184#endif
    183185}
Note: See TracChangeset for help on using the changeset viewer.