Bug 193401 – Add API to generate and consume cached bytecode

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

Follow up on https://bugs.webkit.org/show_bug.cgi?id=192782

[Tadeu Zagallo]

Created attachment 359045 [details]
Patch

[Tadeu Zagallo]

Created attachment 359899 [details]
WIP patch

[Tadeu Zagallo]

Created attachment 359954 [details]
Patch

[Tadeu Zagallo]

Created attachment 360008 [details]
Patch

[Tadeu Zagallo]

<rdar://problem/47514099>

[EWS Watchlist]

Comment on attachment 360008 [details]
Patch

Attachment 360008 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10873229

New failing tests:
stress/ftl-get-by-id-getter-exception-interesting-live-state.js.ftl-eager-no-cjit
apiTests

[Keith Miller]

Is the jsc EWS failure legit? Seems like it's not.

[Keith Miller]

Comment on attachment 360008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360008&action=review

> Source/JavaScriptCore/API/JSScript.h:32
> +namespace JSC {
> +class JSSourceCode;
> +class Identifier;
> +};
> +

Please remove. This shouldn't use C++ features and shouldn't expose internal identifiers.

> Source/JavaScriptCore/API/JSScript.h:70
> -+ (nullable instancetype)scriptFromASCIIFile:(NSURL *)filePath inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath;
> ++ (nullable instancetype)scriptFromASCIIFile:(NSURL *)filePath inContext:(JSContext *)context withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath;
>  
>  
>  /*!
>   This is deprecated and is equivalent to scriptFromASCIIFile:inVirtualMachine:withCodeSigning:andBytecodeCache:.
>   */
> -+ (nullable instancetype)scriptFromUTF8File:(NSURL *)filePath inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath;
> ++ (nullable instancetype)scriptFromUTF8File:(NSURL *)filePath inContext:(JSContext *)context withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath;

We shouldn't make this change. If we have to make a context for now that's ok. Our API shouldn't reflect our current VM limitations if we can avoid it.

> Source/JavaScriptCore/API/JSScript.mm:53
> +    JSScript *result = [[[JSScript alloc] init] autorelease];

Aww man! That's my bad...

> Source/JavaScriptCore/runtime/Completion.cpp:119
> +    ModuleProgramExecutable* executable = ModuleProgramExecutable::create(exec, source);

Why isn't this the UnlinkedModuleProgramExecutable? Why do we need to have the linked module to cache?

[Tadeu Zagallo]

Comment on attachment 360008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360008&action=review

>> Source/JavaScriptCore/API/JSScript.h:32
>> +
> 
> Please remove. This shouldn't use C++ features and shouldn't expose internal identifiers.

Oops

>> Source/JavaScriptCore/API/JSScript.h:70
>> ++ (nullable instancetype)scriptFromUTF8File:(NSURL *)filePath inContext:(JSContext *)context withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath;
> 
> We shouldn't make this change. If we have to make a context for now that's ok. Our API shouldn't reflect our current VM limitations if we can avoid it.

Ok, I'll update it.

>> Source/JavaScriptCore/runtime/Completion.cpp:119
>> +    ModuleProgramExecutable* executable = ModuleProgramExecutable::create(exec, source);
> 
> Why isn't this the UnlinkedModuleProgramExecutable? Why do we need to have the linked module to cache?

There's no UnlinkedModuleProgramExecutable, only UnlinkedFunctionExecutable. The linked module is used for generating the bytecode in CodeCache.h, but it seems that it's mostly just used for getting the information in executableInfo(), so maybe we can remove it if you think it's worth it.

[Keith Miller]

Comment on attachment 360008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360008&action=review

>>> Source/JavaScriptCore/runtime/Completion.cpp:119
>>> +    ModuleProgramExecutable* executable = ModuleProgramExecutable::create(exec, source);
>> 
>> Why isn't this the UnlinkedModuleProgramExecutable? Why do we need to have the linked module to cache?
> 
> There's no UnlinkedModuleProgramExecutable, only UnlinkedFunctionExecutable. The linked module is used for generating the bytecode in CodeCache.h, but it seems that it's mostly just used for getting the information in executableInfo(), so maybe we can remove it if you think it's worth it.

I think it's worth it because then we no longer have a dependency on an ExecState.

[Tadeu Zagallo]

