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 Bugs | Assignee: | 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
Benjamin Poulain
2015-06-04 16:46:05 PDT
Created attachment 254317 [details]
Patch
Created attachment 254319 [details]
Patch
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 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() Committed r185240: <http://trac.webkit.org/changeset/185240> (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 . |