Bug 198482 - JSScript should not keep bytecode cache in memory
Summary: JSScript should not keep bytecode cache in memory
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
Keywords: InRadar
Depends on:
Reported: 2019-06-03 05:31 PDT by Tadeu Zagallo
Modified: 2019-06-04 04:15 PDT (History)
9 users (show)

See Also:

Patch (31.12 KB, patch)
2019-06-03 05:41 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (31.11 KB, patch)
2019-06-03 07:06 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch for landing (32.57 KB, patch)
2019-06-04 03:28 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2019-06-03 05:31:55 PDT
Comment 1 Tadeu Zagallo 2019-06-03 05:41:06 PDT
Created attachment 371185 [details]
Comment 2 Tadeu Zagallo 2019-06-03 07:06:04 PDT
Created attachment 371189 [details]

Fix windows build
Comment 3 Saam Barati 2019-06-03 13:40:54 PDT
Comment on attachment 371189 [details]

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.
Comment 4 Tadeu Zagallo 2019-06-04 03:28:39 PDT
Created attachment 371264 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2019-06-04 04:14:16 PDT
Comment on attachment 371264 [details]
Patch for landing

Clearing flags on attachment: 371264

Committed r246060: <https://trac.webkit.org/changeset/246060>
Comment 6 WebKit Commit Bot 2019-06-04 04:14:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2019-06-04 04:15:21 PDT