1154339 - JS microbenchmark with JQuery proxy is much slower when inner callback function uses tabs instead of spaces for indentation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew]

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

[Andrew]

Please checkout http://jsfiddle.net/1qhsL84w/3/ and http://jsfiddle.net/1qhsL84w/4/ too. The difference is significant more.

Comment 2

[Andrew]

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

[:Gijs (out for now; he/him)]

Can you reproduce this still, on an up-to-date Firefox, with the builtin developer tools?

Comment 4

[Andrew]

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

[:Gijs (out for now; he/him)]

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

[Nicolas B. Pierron [:nbp]]

I will forward this issue to jorendorff, I think he might know better the frontend of SpiderMonkey.

Comment 7

[Nicolas B. Pierron [:nbp]]

I am able to reproduce this issue even when the jit are disabled.

Comment 8

[Jason Orendorff [:jorendorff]]

You have got to be kidding me.

Comment 9

[Jason Orendorff [:jorendorff]]

> 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

[Jan de Mooij [:jandem]]

This is likely caused by the last check in ObjectGroup::useSingletonForClone :/

Comment 11

[Andrew]

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

[Jason Orendorff [:jorendorff]]

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

[Jason Orendorff [:jorendorff]]

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

[Jason Orendorff [:jorendorff]]

Jan has a patch that might help here. Assigning the bug to him.

Comment 15

[Jan de Mooij [:jandem]]

(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

[Boris Zbarsky [:bzbarsky]]

I this still an issue?

Comment 17

[philipp]

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

[Jan de Mooij [:jandem]]

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