Changeset 248800 in webkit


Ignore:
Timestamp:
Aug 16, 2019 3:49:26 PM (5 years ago)
Author:
mark.lam@apple.com
Message:

CodeBlock destructor should clear all of its watchpoints.
https://bugs.webkit.org/show_bug.cgi?id=200792
<rdar://problem/53947800>

Reviewed by Yusuke Suzuki.

JSTests:

  • stress/codeblock-should-clear-watchpoints-on-destruction.js: Added.

Source/JavaScriptCore:

We need to clear the watchpoints explicitly (just like we do in CodeBlock::jettison())
because the JITCode may outlive the CodeBlock for a while. For example, the JITCode
is ref'd in Interpreter::execute(JSC::CallFrameClosure&) like so:

JSValue result = closure.functionExecutable->generatedJITCodeForCall()->execute(&vm, closure.protoCallFrame);

The call to generatedJITCodeForCall() returns a Ref<JITCode> with the underlying
JITCode ref'd. Hence, while the interpreter frame is still on the stack, the
executing JITCode instance will have a non-zero refCount, and be kept alive even
though its CodeBlock may have already been destructed.

Note: the Interpreter execute() methods aren't the only ones who would ref the JITCode:
ExecutableBase also holds a RefPtr<JITCode> m_jitCodeForCall and RefPtr<JITCode>
m_jitCodeForConstruct. But a CodeBlock will be uninstalled before it gets destructed.
Hence, the uninstallation will deref the JITCode before we get to the CodeBlock
destructor. That said, we should be aware that a JITCode's refCount is not always
1 after the JIT installs it into the CodeBlock, and it should not be assumed to be so.

For this patch, I also audited all Watchpoint subclasses to ensure that we are
clearing all the relevant watchpoints in the CodeBlock destructor. Here is the
list of audited Watchpoints:

CodeBlockJettisoningWatchpoint
AdaptiveStructureWatchpoint
AdaptiveInferredPropertyValueWatchpoint

  • these are held in the DFG::CommonData, and is tied to JITCode's life cycle.
  • they need to be cleared eagerly in CodeBlock's destructor.

LLIntPrototypeLoadAdaptiveStructureWatchpoint

  • stored in m_llintGetByIdWatchpointMap in the CodeBlock.
  • this will be automatically cleared on CodeBlock destruction.

The following does not reference CodeBlock:

FunctionRareData::AllocationProfileClearingWatchpoint

  • stored in FunctionRareData and will be cleared automatically on FunctionRareData destruction.
  • only references the owner FunctionRareData.

ObjectToStringAdaptiveStructureWatchpoint
ObjectToStringAdaptiveInferredPropertyValueWatchpoint

  • stored in StructureRareData and will be cleared automatically on StructureRareData destruction.

ObjectPropertyChangeAdaptiveWatchpoint

  • stored in JSGlobalObject, and will be cleared automatically on JSGlobalObject destruction.
  • only references the owner JSGlobalObject.

StructureStubClearingWatchpoint

  • stored in WatchpointsOnStructureStubInfo and will be cleared automatically on WatchpointsOnStructureStubInfo destruction.

PropertyWatchpoint
StructureWatchpoint

  • embedded in AdaptiveInferredPropertyValueWatchpointBase, which is extended as AdaptiveInferredPropertyValueWatchpoint, ObjectPropertyChangeAdaptiveWatchpoint, and ObjectToStringAdaptiveInferredPropertyValueWatchpoint.
  • life cycle is handled by those 3 subclasses.
  • bytecode/CodeBlock.cpp:

(JSC::CodeBlock::~CodeBlock):

Location:
trunk
Files:
1 added
3 edited

Legend:

Unmodified
Added
Removed
  • trunk/JSTests/ChangeLog

    r248798 r248800  
     12019-08-16  Mark Lam  <mark.lam@apple.com>
     2
     3        CodeBlock destructor should clear all of its watchpoints.
     4        https://bugs.webkit.org/show_bug.cgi?id=200792
     5        <rdar://problem/53947800>
     6
     7        Reviewed by Yusuke Suzuki.
     8
     9        * stress/codeblock-should-clear-watchpoints-on-destruction.js: Added.
     10
    1112019-08-16  Justin Michaud  <justin_michaud@apple.com>
    212
  • trunk/Source/JavaScriptCore/ChangeLog

    r248798 r248800  
     12019-08-16  Mark Lam  <mark.lam@apple.com>
     2
     3        CodeBlock destructor should clear all of its watchpoints.
     4        https://bugs.webkit.org/show_bug.cgi?id=200792
     5        <rdar://problem/53947800>
     6
     7        Reviewed by Yusuke Suzuki.
     8
     9        We need to clear the watchpoints explicitly (just like we do in CodeBlock::jettison())
     10        because the JITCode may outlive the CodeBlock for a while.  For example, the JITCode
     11        is ref'd in Interpreter::execute(JSC::CallFrameClosure&) like so:
     12
     13            JSValue result = closure.functionExecutable->generatedJITCodeForCall()->execute(&vm, closure.protoCallFrame);
     14
     15        The call to generatedJITCodeForCall() returns a Ref<JITCode> with the underlying
     16        JITCode ref'd.  Hence, while the interpreter frame is still on the stack, the
     17        executing JITCode instance will have a non-zero refCount, and be kept alive even
     18        though its CodeBlock may have already been destructed.
     19
     20        Note: the Interpreter execute() methods aren't the only ones who would ref the JITCode:
     21        ExecutableBase also holds a RefPtr<JITCode> m_jitCodeForCall and RefPtr<JITCode>
     22        m_jitCodeForConstruct.  But a CodeBlock will be uninstalled before it gets destructed.
     23        Hence, the uninstallation will deref the JITCode before we get to the CodeBlock
     24        destructor.  That said, we should be aware that a JITCode's refCount is not always
     25        1 after the JIT installs it into the CodeBlock, and it should not be assumed to be so.
     26
     27        For this patch, I also audited all Watchpoint subclasses to ensure that we are
     28        clearing all the relevant watchpoints in the CodeBlock destructor.  Here is the
     29        list of audited Watchpoints:
     30
     31            CodeBlockJettisoningWatchpoint
     32            AdaptiveStructureWatchpoint
     33            AdaptiveInferredPropertyValueWatchpoint
     34                - these are held in the DFG::CommonData, and is tied to JITCode's life cycle.
     35                - they need to be cleared eagerly in CodeBlock's destructor.
     36
     37            LLIntPrototypeLoadAdaptiveStructureWatchpoint
     38                - stored in m_llintGetByIdWatchpointMap in the CodeBlock.
     39                - this will be automatically cleared on CodeBlock destruction.
     40
     41        The following does not reference CodeBlock:
     42
     43            FunctionRareData::AllocationProfileClearingWatchpoint
     44                - stored in FunctionRareData and will be cleared automatically on
     45                  FunctionRareData destruction.
     46                - only references the owner FunctionRareData.
     47
     48            ObjectToStringAdaptiveStructureWatchpoint
     49            ObjectToStringAdaptiveInferredPropertyValueWatchpoint
     50                - stored in StructureRareData and will be cleared automatically on
     51                  StructureRareData destruction.
     52
     53            ObjectPropertyChangeAdaptiveWatchpoint
     54                - stored in JSGlobalObject, and will be cleared automatically on
     55                  JSGlobalObject destruction.
     56                - only references the owner JSGlobalObject.
     57
     58            StructureStubClearingWatchpoint
     59                - stored in WatchpointsOnStructureStubInfo and will be cleared automatically
     60                  on WatchpointsOnStructureStubInfo destruction.
     61
     62            PropertyWatchpoint
     63            StructureWatchpoint
     64                - embedded in AdaptiveInferredPropertyValueWatchpointBase, which is extended
     65                  as AdaptiveInferredPropertyValueWatchpoint, ObjectPropertyChangeAdaptiveWatchpoint,
     66                  and ObjectToStringAdaptiveInferredPropertyValueWatchpoint.
     67                - life cycle is handled by those 3 subclasses.
     68
     69        * bytecode/CodeBlock.cpp:
     70        (JSC::CodeBlock::~CodeBlock):
     71
    1722019-08-16  Justin Michaud  <justin_michaud@apple.com>
    273
  • trunk/Source/JavaScriptCore/bytecode/CodeBlock.cpp

    r248027 r248800  
    812812    VM& vm = *m_vm;
    813813
     814#if ENABLE(DFG_JIT)
     815    // The JITCode (and its corresponding DFG::CommonData) may outlive the CodeBlock by
     816    // a short amount of time after the CodeBlock is destructed. For example, the
     817    // Interpreter::execute methods will ref JITCode before invoking it. This can
     818    // result in the JITCode having a non-zero refCount when its owner CodeBlock is
     819    // destructed.
     820    //
     821    // Hence, we cannot rely on DFG::CommonData destruction to clear these now invalid
     822    // watchpoints in a timely manner. We'll ensure they are cleared here eagerly.
     823    //
     824    // We only need to do this for a DFG/FTL CodeBlock because only these will have a
     825    // DFG:CommonData. Hence, the LLInt and Baseline will not have any of these watchpoints.
     826    //
     827    // Note also that the LLIntPrototypeLoadAdaptiveStructureWatchpoint is also related
     828    // to the CodeBlock. However, its lifecycle is tied directly to the CodeBlock, and
     829    // will be automatically cleared when the CodeBlock destructs.
     830
     831    if (JITCode::isOptimizingJIT(jitType()))
     832        jitCode()->dfgCommon()->clearWatchpoints();
     833#endif
    814834    vm.heap.codeBlockSet().remove(this);
    815835   
Note: See TracChangeset for help on using the changeset viewer.