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

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>