Bug 156182 – PolymorphicAccess should have a MegamorphicLoad case

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming

[Filip Pizlo]

Created attachment 275580 [details]
patch that does not work

It generates code for MegamorphicAccess but that code then crashes.

OTOH the other changes in this patch pass tests and are perf-neutral (like changing repatch heuristics and hashing).

[Filip Pizlo]

It now works for a simple test:

                          TipOfTree                   Things                                      

megamorphic-load       29.3178+-2.9974     ^     16.4813+-1.2412        ^ definitely 1.7789x faster

[Filip Pizlo]

Created attachment 275628 [details]
the patch

Not yet ready for review since I am not yet done benchmarking it.

[WebKit Commit Bot]

Attachment 275628 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:154:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

(In reply to comment #4)
> Attachment 275628 [details] did not pass style-queue:
> 
> 
> ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:154:  The parameter name
> "object" adds no information, so it should be removed. 

False.

> [readability/parameter_name] [5]
> Total errors found: 1 in 20 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

[Keith Miller]

Looks like some of the builds are unhappy with your patch.

[Filip Pizlo]

(In reply to comment #6)
> Looks like some of the builds are unhappy with your patch.

Yeah, I haven't compiled/tested 32-bit yet.

[Filip Pizlo]

Created attachment 275676 [details]
the patch

Fixes 32-bit-only link error.

[WebKit Commit Bot]

Attachment 275676 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:154:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 275676 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275676&action=review

r=me

> Source/JavaScriptCore/ChangeLog:22
> +        pathology only makes sense if it also preserves the good performance of the fast case, or reduces the
> +        likelihood of the worst-case by so much that it's a win for the average case even with a slow-down in
> +        the fast case.

Interesting. I was just looking at the same problem for bmalloc.

I think doubleHash() was originally partially a defense against the combination of: (a) hash functions that weren't very good at mixing the low bits, and (b) hash tables where the key was expensive to compare, like a WTF::String. But maybe those problems are in the past.

Maybe we should make the same change in WTF::HashTable, which also doubleHashes.

[Keith Miller]

Comment on attachment 275676 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275676&action=review

r=me too.

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:531
> +        UNUSED_PARAM(loop);

Seems unnecessary.

[Filip Pizlo]

(In reply to comment #11)
> Comment on attachment 275676 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275676&action=review
> 
> r=me too.
> 
> > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:531
> > +        UNUSED_PARAM(loop);
> 
> Seems unnecessary.

Ooops I will remove.

[Filip Pizlo]

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