Created attachment 360040 [details]
Patch

[Keith Miller]

Comment on attachment 360040 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360040&action=review

> Source/JavaScriptCore/API/JSScriptInternal.h:39
> -OBJC_CLASS JSScript;
> +namespace JSC {
> +class JSSourceCode;
> +class Identifier;
> +};
>  
> -const String& getJSScriptSourceCode(JSScript *);
> +@interface JSScript(Internal)
> +
> +- (JSC::JSSourceCode*)jsSourceCode:(JSC::Identifier)moduleKey;
> +

I think you need to guard all this with #if OBJC_API_ENABLED

> Source/JavaScriptCore/parser/SourceProvider.h:176
> +        const CachedBytecode* m_cachedBytecode;

I think this is wrong. If the JSScript is deallocated before a CachedBytecodeSourceProvider then it will unmap the byteocde underneath this provider. I think you'll need something like JSScriptSourceProvider that has a RetainPtr to the JSScript.

Can you add a test for this?

> Source/JavaScriptCore/runtime/CodeCache.cpp:180
> +        generate(unlinkedCodeBlock->functionDecl(i), CodeForConstruct);

I think we should only do CodeForCall. This is exponential and constructors are rare in comparison. It seems like we should be hash-consing the nested unlinked code blocks, which would make this linear instead of exponential. We can do that later, however, if you add a FIXME.

> Source/JavaScriptCore/runtime/CodeCache.cpp:184
> +        generate(unlinkedCodeBlock->functionExpr(i), CodeForConstruct);

Ditto.

> Source/JavaScriptCore/runtime/CodeCache.cpp:203
> +    char filename[512];
> +    int count = snprintf(filename, 512, "%s/%u.cache", cachePath, hash);
> +    if (count < 0 || count > 512)
> +        return;

you should just use WTF::makeString for this

> Source/JavaScriptCore/runtime/CodeCache.h:151
> +        munmap(buffer, size);

Why are you unmapping the file here? It seems like we want to test closer to what the API is doing?

[Tadeu Zagallo]

Comment on attachment 360040 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360040&action=review

>> Source/JavaScriptCore/parser/SourceProvider.h:176
>> +        const CachedBytecode* m_cachedBytecode;
> 
> I think this is wrong. If the JSScript is deallocated before a CachedBytecodeSourceProvider then it will unmap the byteocde underneath this provider. I think you'll need something like JSScriptSourceProvider that has a RetainPtr to the JSScript.
> 
> Can you add a test for this?

Yeah, I think you're right, I'll fix this and update the test case

>> Source/JavaScriptCore/runtime/CodeCache.h:151
>> +        munmap(buffer, size);
> 
> Why are you unmapping the file here? It seems like we want to test closer to what the API is doing?

This is code path is not reached when testing the API. The CachedBytecode from the source provider is consumed above in line 110.

[Tadeu Zagallo]

Created attachment 360114 [details]
Patch

[Tadeu Zagallo]

Comment on attachment 360040 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360040&action=review

>>> Source/JavaScriptCore/parser/SourceProvider.h:176
>>> +        const CachedBytecode* m_cachedBytecode;
>> 
>> I think this is wrong. If the JSScript is deallocated before a CachedBytecodeSourceProvider then it will unmap the byteocde underneath this provider. I think you'll need something like JSScriptSourceProvider that has a RetainPtr to the JSScript.
>> 
>> Can you add a test for this?
> 
> Yeah, I think you're right, I'll fix this and update the test case

I added the SourceProvider to retain the JSScript, but I'm afraid I couldn't figure out a way to test it. Since the JSScript is passed to the promise resolution callback and the JSSourceCode is only extracted inside the JSAPIGlobalObject, I couldn't find a way to ensure that there's no other references to JSScript left before the accessing the cached bytecode.

[Keith Miller]

Comment on attachment 360114 [details]
Patch

r=me. I think you can test the unmapping by making a function inside one of the modules that you call after the JSScript is released. Also, can you add a test for a constructor too?

[Tadeu Zagallo]

Created attachment 360158 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 360158 [details]
Patch for landing

Clearing flags on attachment: 360158

Committed r240511: <https://trac.webkit.org/changeset/240511>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Keith Miller]

This should probably actually be tracked with:

<rdar://problem/46084932>