Bug 196133 - [JSC] Shrink sizeof(RegExp)
Summary: [JSC] Shrink sizeof(RegExp)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-21 23:59 PDT by Yusuke Suzuki
Modified: 2019-03-23 17:49 PDT (History)
6 users (show)

See Also:

Attachments
Patch (14.36 KB, patch)
2019-03-22 00:04 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (14.41 KB, patch)
2019-03-22 11:44 PDT, Yusuke Suzuki 		mark.lam: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-03-21 23:59:17 PDT 
[JSC] Shrink sizeof(RegExp)
Comment 1 Yusuke Suzuki 2019-03-22 00:04:43 PDT 
Created attachment 365685 [details]
Patch
Comment 2 Yusuke Suzuki 2019-03-22 11:44:09 PDT 
Created attachment 365752 [details]
Patch
Comment 3 Mark Lam 2019-03-22 13:19:06 PDT 
Comment on attachment 365752 [details]
Patch

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

r=me with fixes.

> Source/JavaScriptCore/runtime/RegExp.cpp:266
> +        auto& jitCode = createRegExpJITCodeIfNecessary();

Let's rename createRegExpJITCodeIfNecessary() to ensureRegExpJITCode().  See below.

> Source/JavaScriptCore/runtime/RegExp.cpp:324
> +        auto& jitCode = createRegExpJITCodeIfNecessary();

Ditto: rename.

> Source/JavaScriptCore/runtime/RegExp.cpp:445
> +            break;

Should this be a RELEASE_ASSERT_NOT_REACHED()?  The pre-existing code implies that it is not expecting these cases to be possible.

> Source/JavaScriptCore/runtime/RegExp.h:164
> +    Yarr::YarrCodeBlock& createRegExpJITCodeIfNecessary()

Let's call this ensureRegExpJITCode().  I believe that is the way we typically name this idiom.
Comment 4 Yusuke Suzuki 2019-03-23 00:55:41 PDT 
Comment on attachment 365752 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/RegExp.cpp:266
>> +        auto& jitCode = createRegExpJITCodeIfNecessary();
> 
> Let's rename createRegExpJITCodeIfNecessary() to ensureRegExpJITCode().  See below.

Fixed.

>> Source/JavaScriptCore/runtime/RegExp.cpp:324
>> +        auto& jitCode = createRegExpJITCodeIfNecessary();
> 
> Ditto: rename.

Fixed.

>> Source/JavaScriptCore/runtime/RegExp.cpp:445
>> +            break;
> 
> Should this be a RELEASE_ASSERT_NOT_REACHED()?  The pre-existing code implies that it is not expecting these cases to be possible.

This function is only called for debugging. And I think we should dump null address instead if ParseError or NotCompiled appears.
I've changed jit8BitMatchOnlyAddr etc. to take an initializer, which makes content of this array zero-filled.

>> Source/JavaScriptCore/runtime/RegExp.h:164
>> +    Yarr::YarrCodeBlock& createRegExpJITCodeIfNecessary()
> 
> Let's call this ensureRegExpJITCode().  I believe that is the way we typically name this idiom.

Fixed.
Comment 5 Yusuke Suzuki 2019-03-23 00:58:10 PDT 
Committed r243408: <https://trac.webkit.org/changeset/243408>
Comment 6 Radar WebKit Bug Importer 2019-03-23 17:49:32 PDT 
<rdar://problem/49191251>