Bug 98893 – Replace (typeof(x) != <>) with !(typeof(x) == <>)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Valery Ignatyev]

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'.

[Valery Ignatyev]

Created attachment 167986 [details]
patch with fix

[Alexey Proskuryakov]

Is this patch ready for review? If so, please mark is as "review?", so that it doesn't get overlooked.

[Build Bot]

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

[Filip Pizlo]

Comment on attachment 167986 [details]
patch with fix

It seems like you're making tests fail.

[Valery Ignatyev]

Created attachment 168201 [details]
patch second attempt

[Filip Pizlo]

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"?

[Valery Ignatyev]

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.

[Filip Pizlo]

(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").

[Valery Ignatyev]

ping

[Brent Fulgham]

This patch looks good to me.  Filip, is there anything else you want to see before this is approved?

[WebKit Review Bot]

Comment on attachment 168242 [details]
fixed patch

Rejecting attachment 168242 [details] from commit-queue.

[e-mail] 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.

[Brent Fulgham]

Committed r134634: <http://trac.webkit.org/changeset/134634>

[Brent Fulgham]

I have committed the change; please watch the build bots for any test failures created by this change.

[Brent Fulgham]

Out of curiosity, did you run any profiling to see what benefit this provided?  It would be nice to document them in this bug.

[Valery Ignatyev]

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.