Bug 20818 - 'instanceof' operator is slow
Summary: 'instanceof' operator is slow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Maciej Stachowiak
URL:
Keywords:
Depends on:
Blocks: 20813
  Show dependency treegraph
 
Reported: 2008-09-12 17:30 PDT by Cameron Zwarich (cpst)
Modified: 2008-09-23 20:48 PDT (History)
0 users

See Also:


Attachments
enable PIC of the "prototype" property lookup, this saves half the time... (2.23 KB, patch)
2008-09-14 18:52 PDT, Maciej Stachowiak
no flags Details | Formatted Diff | Diff
enable PIC of the "prototype" property lookup (this time for sure) (11.85 KB, patch)
2008-09-14 19:01 PDT, Maciej Stachowiak
no flags Details | Formatted Diff | Diff
replace implementsHasInstance virtual call with a TypeInfo flag (15.21 KB, patch)
2008-09-22 07:55 PDT, Maciej Stachowiak
no flags Details | Formatted Diff | Diff
use a TypeInfo bit to inline hasInfo and do the checks in a smarter order in the common case (15.44 KB, patch)
2008-09-22 19:24 PDT, Maciej Stachowiak
no flags Details | Formatted Diff | Diff
inline JIT the fast paths of instanceof (8.10 KB, patch)
2008-09-23 10:02 PDT, Maciej Stachowiak
zwarich: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-09-12 17:30:10 PDT
JSObject::hasInstance() shows up as 3.1% of the Shark profile for the V8 Earley-Boyer benchmark, so we should try to speed it up a bit. The isObject() check in the op_instanceof also shows up as 2.4% of the Shark profile, but I will make a separate bug for that.
Comment 1 Maciej Stachowiak 2008-09-14 18:52:39 PDT
Created attachment 23427 [details]
enable PIC of the "prototype" property lookup, this saves half the time...
Comment 2 Maciej Stachowiak 2008-09-14 18:53:49 PDT
(This will probably require more work to save the rest of the time).
Comment 3 Maciej Stachowiak 2008-09-14 19:01:28 PDT
Created attachment 23428 [details]
enable PIC of the "prototype" property lookup (this time for sure)
Comment 4 Cameron Zwarich (cpst) 2008-09-14 19:07:01 PDT
Comment on attachment 23428 [details]
enable PIC of the "prototype" property lookup (this time for sure)

r=me

You have a double newline on line 818 of CodeGenerator.cpp.
Comment 5 Maciej Stachowiak 2008-09-14 19:23:19 PDT
Comment on attachment 23428 [details]
enable PIC of the "prototype" property lookup (this time for sure)

Unflagging and obsoleting because this has been landed.
Comment 6 Maciej Stachowiak 2008-09-22 07:55:07 PDT
Created attachment 23650 [details]
replace implementsHasInstance virtual call with a TypeInfo flag
Comment 7 Cameron Zwarich (cpst) 2008-09-22 08:00:59 PDT
Comment on attachment 23650 [details]
replace implementsHasInstance virtual call with a TypeInfo flag

r=me, assuming you've removed the tab in CodeGeneratorJS.pm and you put the speedup in the ChangeLog.
Comment 8 David Kilzer (:ddkilzer) 2008-09-22 11:23:02 PDT
(In reply to comment #7)
> (From update of attachment 23650 [details] [edit])
> r=me, assuming you've removed the tab in CodeGeneratorJS.pm and you put the
> speedup in the ChangeLog.

http://trac.webkit.org/changeset/36766

Comment 9 Maciej Stachowiak 2008-09-22 19:20:06 PDT
Reopening since there is more speedup yet to come.

Comment 10 Maciej Stachowiak 2008-09-22 19:20:26 PDT
Comment on attachment 23650 [details]
replace implementsHasInstance virtual call with a TypeInfo flag

Unflag and obsolete this patch since it ahs been landed.
Comment 11 Maciej Stachowiak 2008-09-22 19:24:59 PDT
Created attachment 23690 [details]
use a TypeInfo bit to inline hasInfo and do the checks in a smarter order in the common case
Comment 12 Darin Adler 2008-09-22 19:49:12 PDT
Comment on attachment 23690 [details]
use a TypeInfo bit to inline hasInfo and do the checks in a smarter order in the common case

From the caller's point of view, customHasInstance is still just hasInstance, so I'm not sure it was an improvement to rename it.

Since you wrote different hasInstance code in cti_op_instanceof, there's no need for the standardHasInstance inline function, so you could have also just left it  alone.

You could have named the flag OverridesHasInstance.

This:

    valueCell->structureID()->typeInfo().type() != ObjectType

Is this same as !isObject(). Why did you write it the longer way?

r=me with or without changes
Comment 13 Maciej Stachowiak 2008-09-23 10:02:33 PDT
Created attachment 23705 [details]
inline JIT the fast paths of instanceof

I am not sure it is a good idea to emit quite so much code inline for instanceof.
Comment 14 Cameron Zwarich (cpst) 2008-09-23 10:10:17 PDT
Comment on attachment 23705 [details]
inline JIT the fast paths of instanceof

Your ASSERT on Machine.cpp:4203 is in the wrong style; the ||'s should be on the beginning of the lines, not the end.

Other than that, r=me.
Comment 15 Darin Adler 2008-09-23 20:48:28 PDT
OK, now I believe this is done.