Cache bytecode for jsc.cpp helpers and fix CachedStringImpl
authortzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Apr 2019 15:41:07 +0000 (15:41 +0000)
committertzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Apr 2019 15:41:07 +0000 (15:41 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196409

Reviewed by Saam Barati.

JSTests:

* stress/bytecode-cache-cached-string-impl.js: Added.
(f):
(g):
* stress/bytecode-cache-run-string.js: Added.

Source/JavaScriptCore:

Some of the helpers in jsc.cpp, such as `functionRunString`, were stll using
using `makeSource` instead of `jscSource`, which does not use the ShellSourceProvider
and therefore does not write the bytecode cache to disk.

Changing that revealed a bug in bytecode cache. The Encoder keeps a mapping
of pointers to offsets of already cached objects, in order to avoid caching
the same object twice. Similarly, the Decoder keeps a mapping from offsets
to pointers, in order to avoid creating multiple objects in memory for the
same cached object. The following was happening:
1) A StringImpl* S was cached as CachedPtr<CachedStringImpl> at offset O. We add
an entry in the Encoder mapping that S has already been encoded at O.
2) We cache StringImpl* S again, but now as CachedPtr<CachedUniquedStringImpl>.
We find an entry in the Encoder mapping for S, and return the offset O. However,
the object cached at O is a CachedPtr<CachedStringImpl> (i.e. not Uniqued).

3) When decoding, there are 2 possibilities:
3.1) We find S for the first time through a CachedPtr<CachedStringImpl>. In
this case, everything works as expected since we add an entry in the decoder
mapping from the offset O to the decoded StringImpl* S. The next time we find
S through the uniqued version, we'll return the already decoded S.
3.2) We find S through a CachedPtr<CachedUniquedStringImpl>. Now we have a
problem, since the CachedPtr has the offset of a CachedStringImpl (not uniqued),
which has a different shape and we crash.

We fix this by making CachedStringImpl and CachedUniquedStringImpl share the
same implementation. Since it doesn't matter whether a string is uniqued for
encoding, and we always decode strings as uniqued either way, they can be used
interchangeably.

* jsc.cpp:
(functionRunString):
(functionLoadString):
(functionDollarAgentStart):
(functionCheckModuleSyntax):
(runInteractive):
* runtime/CachedTypes.cpp:
(JSC::CachedUniquedStringImplBase::decode const):
(JSC::CachedFunctionExecutable::rareData const):
(JSC::CachedCodeBlock::rareData const):
(JSC::CachedFunctionExecutable::encode):
(JSC::CachedCodeBlock<CodeBlockType>::encode):
(JSC::CachedUniquedStringImpl::encode): Deleted.
(JSC::CachedUniquedStringImpl::decode const): Deleted.
(JSC::CachedStringImpl::encode): Deleted.
(JSC::CachedStringImpl::decode const): Deleted.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@243869 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/bytecode-cache-cached-string-impl.js [new file with mode: 0644]
JSTests/stress/bytecode-cache-run-string.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/jsc.cpp
Source/JavaScriptCore/runtime/CachedTypes.cpp

index 7ab9550..f8f912a 100644 (file)
@@ -1,3 +1,15 @@
+2019-04-04  Tadeu Zagallo  <tzagallo@apple.com>
+
+        Cache bytecode for jsc.cpp helpers and fix CachedStringImpl
+        https://bugs.webkit.org/show_bug.cgi?id=196409
+
+        Reviewed by Saam Barati.
+
+        * stress/bytecode-cache-cached-string-impl.js: Added.
+        (f):
+        (g):
+        * stress/bytecode-cache-run-string.js: Added.
+
 2019-04-03  Robin Morisset  <rmorisset@apple.com>
 
         B3 should use associativity to optimize expression trees
