Bug 98893 - Replace (typeof(x) != <>) with !(typeof(x) == <>)
Summary: Replace (typeof(x) != <>) with !(typeof(x) == <>)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-10 06:12 PDT by Valery Ignatyev
Modified: 2022-02-27 23:26 PST (History)
3 users (show)

See Also:


Attachments
patch with fix (1.91 KB, patch)
2012-10-10 06:15 PDT, Valery Ignatyev
fpizlo: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch second attempt (2.13 KB, patch)
2012-10-11 05:44 PDT, Valery Ignatyev
fpizlo: review-
Details | Formatted Diff | Diff
fixed patch (2.19 KB, patch)
2012-10-11 09:36 PDT, Valery Ignatyev
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Valery Ignatyev 2012-10-10 06:12:12 PDT
Replace (typeof(x) != <"object", "undefined", ...>) with !(typeof(x) == {"object",..}). Later is_object, is_<...>  bytecode operation will be used. 

This eliminates expensive typeof implementation and allows to use DFG optimizations, which doesn't support 'typeof'.
Comment 1 Valery Ignatyev 2012-10-10 06:15:09 PDT
Created attachment 167986 [details]
patch with fix
Comment 2 Alexey Proskuryakov 2012-10-10 11:07:34 PDT
Is this patch ready for review? If so, please mark is as "review?", so that it doesn't get overlooked.
Comment 3 Build Bot 2012-10-10 15:05:35 PDT
Comment on attachment 167986 [details]
patch with fix

Attachment 167986 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14244735

New failing tests:
sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/15.1.1/15.1.1.1_NaN/S15.1.1.1_A2_T2.html
platform/mac/plugins/bindings-test-objc.html
fast/workers/worker-close.html
sputnik/Conformance/09_Type_Conversion/9.8_ToString/S9.8_A4_T2.html
fast/js/webcore-string-comparison.html
Comment 4 Filip Pizlo 2012-10-10 15:07:11 PDT
Comment on attachment 167986 [details]
patch with fix

It seems like you're making tests fail.
Comment 5 Valery Ignatyev 2012-10-11 05:44:04 PDT
Created attachment 168201 [details]
patch second attempt
Comment 6 Filip Pizlo 2012-10-11 09:25:25 PDT
Comment on attachment 168201 [details]
patch second attempt

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1060
> +        if (opcodeID == op_nstricteq)

This should be "else if"?
Comment 7 Valery Ignatyev 2012-10-11 09:36:28 PDT
Created attachment 168242 [details]
fixed patch

Thank you very much for a feedback.
Is it codestyle violation?
I didn't understand why 'else if' is necessary.
Comment 8 Filip Pizlo 2012-10-11 09:39:08 PDT
(In reply to comment #7)
> Created an attachment (id=168242) [details]
> fixed patch
> 
> Thank you very much for a feedback.
> Is it codestyle violation?
> I didn't understand why 'else if' is necessary.

1) Style.  At first, since there wasn't an "else", it seemed like both 'if' statements could be taken, which is not the case.  It makes the code less clear on first glance.

2) Performance.  We care about it.  So even in code that isn't known to be hot (though all of the bytecode generator is hot and is subject to continuous interest from the standpoint of optimization), we aim to write the code in a way that will be naturally performant especially if it's super easy to do so - like making sure that the second branch isn't taken if the first one is (by using "else").
Comment 9 Valery Ignatyev 2012-11-02 05:27:15 PDT
ping
Comment 10 Brent Fulgham 2012-11-13 10:58:09 PST
This patch looks good to me.  Filip, is there anything else you want to see before this is approved?
Comment 11 WebKit Review Bot 2012-11-14 02:31:07 PST
Comment on attachment 168242 [details]
fixed patch

Rejecting attachment 168242 [details] from commit-queue.

valery.ignatyev@ispras.ru does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 12 Brent Fulgham 2012-11-14 11:26:35 PST
Committed r134634: <http://trac.webkit.org/changeset/134634>
Comment 13 Brent Fulgham 2012-11-14 11:27:24 PST
I have committed the change; please watch the build bots for any test failures created by this change.
Comment 14 Brent Fulgham 2012-11-14 11:28:20 PST
Out of curiosity, did you run any profiling to see what benefit this provided?  It would be nice to document them in this bug.
Comment 15 Valery Ignatyev 2012-11-15 03:28:54 PST
Generally, this patch is only the first part of 'typeof' support in DFG, which I'am trying to implement in the bug https://bugs.webkit.org/show_bug.cgi?id=98898 , but still have no final success. 
I have separated this part not only by sense, but to familiarize with your patch contributing procedures.                    

We are working on performance improvement of customer's set of tests and this patch allows to use DFG on several functions. It gives 40% speedup on that tests. I can provide only reduced examples if it's necessary.