Bug 145673

Summary: [JSC] Always track out-of-bounds array access explicitly instead of relying on the slow case
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: ossy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 145754    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch ggaren: review+

Description Benjamin Poulain 2015-06-04 16:46:05 PDT
[JSC] Always track out-of-bounds array access explicitly instead of relying on the slow case
Comment 1 Benjamin Poulain 2015-06-04 17:06:44 PDT
Created attachment 254317 [details]
Patch
Comment 2 Benjamin Poulain 2015-06-04 17:39:49 PDT
Created attachment 254319 [details]
Patch
Comment 3 Geoffrey Garen 2015-06-04 18:02:43 PDT
Comment on attachment 254319 [details]
Patch

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

r=me

How much of a speedup is this worth?

> Source/JavaScriptCore/ChangeLog:41
> +        There are a few important cases that are still not covered correctly:

When you say "correctly" I think you might be talking about a bug. I think you meant "optimally".

> Source/JavaScriptCore/bytecode/ArrayProfile.h:213
> +    void setOutOfBounds() { m_outOfBounds = true; }

When I first read this, I though "out of bounds" meant "past length". But now I see that it actually means "past length or a hole value". I wonder if we can use a clearer name for this. How about "sawHole"? Technically, things past length are holes, just like unfilled items within length.

> Source/JavaScriptCore/jit/JITOperations.cpp:1481
> +static bool canUseFastArgumentAccess(JSObject& object, uint32_t index)

I think I would call this "canAccessArgumentIndexQuickly" to match the function name in DirectArguments and ScopedArguments.

I agree that it's surprising that arguments objects have different notions of fast indexed access as compared to other objects, but I think it's probably OK.

> Source/JavaScriptCore/jit/JITOperations.cpp:1484
> +    // For Arguments, we don't have holes creating out-of-bounds access, but we have "overrodeThings()" that
> +    // moves us to slow path.

I think this comment is slightly inaccurate: see below.

> Source/JavaScriptCore/jit/JITOperations.cpp:1489
> +        if (!directArguments->overrodeThings() && index < directArguments->internalLength())

Can we just call DirectArguments::canAccessIndexQuickly here instead? Seems like better encapsulation that way.

> Source/JavaScriptCore/jit/JITOperations.cpp:1495
> +        if (!scopedArguments->overrodeThings() && index < scopedArguments->internalLength())

Same comment. In this case, from reading ScopedArguments::canAccessIndexQuickly, I think this test is slightly wrong from an optimization perspective. ScopeArguments much check m_table to verify that an argument is stored inside the ScopedArguments object. In the DFG, failing this check will cause an OSR exit. So, it would be good to use a generic byVal operation instead of optimized access in the case where the m_table entry was present.
Comment 4 Benjamin Poulain 2015-06-04 18:23:00 PDT
Comment on attachment 254319 [details]
Patch

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

>> Source/JavaScriptCore/jit/JITOperations.cpp:1489
>> +        if (!directArguments->overrodeThings() && index < directArguments->internalLength())
> 
> Can we just call DirectArguments::canAccessIndexQuickly here instead? Seems like better encapsulation that way.

After discussing with Geoff, looks like we should have Arguments::canAccessArgumentIndexQuicklyInDFG()
Comment 5 Benjamin Poulain 2015-06-04 22:21:09 PDT
Committed r185240: <http://trac.webkit.org/changeset/185240>
Comment 6 Csaba Osztrogon√°c 2015-06-08 08:21:40 PDT
(In reply to comment #5)
> Committed r185240: <http://trac.webkit.org/changeset/185240>

It broke the ARM Linux build, new bug report with the fix: bug145754 .