[webkit-reviews] review granted: [Bug 198482] JSScript should not keep bytecode cache in memory : [Attachment 371189] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jun 3 13:40:54 PDT 2019

Saam Barati <sbarati at apple.com> has granted Tadeu Zagallo
<tzagallo at apple.com>'s request for review:
Bug 198482: JSScript should not keep bytecode cache in memory

Attachment 371189: Patch


--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 371189
  --> https://bugs.webkit.org/attachment.cgi?id=371189

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


> Source/JavaScriptCore/runtime/CachedTypes.cpp:176
> +    Ref<CachedBytecode> releaseMapped(BytecodeCacheError& error)

Kinda weird this returns ref instead of ptr. Why not RefPtr and make nullptr
mean there was an error? I feel like it's weird to construct an empty
CachedBytecode here.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:180
> +	       error.standardError(errno);

nit: This style feels kinda weird. Why not just make error something you assign
to? And by default it's empty? And you can make this a static function that is
a factory constructor. I feel like that's more in line with the style of error
handling I've seen elsewhere in WK. So something like:
error = BytecodeCacheError::standardError(errno)
return nullptr 

> Source/JavaScriptCore/runtime/CachedTypes.cpp:2411
> +    Encoder encoder(vm, -1);

You could also make -1 the default argument of this function.

More information about the webkit-reviews mailing list