Title: [220777] trunk/Source/_javascript_Core
- Revision
- 220777
- Author
- sbar...@apple.com
- Date
- 2017-08-15 17:03:45 -0700 (Tue, 15 Aug 2017)
Log Message
Make VM::scratchBufferForSize thread safe
https://bugs.webkit.org/show_bug.cgi?id=175604
Reviewed by Geoffrey Garen and Mark Lam.
I want to use the VM::scratchBufferForSize in another patch I'm writing.
The use case for my other patch is to call it from the compiler thread.
When reading the code, I saw that this API was not thread safe. This patch
makes it thread safe. It actually turns out we were calling this API from
the compiler thread already when we created FTL::State for an FTL OSR entry
compilation, and from FTLLowerDFGToB3. That code was racy and wrong, but
is now correct with this patch.
* runtime/VM.cpp:
(JSC::VM::VM):
(JSC::VM::~VM):
(JSC::VM::gatherConservativeRoots):
(JSC::VM::scratchBufferForSize):
* runtime/VM.h:
(JSC::VM::scratchBufferForSize): Deleted.
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (220776 => 220777)
--- trunk/Source/_javascript_Core/ChangeLog 2017-08-16 00:02:51 UTC (rev 220776)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-08-16 00:03:45 UTC (rev 220777)
@@ -1,3 +1,26 @@
+2017-08-15 Saam Barati <sbar...@apple.com>
+
+ Make VM::scratchBufferForSize thread safe
+ https://bugs.webkit.org/show_bug.cgi?id=175604
+
+ Reviewed by Geoffrey Garen and Mark Lam.
+
+ I want to use the VM::scratchBufferForSize in another patch I'm writing.
+ The use case for my other patch is to call it from the compiler thread.
+ When reading the code, I saw that this API was not thread safe. This patch
+ makes it thread safe. It actually turns out we were calling this API from
+ the compiler thread already when we created FTL::State for an FTL OSR entry
+ compilation, and from FTLLowerDFGToB3. That code was racy and wrong, but
+ is now correct with this patch.
+
+ * runtime/VM.cpp:
+ (JSC::VM::VM):
+ (JSC::VM::~VM):
+ (JSC::VM::gatherConservativeRoots):
+ (JSC::VM::scratchBufferForSize):
+ * runtime/VM.h:
+ (JSC::VM::scratchBufferForSize): Deleted.
+
2017-08-15 Keith Miller <keith_mil...@apple.com>
JSC named bytecode offsets should use references rather than pointers
Modified: trunk/Source/_javascript_Core/runtime/VM.cpp (220776 => 220777)
--- trunk/Source/_javascript_Core/runtime/VM.cpp 2017-08-16 00:02:51 UTC (rev 220776)
+++ trunk/Source/_javascript_Core/runtime/VM.cpp 2017-08-16 00:03:45 UTC (rev 220777)
@@ -194,7 +194,6 @@
, symbolImplToSymbolMap(*this)
, prototypeMap(*this)
, interpreter(0)
- , sizeOfLastScratchBuffer(0)
, entryScope(0)
, m_regExpCache(new RegExpCache(this))
#if ENABLE(REGEXP_TRACING)
@@ -423,8 +422,8 @@
#endif
#if ENABLE(DFG_JIT)
- for (unsigned i = 0; i < scratchBuffers.size(); ++i)
- fastFree(scratchBuffers[i]);
+ for (unsigned i = 0; i < m_scratchBuffers.size(); ++i)
+ fastFree(m_scratchBuffers[i]);
#endif
}
@@ -746,7 +745,8 @@
#if ENABLE(DFG_JIT)
void VM::gatherConservativeRoots(ConservativeRoots& conservativeRoots)
{
- for (auto* scratchBuffer : scratchBuffers) {
+ auto lock = holdLock(m_scratchBufferLock);
+ for (auto* scratchBuffer : m_scratchBuffers) {
if (scratchBuffer->activeLength()) {
void* bufferStart = scratchBuffer->dataBuffer();
conservativeRoots.add(bufferStart, static_cast<void*>(static_cast<char*>(bufferStart) + scratchBuffer->activeLength()));
@@ -1008,4 +1008,27 @@
}
#endif // USE(CF)
+ScratchBuffer* VM::scratchBufferForSize(size_t size)
+{
+ if (!size)
+ return nullptr;
+
+ auto locker = holdLock(m_scratchBufferLock);
+
+ if (size > m_sizeOfLastScratchBuffer) {
+ // Protect against a N^2 memory usage pathology by ensuring
+ // that at worst, we get a geometric series, meaning that the
+ // total memory usage is somewhere around
+ // max(scratch buffer size) * 4.
+ m_sizeOfLastScratchBuffer = size * 2;
+
+ ScratchBuffer* newBuffer = ScratchBuffer::create(m_sizeOfLastScratchBuffer);
+ RELEASE_ASSERT(newBuffer);
+ m_scratchBuffers.append(newBuffer);
+ }
+
+ ScratchBuffer* result = m_scratchBuffers.last();
+ return result;
+}
+
} // namespace JSC
Modified: trunk/Source/_javascript_Core/runtime/VM.h (220776 => 220777)
--- trunk/Source/_javascript_Core/runtime/VM.h 2017-08-16 00:02:51 UTC (rev 220776)
+++ trunk/Source/_javascript_Core/runtime/VM.h 2017-08-16 00:03:45 UTC (rev 220777)
@@ -567,33 +567,13 @@
Instruction* targetInterpreterPCForThrow;
uint32_t osrExitIndex;
void* osrExitJumpDestination;
- Vector<ScratchBuffer*> scratchBuffers;
- size_t sizeOfLastScratchBuffer;
-
bool isExecutingInRegExpJIT { false };
- ScratchBuffer* scratchBufferForSize(size_t size)
- {
- if (!size)
- return 0;
+ // The threading protocol here is as follows:
+ // - You can call scratchBufferForSize from any thread.
+ // - You can only set the ScratchBuffer's activeLength from the main thread.
+ ScratchBuffer* scratchBufferForSize(size_t size);
- if (size > sizeOfLastScratchBuffer) {
- // Protect against a N^2 memory usage pathology by ensuring
- // that at worst, we get a geometric series, meaning that the
- // total memory usage is somewhere around
- // max(scratch buffer size) * 4.
- sizeOfLastScratchBuffer = size * 2;
-
- ScratchBuffer* newBuffer = ScratchBuffer::create(sizeOfLastScratchBuffer);
- RELEASE_ASSERT(newBuffer);
- scratchBuffers.append(newBuffer);
- }
-
- ScratchBuffer* result = scratchBuffers.last();
- result->setActiveLength(0);
- return result;
- }
-
EncodedJSValue* exceptionFuzzingBuffer(size_t size)
{
ASSERT(Options::useExceptionFuzz());
@@ -815,6 +795,9 @@
std::unique_ptr<TypeProfilerLog> m_typeProfilerLog;
unsigned m_typeProfilerEnabledCount;
bool m_needToFirePrimitiveGigacageEnabled { false };
+ Lock m_scratchBufferLock;
+ Vector<ScratchBuffer*> m_scratchBuffers;
+ size_t m_sizeOfLastScratchBuffer { 0 };
InlineWatchpointSet m_primitiveGigacageEnabled;
FunctionHasExecutedCache m_functionHasExecutedCache;
std::unique_ptr<ControlFlowProfiler> m_controlFlowProfiler;
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
