597864 - Javascript strangely slow/ locking up browser ( and tabs)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[DmD Lj]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b7pre) Gecko/20100919 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b7pre) Gecko/20100919 Firefox/4.0b7pre

http://js1k.com/demo/766   appears to cause FF to become horribly unresponsive, to the part where it's a chore even to close the tab.


Reproducible: Always

Steps to Reproduce:
1. go to url
2.  Wait/reload (javascript not always starting on the first load?)
3.  try to change tab/close the offending tab
Actual Results:  
Horrible lag in the whole browser, refreshes not happening, tab not appearing to change.

Expected Results:  
Silky smooth animation like in Chrome,  Even if it chokes, allowing me to close the tab or navigate away.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  The refreshes not happening issue is probably due to this thing keeping the CPU very very busy and also spamming the event loop.  As long as everything is cooperatively scheduled, that's a guarantee of lossage.

For the performance, this looks like a pure JS issue.  Breakdown seems to be:

  27% in mjit-generated code
  20% in mjit::ic::GetGlobalName
   3% in PropertyTable::search under mjit::ic::GetGlobalName
  30% under stubs::GetGlobalName (about half under js_FindPropertyHelper
      filling the propcache, and the rest fullTest on the propCache, self time
      in NameOp, self time in stubs::GetGlobalName).
   9% stubs::SetName<0> calling js_SetPropertyHelper and hence propcache fills,
      js_LookupPropertyWithFlags, etc.
   7% self time in mjit::ic::SetGlobalName

and some minor stuf.  It sounds like we're managing to not hit our PICs or propcache much, to me....

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: HTML testcase with prettyprinted source

I changed the setInterval to take a function instead of a string, but that doesn't change the profile results much, so far....

Comment 3

[Boris Zbarsky [:bzbarsky]]

Are we possibly using MICs where we should be using PICs?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yeah - haven't checked yet, but perhaps either this case should devolve into a PIC, or just stop patching.

Comment 5

[Notlost]

Confiming on ubuntu 10.04 with latest nightly. thinkpad t42p

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

The reason this test is so choppy is because the author forgot to declare "j" as some kind of variable. Inside the setInterval'd function, "j" is conditionally set, but unconditionally read. 

The first 50 times it runs, it just throws a ReferenceError, visible in the JS console. After a while the condition gets hit so the code runs without throwing.

In general our ICs disable after any error, on the expectation that they won't work the next time around. We can just change that, there's no reason to disable since it won't cost performance.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: reduced test case

Okay, so it turns out we already do this. That wasn't the problem. The problem is that the global object is changing shape on every iteration of the main loop. Test case attached where we are roughly 13X slower than v8:

dvander@dvander:~/mozilla/tracemonkey/js/src$ ~/projects/v8/shell brand.js 
99
dvander@dvander:~/mozilla/tracemonkey/js/src$ ./Opt32/js -m brand.js 
1317

The problem is a loop that uses the same global variable |s| to call two different functions. The first IC hit uses the property cache, and since it's a call operation, it brands the object. Every iteration of the loop thereafter causes two shape changes on the global object, because it's flipping the method value of |s|.

A quick fix would be to stop using the property cache on the first hit of an IC, and just fetching the property manually (which would also be faster). We currently don't use branding information for global property accesses so this shouldn't matter, but it seems like we should also have some mechanism to back-off from branding.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Avoiding the property cache makes the test case run in 113ms.

Comment 9

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 10

[Brendan Eich [:brendan]]

This patch adds a method thrash count in two bits of JSObject::flags, and aggressively unbrands and prevents future branding if any method shape change causes a branded object's method thrash count to reach the maximum value.

/be

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 492537 [details] [diff] [review]
proposed fix

>+    if (branded()) {
>+        generateOwnShape(cx);
>+        if (js_IsPropertyCacheDisabled(cx))  // check for rt->shapeGen overflow
>+            return false;

This won't report any error, is that intended?

Looks good otherwise, test case is within 10ms of v8 now!

Comment 12

[Robert Sayre]

Any reason not to land this?

Comment 13

[Brendan Eich [:brendan]]

Attachment: updated to use brand and unbrand as infallible, with comments

dvander reviewed this and raised the issue, which was a botch -- shape overflow is not a failure condition, rather a don't-fill-prop-cache and bail-off-trace condition.

/be

Comment 14

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/1002cba2f2d6

/be

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1002cba2f2d6