Bug 158911 – InlineAccess should do StringLength

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 281668 [details]
patch

[Keith Miller]

Comment on attachment 281668 [details]
patch

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

r=me with comments.

> Source/JavaScriptCore/bytecode/InlineAccess.cpp:305
> +    auto branchToSlowPath = jit.patchableBranch8(
> +        CCallHelpers::NotEqual,
> +        CCallHelpers::Address(base, JSCell::typeInfoTypeOffset()),
> +        CCallHelpers::TrustedImm32(StringType));

I think it would be helpful here to put a comment saying the the patchableBranch is used to avoid branch compaction.

[Yusuke Suzuki]

Are you planning landing this patch? I think InlinedAccessCase for StringLength is profitable.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #3)
> Are you planning landing this patch? I think InlinedAccessCase for
> StringLength is profitable.

I forgot about this! I'll rebase and land it soon.

[Saam Barati]

Will try to get this landed tomorrow.

[Saam Barati]

Created attachment 348456 [details]
patch

[EWS Watchlist]

Attachment 348456 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/InlineAccess.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/InlineAccess.cpp:52:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 348460 [details]
patch

[Yusuke Suzuki]

Comment on attachment 348460 [details]
patch

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

r=me

> Source/JavaScriptCore/bytecode/InlineAccess.cpp:173
> +    GPRReg base = stubInfo.baseGPR();

Nice improvement.

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:201
>  #endif

I think we can use GPRReg directly here (instead of int8_t), since they are `enum : int8_t` now!

[Saam Barati]

Comment on attachment 348460 [details]
patch

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

>> Source/JavaScriptCore/bytecode/StructureStubInfo.h:201
>>  #endif
> 
> I think we can use GPRReg directly here (instead of int8_t), since they are `enum : int8_t` now!

Let's do this as a follow up for all these fields.

[Saam Barati]

(In reply to Saam Barati from comment #10)
> Comment on attachment 348460 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348460&action=review
> 
> >> Source/JavaScriptCore/bytecode/StructureStubInfo.h:201
> >>  #endif
> > 
> > I think we can use GPRReg directly here (instead of int8_t), since they are `enum : int8_t` now!
> 
> Let's do this as a follow up for all these fields.

https://bugs.webkit.org/show_bug.cgi?id=189166

[Mark Lam]

Comment on attachment 348460 [details]
patch

FWIW, r=me too.

[WebKit Commit Bot]

Comment on attachment 348460 [details]
patch

Clearing flags on attachment: 348460

Committed r235517: <https://trac.webkit.org/changeset/235517>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

This patch broke my ARMv7 linux WPE build:

Source/JavaScriptCore/bytecode/InlineAccess.cpp:53:13: error: 'class JSC::CCallHelpers' has no member named 'patchableBranch8'; did you mean 'patchableBranch32'?
         jit.patchableBranch8(
             ^~~~~~~~~~~~~~~~
             patchableBranch32
In file included from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource29.cpp:5:0:
Source/JavaScriptCore/bytecode/InlineAccess.cpp: In static member function 'static bool JSC::InlineAccess::generateStringLength(JSC::StructureStubInfo&)':
/home/phil/workspace/sources/wpewebkit/Source/JavaScriptCore/bytecode/InlineAccess.cpp:298:33: error: 'class JSC::CCallHelpers' has no member named 'patchableBranch8'; did you mean 'patchableBranch32'?
     auto branchToSlowPath = jit.patchableBranch8(
                                 ^~~~~~~~~~~~~~~~
                                 patchableBranch32

[Radar WebKit Bug Importer]

<rdar://problem/43935465>

[Philippe Normand]

The JSCOnly ARMv7 build bots also expose this issue.

[Radar WebKit Bug Importer]

<rdar://problem/43935775>

[Mark Lam]

<rdar://problem/43935465>

[Mark Lam]

ARMv7 speculative build fix landed in r235557: <https://trac.webkit.org/r235557>.