Skip to content

StringImpl refcounting should be atomic #1713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
justinmichaud wants to merge 1 commit into
base: wpe-2.46
Choose a base branch
from
Open

StringImpl refcounting should be atomic #1713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Source/JavaScriptCore/bytecode/DirectEvalCodeCache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@

namespace JSC {

void DirectEvalCodeCache::setSlow(JSGlobalObject* globalObject, JSCell* owner, const String& evalSource, BytecodeIndex bytecodeIndex, DirectEvalExecutable* evalExecutable)
void DirectEvalCodeCache::setSlow(JSGlobalObject* globalObject, JSCell* owner, const CacheLookupKey& cacheKey, DirectEvalExecutable* evalExecutable)
{
if (!evalExecutable->allowDirectEvalCache())
return;

Locker locker { m_lock };
m_cacheMap.set(CacheKey(evalSource, bytecodeIndex), WriteBarrier<DirectEvalExecutable>(globalObject->vm(), owner, evalExecutable));
m_cacheMap.set(cacheKey, WriteBarrier<DirectEvalExecutable>(globalObject->vm(), owner, evalExecutable));
}

void DirectEvalCodeCache::clear()
Expand Down
71 changes: 62 additions & 9 deletions Source/JavaScriptCore/bytecode/DirectEvalCodeCache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,20 @@ namespace JSC {

class DirectEvalCodeCache {
public:
enum class RopeSuffix : uint8_t {
None,
FunctionCall
};

class CacheLookupKey;

class CacheKey {
friend class CacheLookupKey;
public:
CacheKey(const String& source, BytecodeIndex bytecodeIndex)
: m_source(source.impl())
CacheKey(StringImpl* source, BytecodeIndex bytecodeIndex, RopeSuffix ropeSuffix)
: m_source(source)
, m_bytecodeIndex(bytecodeIndex)
, m_ropeSuffix(ropeSuffix)
{
}

Expand All @@ -54,13 +63,13 @@ namespace JSC {

CacheKey() = default;

unsigned hash() const { return m_source->hash() ^ m_bytecodeIndex.asBits(); }
unsigned hash() const { return m_source->hash() + m_bytecodeIndex.asBits() + enumToUnderlyingType(m_ropeSuffix); }

bool isEmptyValue() const { return !m_source; }

bool operator==(const CacheKey& other) const
{
return m_bytecodeIndex == other.m_bytecodeIndex && WTF::equal(m_source.get(), other.m_source.get());
return m_bytecodeIndex == other.m_bytecodeIndex && m_ropeSuffix == other.m_ropeSuffix && WTF::equal(m_source.get(), other.m_source.get());
}

bool isHashTableDeletedValue() const { return m_source.isHashTableDeletedValue(); }
Expand All @@ -82,17 +91,61 @@ namespace JSC {
private:
RefPtr<StringImpl> m_source;
BytecodeIndex m_bytecodeIndex;
RopeSuffix m_ropeSuffix;
};

class CacheLookupKey {
void* operator new(size_t) = delete;

public:
CacheLookupKey(StringImpl* source, BytecodeIndex bytecodeIndex, RopeSuffix ropeSuffix)
: m_source(source)
, m_bytecodeIndex(bytecodeIndex)
, m_ropeSuffix(ropeSuffix)
{
}

CacheLookupKey() = default;

unsigned hash() const { return m_source->hash() + m_bytecodeIndex.asBits() + enumToUnderlyingType(m_ropeSuffix); }

bool operator==(const CacheKey& other) const
{
return m_bytecodeIndex == other.m_bytecodeIndex && m_ropeSuffix == other.m_ropeSuffix && WTF::equal(m_source, other.m_source.get());
}

operator CacheKey() const
{
return CacheKey(m_source, m_bytecodeIndex, m_ropeSuffix);
}

private:
SUPPRESS_UNCOUNTED_MEMBER StringImpl* m_source;
BytecodeIndex m_bytecodeIndex;
RopeSuffix m_ropeSuffix;
};

struct CacheLookupKeyHashTranslator {
static unsigned hash(const CacheLookupKey& key)
{
return key.hash();
}

static bool equal(const CacheKey& a, const CacheLookupKey& b)
{
return b == a;
}
};

DirectEvalExecutable* tryGet(const String& evalSource, BytecodeIndex bytecodeIndex)
DirectEvalExecutable* get(const CacheLookupKey& cacheKey)
{
return m_cacheMap.inlineGet(CacheKey(evalSource, bytecodeIndex)).get();
return m_cacheMap.inlineGet<CacheLookupKeyHashTranslator>(cacheKey).get();
}

void set(JSGlobalObject* globalObject, JSCell* owner, const String& evalSource, BytecodeIndex bytecodeIndex, DirectEvalExecutable* evalExecutable)
void set(JSGlobalObject* globalObject, JSCell* owner, const CacheLookupKey& cacheKey, DirectEvalExecutable* evalExecutable)
{
if (m_cacheMap.size() < maxCacheEntries)
setSlow(globalObject, owner, evalSource, bytecodeIndex, evalExecutable);
setSlow(globalObject, owner, cacheKey, evalExecutable);
}

bool isEmpty() const { return m_cacheMap.isEmpty(); }
Expand All @@ -104,7 +157,7 @@ namespace JSC {
private:
static constexpr int maxCacheEntries = 64;

void setSlow(JSGlobalObject*, JSCell* owner, const String& evalSource, BytecodeIndex, DirectEvalExecutable*);
void setSlow(JSGlobalObject*, JSCell* owner, const CacheLookupKey& cacheKey, DirectEvalExecutable*);

typedef UncheckedKeyHashMap<CacheKey, WriteBarrier<DirectEvalExecutable>, CacheKey::Hash, CacheKey::HashTraits> EvalCacheMap;
EvalCacheMap m_cacheMap;
Expand Down
29 changes: 22 additions & 7 deletions Source/JavaScriptCore/dfg/DFGOperations.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ JSC_DEFINE_JIT_OPERATION(operationObjectAssignObject, void, (JSGlobalObject* glo
auto scope = DECLARE_THROW_SCOPE(vm);

if (auto* targetObject = jsDynamicCast<JSFinalObject*>(target); targetObject && targetObject->canPerformFastPutInlineExcludingProto() && targetObject->isStructureExtensible()) {
Vector<RefPtr<UniquedStringImpl>, 8> properties;
Vector<UniquedStringImpl*, 8> properties;
MarkedArgumentBuffer values;
if (!source->staticPropertiesReified()) {
source->reifyAllStaticProperties(globalObject);
Expand Down Expand Up @@ -351,7 +351,7 @@ JSC_DEFINE_JIT_OPERATION(operationObjectAssignUntyped, void, (JSGlobalObject* gl
OPERATION_RETURN_IF_EXCEPTION(scope);
}

Vector<RefPtr<UniquedStringImpl>, 8> properties;
Vector<UniquedStringImpl*, 8> properties;
MarkedArgumentBuffer values;
bool objectAssignFastSucceeded = objectAssignFast(globalObject, targetObject, source, properties, values);
OPERATION_RETURN_IF_EXCEPTION(scope);
Expand Down Expand Up @@ -855,10 +855,10 @@ JSC_DEFINE_JIT_OPERATION(operationGetByValObjectString, EncodedJSValue, (JSGloba

auto scope = DECLARE_THROW_SCOPE(vm);

auto propertyName = asString(string)->toIdentifier(globalObject);
auto propertyName = asString(string)->toAtomString(globalObject);
OPERATION_RETURN_IF_EXCEPTION(scope, encodedJSValue());

OPERATION_RETURN(scope, JSValue::encode(getByValObject(globalObject, vm, asObject(base), propertyName)));
OPERATION_RETURN(scope, JSValue::encode(getByValObject(globalObject, vm, asObject(base), propertyName.data)));
}

JSC_DEFINE_JIT_OPERATION(operationGetByValObjectSymbol, EncodedJSValue, (JSGlobalObject* globalObject, JSCell* base, JSCell* symbol))
Expand Down Expand Up @@ -2003,7 +2003,7 @@ JSC_DEFINE_JIT_OPERATION(operationPutByValWithThis, void, (JSGlobalObject* globa
OPERATION_RETURN(scope);
}

ALWAYS_INLINE static void defineDataProperty(JSGlobalObject* globalObject, JSObject* base, const Identifier& propertyName, JSValue value, int32_t attributes)
ALWAYS_INLINE static void defineDataProperty(JSGlobalObject* globalObject, JSObject* base, PropertyName propertyName, JSValue value, int32_t attributes)
{
PropertyDescriptor descriptor = toPropertyDescriptor(value, jsUndefined(), jsUndefined(), DefinePropertyAttributes(attributes));
ASSERT((descriptor.attributes() & PropertyAttribute::Accessor) || (!descriptor.isAccessorDescriptor()));
Expand Down Expand Up @@ -2047,7 +2047,7 @@ JSC_DEFINE_JIT_OPERATION(operationDefineDataPropertyStringIdent, void, (JSGlobal
CallFrame* callFrame = DECLARE_CALL_FRAME(vm);
JITOperationPrologueCallFrameTracer tracer(vm, callFrame);
auto scope = DECLARE_THROW_SCOPE(vm);
defineDataProperty(globalObject, base, Identifier::fromUid(vm, property), JSValue::decode(encodedValue), attributes);
defineDataProperty(globalObject, base, property, JSValue::decode(encodedValue), attributes);
OPERATION_RETURN(scope);
}

Expand Down Expand Up @@ -2667,7 +2667,7 @@ JSC_DEFINE_JIT_OPERATION(operationEnumeratorNextUpdatePropertyName, JSString*, (

if (modeNumber == JSPropertyNameEnumerator::IndexedMode) {
if (index < enumerator->indexedLength())
OPERATION_RETURN(scope, jsString(vm, Identifier::from(vm, index).string()));
OPERATION_RETURN(scope, jsString(vm, Identifier::from(vm, index).releaseImpl()));
OPERATION_RETURN(scope, vm.smallStrings.sentinelString());
}

Expand Down Expand Up @@ -3633,6 +3633,21 @@ JSC_DEFINE_JIT_OPERATION(operationHasOwnProperty, size_t, (JSGlobalObject* globa
auto scope = DECLARE_THROW_SCOPE(vm);

JSValue key = JSValue::decode(encodedKey);

if (LIKELY(key.isString())) {
auto propertyName = asString(key)->toAtomString(globalObject);
OPERATION_RETURN_IF_EXCEPTION(scope, false);

PropertySlot slot(thisObject, PropertySlot::InternalMethodType::GetOwnProperty);
bool result = thisObject->hasOwnProperty(globalObject, propertyName.data, slot);
OPERATION_RETURN_IF_EXCEPTION(scope, false);

HasOwnPropertyCache* hasOwnPropertyCache = vm.hasOwnPropertyCache();
ASSERT(hasOwnPropertyCache);
hasOwnPropertyCache->tryAdd(slot, thisObject, propertyName.data, result);
OPERATION_RETURN(scope, result);
}

Identifier propertyName = key.toPropertyKey(globalObject);
OPERATION_RETURN_IF_EXCEPTION(scope, false);

Expand Down
9 changes: 7 additions & 2 deletions Source/JavaScriptCore/heap/GCOwnedDataScope.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#pragma once

#include "EnsureStillAliveHere.h"
#include <wtf/text/StringView.h>

namespace JSC {

Expand Down Expand Up @@ -66,6 +67,10 @@ class JSCell;

template<typename T>
struct GCOwnedDataScope {
WTF_FORBID_HEAP_ALLOCATION;
public:
GCOwnedDataScope() = default;

GCOwnedDataScope(const JSCell* cell, T value)
: owner(cell)
, data(value)
Expand All @@ -87,8 +92,8 @@ struct GCOwnedDataScope {
// Convenience conversion for String -> StringView
operator StringView() const requires (std::is_same_v<std::decay_t<T>, String>) { return data; }

const JSCell* owner;
T data;
const JSCell* owner { nullptr };
SUPPRESS_UNCOUNTED_MEMBER T data { };
};

}
37 changes: 25 additions & 12 deletions Source/JavaScriptCore/interpreter/Interpreter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,17 @@

namespace JSC {

static inline DirectEvalCodeCache::CacheLookupKey directEvalCacheKey(JSGlobalObject* globalObject, JSString* string, BytecodeIndex bytecodeIndex)
{
if (string->isRope()) {
auto rope = string->asRope();
if (auto source = rope->tryGetLHS("()"_s))
return DirectEvalCodeCache::CacheLookupKey(source, bytecodeIndex, DirectEvalCodeCache::RopeSuffix::FunctionCall);
return DirectEvalCodeCache::CacheLookupKey(rope->resolveRope(globalObject).impl(), bytecodeIndex, DirectEvalCodeCache::RopeSuffix::None);
}
return DirectEvalCodeCache::CacheLookupKey(string->getValueImpl(), bytecodeIndex, DirectEvalCodeCache::RopeSuffix::None);
}

JSValue eval(CallFrame* callFrame, JSValue thisValue, JSScope* callerScopeChain, LexicallyScopedFeatures lexicallyScopedFeatures)
{
CallFrame* callerFrame = callFrame->callerFrame();
Expand Down Expand Up @@ -123,32 +134,31 @@ JSValue eval(CallFrame* callFrame, JSValue thisValue, JSScope* callerScopeChain,
return jsUndefined();

JSValue program = callFrame->argument(0);
String programSource;
JSString* programString = nullptr;
bool isTrusted = false;
if (LIKELY(program.isString())) {
programSource = program.toWTFString(globalObject);
RETURN_IF_EXCEPTION(scope, JSValue());
} else if (Options::useTrustedTypes() && program.isObject()) {
if (LIKELY(program.isString()))
programString = asString(program);
else if (Options::useTrustedTypes() && program.isObject()) {
auto* structure = globalObject->trustedScriptStructure();
if (structure == asObject(program)->structure()) {
programSource = program.toWTFString(globalObject);
programString = program.toString(globalObject);
RETURN_IF_EXCEPTION(scope, { });
isTrusted = true;
} else {
auto code = globalObject->globalObjectMethodTable()->codeForEval(globalObject, program);
RETURN_IF_EXCEPTION(scope, { });
if (!code.isNull()) {
programSource = code;
programString = jsString(vm, code);
isTrusted = true;
}
}
}

if (programSource.isNull())
if (!programString)
return program;

if (Options::useTrustedTypes() && globalObject->requiresTrustedTypes() && !isTrusted) {
bool canCompileStrings = globalObject->globalObjectMethodTable()->canCompileStrings(globalObject, CompilationType::DirectEval, programSource, *vm.emptyList);
bool canCompileStrings = globalObject->globalObjectMethodTable()->canCompileStrings(globalObject, CompilationType::DirectEval, programString->value(globalObject).data, *vm.emptyList);
RETURN_IF_EXCEPTION(scope, { });
if (!canCompileStrings) {
throwException(globalObject, scope, createEvalError(globalObject, "Refused to evaluate a string as JavaScript because this document requires a 'Trusted Type' assignment."_s));
Expand All @@ -158,7 +168,7 @@ JSValue eval(CallFrame* callFrame, JSValue thisValue, JSScope* callerScopeChain,

TopCallFrameSetter topCallFrame(vm, callFrame);
if (!globalObject->evalEnabled()) {
globalObject->globalObjectMethodTable()->reportViolationForUnsafeEval(globalObject, programSource);
globalObject->globalObjectMethodTable()->reportViolationForUnsafeEval(globalObject, programString->value(globalObject).data);
throwException(globalObject, scope, createEvalError(globalObject, globalObject->evalDisabledErrorMessage()));
return { };
}
Expand All @@ -182,8 +192,11 @@ JSValue eval(CallFrame* callFrame, JSValue thisValue, JSScope* callerScopeChain,
else
evalContextType = EvalContextType::None;

DirectEvalExecutable* eval = callerBaselineCodeBlock->directEvalCodeCache().tryGet(programSource, bytecodeIndex);
auto cacheKey = directEvalCacheKey(globalObject, programString, bytecodeIndex);
RETURN_IF_EXCEPTION(scope, { });
DirectEvalExecutable* eval = callerBaselineCodeBlock->directEvalCodeCache().get(cacheKey);
if (!eval) {
auto programSource = programString->value(globalObject).data;
if (!(lexicallyScopedFeatures & StrictModeLexicallyScopedFeature)) {
if (programSource.is8Bit()) {
LiteralParser<LChar, JSONReviverMode::Disabled> preparser(globalObject, programSource.span8(), SloppyJSON, callerBaselineCodeBlock);
Expand All @@ -210,7 +223,7 @@ JSValue eval(CallFrame* callFrame, JSValue thisValue, JSScope* callerScopeChain,

// Skip the eval cache if tainted since another eval call could have a different taintedness.
if (sourceTaintedOrigin == SourceTaintedOrigin::Untainted)
callerBaselineCodeBlock->directEvalCodeCache().set(globalObject, callerBaselineCodeBlock, programSource, bytecodeIndex, eval);
callerBaselineCodeBlock->directEvalCodeCache().set(globalObject, callerBaselineCodeBlock, cacheKey, eval);
}

RELEASE_AND_RETURN(scope, vm.interpreter.executeEval(eval, thisValue, callerScopeChain));
Expand Down
Loading