diff --git a/JSTests/stress/bytecode-cache-cached-string-impl.js b/JSTests/stress/bytecode-cache-cached-string-impl.js
new file mode 100644 (file)
index 0000000..056479f
--- /dev/null
@@ -0,0 +1,20 @@
+//@ runBytecodeCache
+
+function f() {
+    let x = {};
+    x.foo = "foo";
+}
+function g() {
+    switch ('foo') {
+        case "foo":
+            break;
+        case "bar":
+        case "baz":
+        default:
+            throw new Error('Should have jumped to `foo`');
+            break;
+    }
+}
+
+g();
+f();
diff --git a/JSTests/stress/bytecode-cache-run-string.js b/JSTests/stress/bytecode-cache-run-string.js
new file mode 100644 (file)
index 0000000..6c7dfd5
--- /dev/null
@@ -0,0 +1,3 @@
+//@ runBytecodeCache
+
+runString('undefined');
index 5f0273f..e733b05 100644 (file)
@@ -1,5 +1,58 @@
 2019-04-04  Tadeu Zagallo  <tzagallo@apple.com>
 
+        Cache bytecode for jsc.cpp helpers and fix CachedStringImpl
+        https://bugs.webkit.org/show_bug.cgi?id=196409
+
+        Reviewed by Saam Barati.
+
+        Some of the helpers in jsc.cpp, such as `functionRunString`, were stll using
+        using `makeSource` instead of `jscSource`, which does not use the ShellSourceProvider
+        and therefore does not write the bytecode cache to disk.
+
+        Changing that revealed a bug in bytecode cache. The Encoder keeps a mapping
+        of pointers to offsets of already cached objects, in order to avoid caching
+        the same object twice. Similarly, the Decoder keeps a mapping from offsets
+        to pointers, in order to avoid creating multiple objects in memory for the
+        same cached object. The following was happening:
+        1) A StringImpl* S was cached as CachedPtr<CachedStringImpl> at offset O. We add
+        an entry in the Encoder mapping that S has already been encoded at O.
+        2) We cache StringImpl* S again, but now as CachedPtr<CachedUniquedStringImpl>.
+        We find an entry in the Encoder mapping for S, and return the offset O. However,
+        the object cached at O is a CachedPtr<CachedStringImpl> (i.e. not Uniqued).
+
+        3) When decoding, there are 2 possibilities:
+        3.1) We find S for the first time through a CachedPtr<CachedStringImpl>. In
+        this case, everything works as expected since we add an entry in the decoder
+        mapping from the offset O to the decoded StringImpl* S. The next time we find
+        S through the uniqued version, we'll return the already decoded S.
+        3.2) We find S through a CachedPtr<CachedUniquedStringImpl>. Now we have a
+        problem, since the CachedPtr has the offset of a CachedStringImpl (not uniqued),
+        which has a different shape and we crash.
+
+        We fix this by making CachedStringImpl and CachedUniquedStringImpl share the
+        same implementation. Since it doesn't matter whether a string is uniqued for
+        encoding, and we always decode strings as uniqued either way, they can be used
+        interchangeably.
+
+        * jsc.cpp:
+        (functionRunString):
+        (functionLoadString):
+        (functionDollarAgentStart):
+        (functionCheckModuleSyntax):
+        (runInteractive):
+        * runtime/CachedTypes.cpp:
+        (JSC::CachedUniquedStringImplBase::decode const):
+        (JSC::CachedFunctionExecutable::rareData const):
+        (JSC::CachedCodeBlock::rareData const):
+        (JSC::CachedFunctionExecutable::encode):
+        (JSC::CachedCodeBlock<CodeBlockType>::encode):
+        (JSC::CachedUniquedStringImpl::encode): Deleted.
+        (JSC::CachedUniquedStringImpl::decode const): Deleted.
+        (JSC::CachedStringImpl::encode): Deleted.
+        (JSC::CachedStringImpl::decode const): Deleted.
+
+2019-04-04  Tadeu Zagallo  <tzagallo@apple.com>
+
         UnlinkedCodeBlock constructor from cache should initialize m_didOptimize
         https://bugs.webkit.org/show_bug.cgi?id=196396
 
index 6fd1f6a..6336642 100644 (file)
@@ -1459,7 +1459,7 @@ EncodedJSValue JSC_HOST_CALL functionRunString(ExecState* exec)
         vm, Identifier::fromString(globalObject->globalExec(), "arguments"), array);
 
     NakedPtr<Exception> exception;
