Bug 193401

Summary: Add API to generate and consume cached bytecode
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: JavaScriptCoreAssignee: Tadeu Zagallo <tzagallo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
WIP patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Tadeu Zagallo 2019-01-14 09:42:02 PST
Follow up on https://bugs.webkit.org/show_bug.cgi?id=192782
Comment 1 Tadeu Zagallo 2019-01-14 09:45:45 PST
Created attachment 359045 [details]
Patch
Comment 2 Tadeu Zagallo 2019-01-23 10:03:19 PST
Created attachment 359899 [details]
WIP patch
Comment 3 Tadeu Zagallo 2019-01-23 15:27:58 PST
Created attachment 359954 [details]
Patch
Comment 4 Tadeu Zagallo 2019-01-24 06:32:17 PST
Created attachment 360008 [details]
Patch
Comment 5 Tadeu Zagallo 2019-01-24 06:33:53 PST
<rdar://problem/47514099>
Comment 6 EWS Watchlist 2019-01-24 08:26:29 PST
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
Comment 7 Keith Miller 2019-01-24 10:45:02 PST
Is the jsc EWS failure legit? Seems like it's not.
Comment 8 Keith Miller 2019-01-24 12:43:41 PST
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?
Comment 9 Tadeu Zagallo 2019-01-24 13:54:33 PST
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.
Comment 10 Keith Miller 2019-01-24 13:55:42 PST
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.
Comment 11 Tadeu Zagallo 2019-01-24 14:37:30 PST
Created attachment 360040 [details]
Patch
Comment 12 Keith Miller 2019-01-24 18:14:08 PST
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?
Comment 13 Tadeu Zagallo 2019-01-25 06:52:31 PST
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.
Comment 14 Tadeu Zagallo 2019-01-25 07:58:26 PST
Created attachment 360114 [details]
Patch
Comment 15 Tadeu Zagallo 2019-01-25 08:09:15 PST
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.
Comment 16 Keith Miller 2019-01-25 08:58:22 PST
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?
Comment 17 Tadeu Zagallo 2019-01-25 13:49:37 PST
Created attachment 360158 [details]
Patch for landing
Comment 18 WebKit Commit Bot 2019-01-25 14:31:18 PST
Comment on attachment 360158 [details]
Patch for landing

Clearing flags on attachment: 360158

Committed r240511: <https://trac.webkit.org/changeset/240511>
Comment 19 WebKit Commit Bot 2019-01-25 14:31:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Keith Miller 2019-01-28 10:18:12 PST
This should probably actually be tracked with:

<rdar://problem/46084932>