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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: Andrew, Assigned: jandem)

Tracking

({reproducible, testcase})

37 Branch
reproducible, testcase
Points:
---

Firefox Tracking Flags

(platform-rel +)

Details

(Whiteboard: [platform-rel-jQuery])

(Reporter)

Description

2 years ago
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?
(Reporter)

Comment 1

2 years ago
Please checkout http://jsfiddle.net/1qhsL84w/3/ and http://jsfiddle.net/1qhsL84w/4/ too. The difference is significant more.
(Reporter)

Comment 2

2 years ago
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

a year ago
Can you reproduce this still, on an up-to-date Firefox, with the builtin developer tools?
Component: General → Untriaged
Flags: needinfo?(aladjev.andrew)
(Reporter)

Comment 4

a year ago
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.
Flags: needinfo?(aladjev.andrew)

Comment 5

a year ago
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)
Status: UNCONFIRMED → NEW
Component: Untriaged → JavaScript Engine
Ever confirmed: true
Flags: needinfo?(jdemooij)
Keywords: reproducible, testcase
Product: Firefox → Core
Summary: JQuery proxy is extremely slow with firebug opened → JS microbenchmark with JQuery proxy is much slower when inner callback function uses tabs instead of spaces for indentation
I will forward this issue to jorendorff, I think he might know better the frontend of SpiderMonkey.
Flags: needinfo?(jdemooij) → needinfo?(jorendorff)
I am able to reproduce this issue even when the jit are disabled.
You have got to be kidding me.
> 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.
Assignee: nobody → jorendorff
Flags: needinfo?(jorendorff)
(Assignee)

Comment 10

a year ago
This is likely caused by the last check in ObjectGroup::useSingletonForClone :/
(Reporter)

Comment 11

a year ago
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.
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.)
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.
Jan has a patch that might help here. Assigning the bug to him.
Assignee: jorendorff → jdemooij
(Assignee)

Comment 15

a year ago
(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.
Depends on: 1227035
Whiteboard: [platform-rel-jQuery]

Updated

10 months ago
platform-rel: --- → ?
platform-rel: ? → +
I this still an issue?
Flags: needinfo?(jdemooij)

Comment 17

9 months ago
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
(Assignee)

Comment 18

9 months ago
(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.
Flags: needinfo?(jdemooij)
(Assignee)

Updated

9 months ago
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.