Bug 156182 - PolymorphicAccess should have a MegamorphicLoad case
Summary: PolymorphicAccess should have a MegamorphicLoad case
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-04 15:09 PDT by Filip Pizlo
Modified: 2016-04-05 12:58 PDT (History)
7 users (show)

See Also:


Attachments
patch that does not work (35.79 KB, patch)
2016-04-04 15:11 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (47.11 KB, patch)
2016-04-04 20:35 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (46.75 KB, patch)
2016-04-05 11:07 PDT, Filip Pizlo
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-04-04 15:09:28 PDT
Patch forthcoming
Comment 1 Filip Pizlo 2016-04-04 15:11:38 PDT
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).
Comment 2 Filip Pizlo 2016-04-04 16:46:09 PDT
It now works for a simple test:

                          TipOfTree                   Things                                      

megamorphic-load       29.3178+-2.9974     ^     16.4813+-1.2412        ^ definitely 1.7789x faster
Comment 3 Filip Pizlo 2016-04-04 20:35:19 PDT
Created attachment 275628 [details]
the patch

Not yet ready for review since I am not yet done benchmarking it.
Comment 4 WebKit Commit Bot 2016-04-04 21:11:32 PDT
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.
Comment 5 Filip Pizlo 2016-04-05 10:57:57 PDT
(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.
Comment 6 Keith Miller 2016-04-05 10:59:41 PDT
Looks like some of the builds are unhappy with your patch.
Comment 7 Filip Pizlo 2016-04-05 11:00:15 PDT
(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.
Comment 8 Filip Pizlo 2016-04-05 11:07:33 PDT
Created attachment 275676 [details]
the patch

Fixes 32-bit-only link error.
Comment 9 WebKit Commit Bot 2016-04-05 11:09:54 PDT
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.
Comment 10 Geoffrey Garen 2016-04-05 12:48:50 PDT
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.
Comment 11 Keith Miller 2016-04-05 12:52:32 PDT
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.
Comment 12 Filip Pizlo 2016-04-05 12:53:13 PDT
(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.
Comment 13 Filip Pizlo 2016-04-05 12:58:25 PDT
Landed in http://trac.webkit.org/changeset/199069