Bug 127750

Summary: FTL should support GetById(Untyped:)
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, mark.lam, mhahnenberg, mmirman, msaboff, nrotem, oliver, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 127830, 127746    
Attachments:
Description Flags
work in progress
none
more
none
the patch
none
the patch oliver: review+

Filip Pizlo
Reported 2014-01-27 18:12:29 PST
...
Attachments
work in progress (6.49 KB, patch)
2014-01-28 22:43 PST, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
more (23.26 KB, patch)
2014-01-29 20:42 PST, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (34.82 KB, patch)
2014-01-29 22:25 PST, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (37.72 KB, patch)
2014-01-29 23:18 PST, Filip Pizlo 		oliver: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2014-01-28 22:43:38 PST
Created attachment 222555 [details] work in progress
Filip Pizlo
Comment 2 2014-01-28 22:49:15 PST
Heh, this reveals some bugs: ** The following JSC stress test failures have been introduced: internal-js-tests.yaml/V8v7/deltablue.js.ftl-no-cjit internal-js-tests.yaml/V8v7/deltablue.js.default-ftl sunspider-1.0/access-fannkuch.js.ftl-no-cjit-validate sunspider-1.0/access-fannkuch.js.ftl-no-cjit-osr-validation sunspider-1.0/access-fannkuch.js.ftl-eager-no-cjit sunspider-1.0/access-fannkuch.js.ftl-eager-no-cjit-osr-validation internal-js-tests.yaml/Kraken/stanford-crypto-aes.js.ftl-no-cjit v8-v6/v8-deltablue.js.ftl-eager-no-cjit v8-v6/v8-deltablue.js.ftl-eager-no-cjit-osr-validation I am investigating.
Filip Pizlo
Comment 3 2014-01-29 19:48:17 PST
Lol. The bug is that LLVM may duplicate code that does patchpoints and stackmaps. Our code gets confused by this, because we incorrectly assume that for a patchpoint/stackmap that we emit in IR, LLVM will only generate one copy in the resulting code. That's clearly bogus. This will require some fun reworking. Since GetById(Untyped:) is relative straight-forward, I think I will just include the fix in the patch. It'll take a while.
Filip Pizlo
Comment 4 2014-01-29 20:42:25 PST
Created attachment 222624 [details] more Working on a fix
Filip Pizlo
Comment 5 2014-01-29 22:25:52 PST
Created attachment 222626 [details] the patch
Filip Pizlo
Comment 6 2014-01-29 23:18:31 PST
Created attachment 222629 [details] the patch
Geoffrey Garen
Comment 7 2014-01-30 10:59:08 PST
Comment on attachment 222629 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=222629&action=review > Source/JavaScriptCore/ftl/FTLStackMaps.h:91 > + typedef HashMap<uint32_t, Vector<Record>, WTF::IntHash<uint32_t>, WTF::UnsignedWithZeroKeyHashTraits<uint32_t>> RecordMap; Since the common case is one entry, can we use Vector<Record, 1>? That will substantially reduce malloc traffic, with very little space overhead.
Filip Pizlo
Comment 8 2014-01-30 13:19:05 PST
(In reply to comment #7) > (From update of attachment 222629 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222629&action=review > > > Source/JavaScriptCore/ftl/FTLStackMaps.h:91 > > + typedef HashMap<uint32_t, Vector<Record>, WTF::IntHash<uint32_t>, WTF::UnsignedWithZeroKeyHashTraits<uint32_t>> RecordMap; > > Since the common case is one entry, can we use Vector<Record, 1>? That will substantially reduce malloc traffic, with very little space overhead. Good point! I will make that change.
Filip Pizlo
Comment 9 2014-01-30 14:55:50 PST
Landed in http://trac.webkit.org/changeset/163119
Note You need to log in before you can comment on or make changes to this bug.