Bug 127750 – FTL should support GetById(Untyped:)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

...

[Filip Pizlo]

Created attachment 222555 [details]
work in progress

[Filip Pizlo]

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]

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]

Created attachment 222624 [details]
more

Working on a fix

[Filip Pizlo]

Created attachment 222626 [details]
the patch

[Filip Pizlo]

Created attachment 222629 [details]
the patch

[Geoffrey Garen]

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]

(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]

Landed in http://trac.webkit.org/changeset/163119