[webkit-reviews] review denied: [Bug 193401] Add API to generate and consume cached bytecode : [Attachment 360040] Patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has denied Tadeu Zagallo
<tzagallo at apple.com>'s request for review:
Bug 193401: Add API to generate and consume cached bytecode
https://bugs.webkit.org/show_bug.cgi?id=193401

Attachment 360040: Patch

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




--- Comment #12 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 360040
  --> https://bugs.webkit.org/attachment.cgi?id=360040
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 *);
> + at 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?