WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
145673
[JSC] Always track out-of-bounds array access explicitly instead of relying on the slow case
https://bugs.webkit.org/show_bug.cgi?id=145673
Summary
[JSC] Always track out-of-bounds array access explicitly instead of relying o...
Benjamin Poulain
Reported
2015-06-04 16:46:05 PDT
[JSC] Always track out-of-bounds array access explicitly instead of relying on the slow case
Attachments
Patch
(62.33 KB, patch)
2015-06-04 17:06 PDT
,
Benjamin Poulain
no flags
Details
Formatted Diff
Diff
Patch
(62.34 KB, patch)
2015-06-04 17:39 PDT
,
Benjamin Poulain
ggaren
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Benjamin Poulain
Comment 1
2015-06-04 17:06:44 PDT
Created
attachment 254317
[details]
Patch
Benjamin Poulain
Comment 2
2015-06-04 17:39:49 PDT
Created
attachment 254319
[details]
Patch
Geoffrey Garen
Comment 3
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.
Benjamin Poulain
Comment 4
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()
Benjamin Poulain
Comment 5
2015-06-04 22:21:09 PDT
Committed
r185240
: <
http://trac.webkit.org/changeset/185240
>
Csaba Osztrogonác
Comment 6
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
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug