Last Comment Bug 1154339 - JS microbenchmark with JQuery proxy is much slower when inner callback function uses tabs instead of spaces for indentation
: JS microbenchmark with JQuery proxy is much slower when inner callback functi...
Status: RESOLVED FIXED
[platform-rel-jQuery]
: reproducible, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 37 Branch
: All All
-- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 1227035
Blocks:
  Show dependency treegraph
 
Reported: 2015-04-14 09:31 PDT by Andrew
Modified: 2016-08-09 02:22 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments

Description User image Andrew 2015-04-14 09:31:01 PDT
Please open http://jsfiddle.net/1qhsL84w/1/ than open firebug, press "run". The result will be "5-15".
Please open http://jsfiddle.net/1qhsL84w/2/ than open firebug, press "run". The result will be "0".

There are 2 scripts:
http://googledrive.com/host/0B11zLXPSukRzdmYyRndDQlRhT00 is original jquery-1.10.2
http://googledrive.com/host/0B11zLXPSukRzZTNyeUVDNVVldGc is a bit modified jquery-1.10.2

--- 0B11zLXPSukRzdmYyRndDQlRhT00
+++ 0B11zLXPSukRzZTNyeUVDNVVldGc
@@ -824,7 +824,7 @@
 		// Simulated bind
 		args = core_slice.call( arguments, 2 );
 		proxy = function() {
-			return fn.apply( context || this, args.concat( core_slice.call( arguments ) ) );
+            return fn.apply( context || this, args.concat( core_slice.call( arguments ) ) );
 		};

Original jquery-1.10.2 have "\t\t\t" (3 tabs), modified have "            " (12 spaces).

This looks like a serious bug in your js engine. How white space in script can affect the performance of this script?
Comment 1 User image Andrew 2015-04-14 10:23:58 PDT
Please checkout http://jsfiddle.net/1qhsL84w/3/ and http://jsfiddle.net/1qhsL84w/4/ too. The difference is significant more.
Comment 2 User image Andrew 2015-04-14 11:56:32 PDT
You can disable "script" tab in firebug and bug will disappear. It looks like "script" tab uses some buggy method from javascript SDK for addons.
Comment 3 User image :Gijs (away until Feb 27) 2016-01-27 15:34:53 PST
Can you reproduce this still, on an up-to-date Firefox, with the builtin developer tools?
Comment 4 User image Andrew 2016-03-10 14:52:55 PST
Yes. I've installed new firefox without any extensions in new clean windows 7 virtual machine.

Please click here: http://jsfiddle.net/1qhsL84w/7/ and http://jsfiddle.net/1qhsL84w/6/
I have 234 and 50 miliseconds. The diff between these scripts is 3 tabs and 12 spaces.
Comment 5 User image :Gijs (away until Feb 27) 2016-03-11 02:54:05 PST
I'm flummoxed. Jan, can you or someone else on the JS team take a look? You don't need firebug - I can reproduce on plain 45 release as well as Nightly (both on win8)
Comment 6 User image Nicolas B. Pierron [:nbp] 2016-03-11 03:05:57 PST
I will forward this issue to jorendorff, I think he might know better the frontend of SpiderMonkey.
Comment 7 User image Nicolas B. Pierron [:nbp] 2016-03-11 04:50:27 PST
I am able to reproduce this issue even when the jit are disabled.
Comment 8 User image Jason Orendorff [:jorendorff] 2016-03-11 05:19:58 PST
You have got to be kidding me.
Comment 9 User image Jason Orendorff [:jorendorff] 2016-03-11 05:23:34 PST
> This looks like a serious bug in your js engine. How white space in script can affect the performance
> of this script?

This is extremely surprising. I can't imagine what would cause this. Looking into it.
Comment 10 User image Jan de Mooij [:jandem] 2016-03-11 05:25:04 PST
This is likely caused by the last check in ObjectGroup::useSingletonForClone :/
Comment 11 User image Andrew 2016-03-11 05:37:44 PST
I had a bug that website work extremely slow only in firefox. I've figured out that jquery.proxy was a bottleneck. I've upgraded jquery to latest version and the bug worked perfect. I've pretty printed jquery.js with spaces and got ready to ... but the bug disappeared. Than I've started to replace spaces with tabs line by line and found that the problem was in tabs around line:

> return fn.apply( context || this, args.concat( core_slice.call( arguments ) ) );

This looks absurdly, but it works.

PS Firebug's script tab somehow amplifies this bug.
Comment 12 User image Jason Orendorff [:jorendorff] 2016-03-11 05:52:02 PST
Andrew, we have an optimization for fn.apply() that only kicks in if the function that is calling fn.apply() is very short: 100 characters or less.

Does this agree with what you've observed?

(We do this because the cost of the optimization itself is proportional to the size of that function - the "optimized" version can even be slower in some cases. Admittedly measuring how long a function is by counting characters is goofy.)
Comment 13 User image Jason Orendorff [:jorendorff] 2016-03-11 05:54:27 PST
The fact that the tabs version is slower here makes me think the "optimization" is backfiring. It would make sense, as the microbenchmark creates many instances of this function.
Comment 14 User image Jason Orendorff [:jorendorff] 2016-03-11 05:59:20 PST
Jan has a patch that might help here. Assigning the bug to him.
Comment 15 User image Jan de Mooij [:jandem] 2016-03-11 06:13:55 PST
(In reply to Jason Orendorff [:jorendorff] from comment #14)
> Jan has a patch that might help here. Assigning the bug to him.

Bug 1227035. I wrote a patch for it yesterday, weird coincidence.
Comment 16 User image Boris Zbarsky [:bz] (still a bit busy) 2016-07-19 07:05:00 PDT
I this still an issue?
Comment 17 User image philipp 2016-07-19 14:05:50 PDT
I just tested with the jsFiddles from comment #4 and got consistent results in both independent of the developer tools being open.

Firefox 47.0.1 under Linux
Comment 18 User image Jan de Mooij [:jandem] 2016-08-09 02:21:56 PDT
(In reply to Boris Zbarsky [:bz] from comment #16)
> I this still an issue?

The patch for bug 1227035 should have fixed this one. The bad TI heuristic is still there; unfortunately fixing that will require more thought/work.

Note You need to log in before you can comment on or make changes to this bug.