-    evaluate(globalObject->globalExec(), makeSource(source, exec->callerSourceOrigin()), JSValue(), exception);
+    evaluate(globalObject->globalExec(), jscSource(source, exec->callerSourceOrigin()), JSValue(), exception);
 
     if (exception) {
         scope.throwException(globalObject->globalExec(), exception);
@@ -1499,7 +1499,7 @@ EncodedJSValue JSC_HOST_CALL functionLoadString(ExecState* exec)
     JSGlobalObject* globalObject = exec->lexicalGlobalObject();
 
     NakedPtr<Exception> evaluationException;
-    JSValue result = evaluate(globalObject->globalExec(), makeSource(sourceCode, exec->callerSourceOrigin()), JSValue(), evaluationException);
+    JSValue result = evaluate(globalObject->globalExec(), jscSource(sourceCode, exec->callerSourceOrigin()), JSValue(), evaluationException);
     if (evaluationException)
         throwException(exec, scope, evaluationException);
     return JSValue::encode(result);
@@ -1843,7 +1843,7 @@ EncodedJSValue JSC_HOST_CALL functionDollarAgentStart(ExecState* exec)
                     
                     NakedPtr<Exception> evaluationException;
                     JSValue result;
-                    result = evaluate(globalObject->globalExec(), makeSource(sourceCode, SourceOrigin("worker"_s)), JSValue(), evaluationException);
+                    result = evaluate(globalObject->globalExec(), jscSource(sourceCode, SourceOrigin("worker"_s)), JSValue(), evaluationException);
                     if (evaluationException)
                         result = evaluationException->value();
                     checkException(globalObject->globalExec(), globalObject, true, evaluationException, result, commandLine, success);
@@ -2185,7 +2185,7 @@ EncodedJSValue JSC_HOST_CALL functionCheckModuleSyntax(ExecState* exec)
     stopWatch.start();
 
     ParserError error;
-    bool validSyntax = checkModuleSyntax(exec, makeSource(source, { }, URL(), TextPosition(), SourceProviderSourceType::Module), error);
+    bool validSyntax = checkModuleSyntax(exec, jscSource(source, { }, URL(), TextPosition(), SourceProviderSourceType::Module), error);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
     stopWatch.stop();
 
@@ -2624,7 +2624,7 @@ static void runInteractive(GlobalObject* globalObject)
                 break;
             source = source + String::fromUTF8(line);
             source = source + '\n';
-            checkSyntax(vm, makeSource(source, sourceOrigin), error);
+            checkSyntax(vm, jscSource(source, sourceOrigin), error);
             if (!line[0]) {
                 free(line);
                 break;
@@ -2640,7 +2640,7 @@ static void runInteractive(GlobalObject* globalObject)
         
         
         NakedPtr<Exception> evaluationException;
-        JSValue returnValue = evaluate(globalObject->globalExec(), makeSource(source, sourceOrigin), JSValue(), evaluationException);
+        JSValue returnValue = evaluate(globalObject->globalExec(), jscSource(source, sourceOrigin), JSValue(), evaluationException);
 #else
         printf("%s", interactivePrompt);
         Vector<char, 256> line;
index af042a2..3fc6702 100644 (file)
@@ -575,7 +575,8 @@ private:
     CachedVector<CachedPair<Key, Value>> m_entries;
 };
 
-class CachedUniquedStringImpl : public VariableLengthObject<UniquedStringImpl> {
+template<typename T>
+class CachedUniquedStringImplBase : public VariableLengthObject<T> {
 public:
     void encode(Encoder& encoder, const StringImpl& string)
     {
@@ -631,8 +632,8 @@ public:
         }
 
         if (m_is8Bit)
-            return create(this->buffer<LChar>());
-        return create(this->buffer<UChar>());
+            return create(this->template buffer<LChar>());
+        return create(this->template buffer<UChar>());
     }
 
 private:
@@ -642,21 +643,8 @@ private:
     unsigned m_length;
 };
 
-class CachedStringImpl : public VariableLengthObject<StringImpl> {
-public:
-    void encode(Encoder& encoder, const StringImpl& impl)
-    {
-        m_uniquedStringImpl.encode(encoder, impl);
-    }
-
-    StringImpl* decode(Decoder& decoder) const
-    {
-        return m_uniquedStringImpl.decode(decoder);
-    }
-
-private:
-    CachedUniquedStringImpl m_uniquedStringImpl;
-};
+class CachedUniquedStringImpl : public CachedUniquedStringImplBase<UniquedStringImpl> { };
+class CachedStringImpl : public CachedUniquedStringImplBase<StringImpl> { };
 
 class CachedString : public VariableLengthObject<String> {
 public:
@@ -1572,7 +1560,7 @@ public:
     Identifier ecmaName(Decoder& decoder) const { return m_ecmaName.decode(decoder); }
     Identifier inferredName(Decoder& decoder) const { return m_inferredName.decode(decoder); }
 
-    UnlinkedFunctionExecutable::RareData* rareData(Decoder& decoder) const { return m_rareData.decodeAsPtr(decoder); }
+    UnlinkedFunctionExecutable::RareData* rareData(Decoder& decoder) const { return m_rareData.decode(decoder); }
 
     const CachedWriteBarrier<CachedFunctionCodeBlock, UnlinkedFunctionCodeBlock>& unlinkedCodeBlockForCall() const { return m_unlinkedCodeBlockForCall; }
     const CachedWriteBarrier<CachedFunctionCodeBlock, UnlinkedFunctionCodeBlock>& unlinkedCodeBlockForConstruct() const { return m_unlinkedCodeBlockForConstruct; }
@@ -1602,7 +1590,7 @@ private:
     unsigned m_superBinding : 1;
     unsigned m_derivedContextType: 2;
 
-    CachedOptional<CachedFunctionExecutableRareData> m_rareData;
+    CachedPtr<CachedFunctionExecutableRareData> m_rareData;
 
     CachedIdentifier m_name;
     CachedIdentifier m_ecmaName;
@@ -1655,7 +1643,7 @@ public:
     SourceParseMode parseMode() const { return m_parseMode; }
     unsigned codeType() const { return m_codeType; }
 
-    UnlinkedCodeBlock::RareData* rareData(Decoder& decoder) const { return m_rareData.decodeAsPtr(decoder); }
+    UnlinkedCodeBlock::RareData* rareData(Decoder& decoder) const { return m_rareData.decode(decoder); }
 
 private:
     VirtualRegister m_thisRegister;
@@ -1690,7 +1678,7 @@ private:
 
     CachedMetadataTable m_metadata;
 
-    CachedOptional<CachedCodeBlockRareData> m_rareData;
+    CachedPtr<CachedCodeBlockRareData> m_rareData;
 
     CachedString m_sourceURLDirective;
     CachedString m_sourceMappingURLDirective;
@@ -1955,7 +1943,7 @@ ALWAYS_INLINE void CachedFunctionExecutable::encode(Encoder& encoder, const Unli
     m_superBinding = executable.m_superBinding;
     m_derivedContextType = executable.m_derivedContextType;
 
-    m_rareData.encode(encoder, executable.m_rareData);
+    m_rareData.encode(encoder, executable.m_rareData.get());
 
     m_name.encode(encoder, executable.name());
     m_ecmaName.encode(encoder, executable.ecmaName());
@@ -2060,7 +2048,7 @@ ALWAYS_INLINE void CachedCodeBlock<CodeBlockType>::encode(Encoder& encoder, cons
         m_linkTimeConstants[i] = codeBlock.m_linkTimeConstants[i];
 
     m_metadata.encode(encoder, codeBlock.m_metadata.get());
-    m_rareData.encode(encoder, codeBlock.m_rareData);
+    m_rareData.encode(encoder, codeBlock.m_rareData.get());
 
     m_sourceURLDirective.encode(encoder, codeBlock.m_sourceURLDirective.impl());
     m_sourceMappingURLDirective.encode(encoder, codeBlock.m_sourceURLDirective.impl());