Bug 196133

Summary: [JSC] Shrink sizeof(RegExp)
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: 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
Patch mark.lam: review+

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>