875797 - Firefox 21 take long time compiling LESS CSS runtime with less-1.3.3.min.js.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Stefano Storti]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0 (Beta/Release)
Build ID: 20130409194949

Steps to reproduce:

I'm programming a web interface with LESS CSS (compiling runtime with less-1.3.3.min.js) and testing it with Firefox.
Till Firefox v. 20 all is going right.


Actual results:

After updated to Firefox 21, compiling LESS CSS runtime blocks Firefox for abount 15 seconds.
I manually downgrade to 20.0.1 again an the problem disappear.

Comment 2

[Drew Willcoxon :adw]

It looks like by "compile" you mean that you include the LESS script in your page, include your own LESS code in the page, call a function in the LESS script, and pass it your LESS code, which produces CSS, right?

Please try to reproduce in Firefox 21 in safe mode with add-ons disabled and report back here:
https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode

Moving to Core::JavaScript Engine, since this appears to concern JS performance.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Stefano, can you please link to a page showing the problem?

Comment 4

[Stefano Storti]

I'm sorry. Seem to be a problem of Firebug 1.11.4 with Firefox 21, it's not a issue of Firefox itself.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Sounds like bug 876075, but with maybe actual steps to reproduce.  I'd still really appreciate a link to a page showing the problem so we can figure out what's going on.

Comment 6

[Stefano Storti]

You cannot reproduce it only by visiting an online page, because the issue appear not at first page load but after editing a .less file and refreshing the page. Anyway I thik it's a Firebug issue, not a Firefox one: if I disable Firebug, the problem disappear.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'm OK with steps to reproduce more complicated than "load this URL", as long as they're detailed enough to follow...

The issue seems to be in the Firefox JS debugger, which Firebug activates, per bug 876075.  So it would be very helpful to have a way to reproduce this so that fixes can be tested properly.

Comment 8

[Stefano Storti]

Attachment: Project with an html page and some LESS CSS files to test the problem

Open the project root (index.html) with your browser in localhost, activate firebug, refresh. If nothing strange happens, try to edit something in style.less file and then refresh again.
The issue is that refresh process blocks for some seconds. This didn't happen with Firefox 20.

Comment 9

[Stefano Storti]

Hi Boris and thank you for your interest in solving the bug. I attach a zip with a test project to reproduce the block while refreshing. See istruction in attachment post.

Comment 10

[Sebastian Zartner [:sebo]]

Tested the test case of comment 8 using all combinations of Firefox 20.0.1/21.0/22.0/23.0a2/24.0a1 + Firebug 1.11.0/1.11.4/1.12a6/1.12a6 (jsd2 branch) on Win7.

While I see a massive slowdown (page load takes about 10x longer) on Firefox 21.0 + Firebug 1.11.0/1.11.4/1.12a6, there is only a little slowdown on all other combinations.
Also I can confirm that this issue is related to JSD, because when disabling the Console and Script panel in Firebug I don't see any slowdown.

So maybe the related fixes could be ported to Firefox 20?

Sebastian

Comment 11

[Sebastian Zartner [:sebo]]

Just to be clear, here are my steps to reproduce:
1. Install Firebug 1.11.4 from AMO (https://addons.mozilla.org/firefox/addon/firebug/)
2. Unpack the files from the test case attached to comment 8 and load index.html
3. Open Firebug by pressing F12
4. Enable the Script and Console panel by right-clicking their tabs and choosing "Enabled" from the context menu
5. Switch to the Console panel
6. Reload the page

=> The page takes relatively long to load. (The log messages inside the Console panel indicate some timing. On my machine this is ~2.8s in comparison to ~220ms when the two panels are disabled.)

Boris, can you reproduce that?

Sebastian

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yes, I can definitely reproduce with the steps from comment 11.

I was confused by comment 10 talking about backporting to 20, given that the problem is only claimed to be present in 21...

In any case, I hope to have some idea of what caused and/or fixed the issue later tonight.

Comment 13

[Sebastian Zartner [:sebo]]

> I was confused by comment 10 talking about backporting to 20
Oops, of course I meant fixing this for 21. Sorry for the confusion!

Sebastian

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK.  So the problem first appeared back near the end of January, in this range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f18b12139151&tochange=80fed51ae074

And then it got significantly better (going from 2800ms to about 500ms on my machine) in this range in mid-March: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b052daa913c&tochange=b03bb3ce8cee

Since then it's gotten a bit faster, down into the 320ms range.

As far as the fix range goes, interesting things are bug 759585 and maybe bug 836968.

For the regression range, bug 751618 or bug 834500 or bug 824473.  

My gut feeling is that this is something to do with the zones patches, which makes changes on 21 seem somewhat unlikely...

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The patches in bug 751618 weren't supposed to do anything besides renaming some stuff, so it's not very likely that they would have made anything slower.

What's the baseline time before January? Was it less than it is now?

Comment 16

[Boris Zbarsky [:bzbarsky]]

It used to be about 250ms, so yes, less than now.

The question is whether the problem on 21 needs a chemspill release, but figuring that out requires isolating the problem... before it becomes pointless to do a chemspill.  We've already sat on this bug for 2.5 weeks out of a 6-week cycle.

Comment 17

[Alexei Naybich]

I can confirm this issue too. Takes eternally to compile in the runtime.

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexei Naybich from comment #17)
> I can confirm this issue too. Takes eternally to compile in the runtime.
Does it work for you in Firefox 22?
http://www.mozilla.org/en-US/firefox/beta/
(new fresh profile with only Firebug installed)

Honza

Comment 19

[Boris Zbarsky [:bzbarsky]]

Bisect says:

The first bad revision is:
changeset:   119865:d8af2dfc0c2a
user:        Jan de Mooij <[e-mail]>
date:        Fri Jan 25 09:49:29 2013 +0100
summary:     Bug 833817 part 3 - Replace JSStackFrame with JSAbstractFramePtr. r=luke

Comment 20

[Jan de Mooij [:jandem]]

Bug 833817 causing this is possible since it affects debug mode / JSD.

bz, how confident are you the range in which it improved from 2800 to 500 ms is correct? I see nothing there that's related (http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b052daa913c&tochange=b03bb3ce8cee)

Bug 848665 landed around that time, but was backported to Aurora (Firefox 21)...

Comment 21

[Boris Zbarsky [:bzbarsky]]

> bz, how confident are you the range in which it improved from 2800 to 500 ms is correct?

Pretty confident, and I'm bisecting it now to find what actually improved things in there.

Comment 22

[Jan de Mooij [:jandem]]

So it looks like we're spending a *ton* of time in mjit::ExpandInlineFrames. The ScriptFrameIter constructor calls that function and with bug 833817 we use a ScriptFrameIter in js::ScriptDebugPrologue / IsTopFrameConstructing.

Fortunately, the ExpandInlineFrames foot gun is gone with the removal of JM, so this is no longer a problem on trunk.

It looks like Bill's Zones patch changed ExpandInlineFrames to take a Zone instead of a Compartment, so instead of iterating all frames on the stack *once for every compartment* we changed that to *once for every zone*. The latter should be a lot faster and I think explains why this got faster.

It's very easy to avoid using ScriptFrameIter there in Firefox 21/22 (before BC landed), I will post a small patch in a sec so that we can verify that's the problem.

Comment 23

[Boris Zbarsky [:bzbarsky]]

The improvement was:

The first good revision is:
changeset:   125047:23df95aba9cd
user:        Bill McCloskey <[e-mail]>
date:        Sat Mar 16 20:36:37 2013 -0700
summary:     Bug 759585 - Zones (r=jonco,bhackett,njn,dvander,luke,bz,mccr8,bholley)

which matches comment 22.

Comment 24

[Jan de Mooij [:jandem]]

Attachment: Possible fix for Firefox 21/22

This patch is wrong for Firefox 23+ but should be safe for 21/22..

Comment 25

[Boris Zbarsky [:bzbarsky]]

Hmm.  So even on 23 if you have lots of tabs open this will be slower, then, since the main win was from all the chrome stuff being one zone?  Or did the JM removal make 23?

Comment 26

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky (:bz) from comment #25)
> Hmm.  So even on 23 if you have lots of tabs open this will be slower, then,
> since the main win was from all the chrome stuff being one zone?  Or did the
> JM removal make 23?

It did not make 23, but JM is disabled and ExpandInlineFrames does:

if (!rt->hasJaegerRuntime())
    return;

A JaegerRuntime is only created by JM when it's enabled.

Comment 27

[Jan de Mooij [:jandem]]

Comment on attachment 761720 [details] [diff] [review]
Possible fix for Firefox 21/22

Luke, this is the most minimal fix I can think of. It's safe for 22 because AbstractFramePtr always points to a StackFrame there (Baseline is in 23), so we can call isConstructing() directly.

Comment 28

[Jan de Mooij [:jandem]]

Very safe patch, should only affect Firebug users (code is not executed when no debugger is active), but fixes a pretty bad performance problem when debugging.

Comment 29

[Luke Wagner [:luke]]

Comment on attachment 761720 [details] [diff] [review]
Possible fix for Firefox 21/22

Nice

Comment 30

[Jan de Mooij [:jandem]]

Comment on attachment 761720 [details] [diff] [review]
Possible fix for Firefox 21/22

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 833817
User impact if declined: Browser freezes when debugging
Testing completed (on m-c, etc.): Patch does not apply to m-c/aurora
Risk to taking this patch (and alternatives if risky): Low, code is not executed without Firebug.
String or IDL/UUID changes made by this patch: None.

Comment 31

[Andrew McCreight [:mccr8]]

The final beta was built on Tuesday, FYI...

Comment 32

[Boris Zbarsky [:bzbarsky]]

Is this something worth chemspilling for?  It makes using firebug ... painful.

Comment 33

[Alex Keybl [:akeybl]]

(In reply to Jan de Mooij [:jandem] from comment #30)
> Risk to taking this patch (and alternatives if risky): Low, code is not
> executed without Firebug.

This doesn't feel like a critical bug if it's been with the pre-release audience for ~22wk and the release audience for 4wk before being called out.

I think we can release note this, and those affected can move to Nightly/Aurora/Beta(23) instead of staying on release.

(In reply to Andrew McCreight [:mccr8] from comment #31)
> The final beta was built on Tuesday, FYI...

It's actually Monday the final week of the cycle fyi.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> and the release audience for 4wk before being called out.

This bug and bug 876075 were reported within 10 days of the Firefox 21 release.  The thread on the firebug newsgroups about this problem started 2 days after said release.

As in, the moment people started using the release they noticed it.  It just took the Firebug developers a week or so to pin this down to a Gecko problem and report it, and it took us weeks to bother looking at the problem (why _I_ am having to bisect js engine bugs, I can't tell you).

All of which just says that our pre-release audience doesn't use Firebug and actual web developers aren't in our pre-release audience, all of which we already knew.

I think we should strongly reconsider wontfixing this...

Comment 35

[Boris Zbarsky [:bzbarsky]]

Oh, and the reason that people who use Firebug don't use our pre-release stuff is because we so often break things for that combination.

Comment 36

[Boris Zbarsky [:bzbarsky]]

I'm going to renominate, since comment 33 seems to have been based on an incorrect understanding of the timeline and of the problem's severity.

Comment 38

[Alex Keybl [:akeybl]]

(In reply to Boris Zbarsky (:bz) from comment #36)
> I'm going to renominate, since comment 33 seems to have been based on an
> incorrect understanding of the timeline and of the problem's severity.

Yes, this paints a much better picture and is a good case for uplift. Thanks for the clarification bz. Upon further review we will approve.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1e0aee8b507c

Comment 40

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
Build ID: 20130617145905

Verified as fixed on Firefox 22 beta 6 - the time needed for compiling LESS CSS runtime with less-1.3.3.min.js is significantly lower (on my machine is 385ms on Firefox 22 beta 6 compared to 3737ms on Firefox 21 beta 2).

Comment 41

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Could anyone please test if the fix solves also the problem with Firebug slowing down the whole browser?
Thanks!
Honza

Comment 42

[Simona Badau, Desktop QA]

(In reply to Jan Honza Odvarko from comment #41)
> Could anyone please test if the fix solves also the problem with Firebug
> slowing down the whole browser?
> Thanks!
> Honza

The situation has significantly improved on Firefox 22 compared with Firefox 21. But a small delay is still noticeable on Firefox 22 if Firebug is opened (I loaded the test cases that was provided in Comment 11).