Bug 127750 - FTL should support GetById(Untyped:)
Summary: FTL should support GetById(Untyped:)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 127830 127746
  Show dependency tree / graph
 
Reported: 2014-01-27 18:12 PST by Filip Pizlo
Modified: 2014-01-30 14:55 PST (History)
9 users (show)

See Also:

Attachments
work in progress (6.49 KB, patch)
2014-01-28 22:43 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
more (23.26 KB, patch)
2014-01-29 20:42 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
the patch (34.82 KB, patch)
2014-01-29 22:25 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
the patch (37.72 KB, patch)
2014-01-29 23:18 PST, Filip Pizlo 		oliver: review+
Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2014-01-27 18:12:29 PST 
...
Comment 1 Filip Pizlo 2014-01-28 22:43:38 PST 
Created attachment 222555 [details]
work in progress
Comment 2 Filip Pizlo 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.
Comment 3 Filip Pizlo 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.
Comment 4 Filip Pizlo 2014-01-29 20:42:25 PST 
Created attachment 222624 [details]
more

Working on a fix
Comment 5 Filip Pizlo 2014-01-29 22:25:52 PST 
Created attachment 222626 [details]
the patch
Comment 6 Filip Pizlo 2014-01-29 23:18:31 PST 
Created attachment 222629 [details]
the patch
Comment 7 Geoffrey Garen 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.
Comment 8 Filip Pizlo 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.
Comment 9 Filip Pizlo 2014-01-30 14:55:50 PST 
Landed in http://trac.webkit.org/changeset/163119