Closed Bug 597864 Opened 13 years ago Closed 13 years ago

Javascript strangely slow/ locking up browser ( and tabs)


(Core :: JavaScript Engine, defect, P1)




Tracking Status
blocking2.0 --- betaN+


(Reporter: spider, Assigned: brendan)


(Keywords: perf, Whiteboard: fixed-in-tracemonkey)


(3 files, 1 obsolete file)

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   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.
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....
Assignee: nobody → general
Component: General → JavaScript Engine
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → general
I changed the setInterval to take a function instead of a string, but that doesn't change the profile results much, so far....
Are we possibly using MICs where we should be using PICs?
Yeah - haven't checked yet, but perhaps either this case should devolve into a PIC, or just stop patching.
Confiming on ubuntu 10.04 with latest nightly. thinkpad t42p
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Assignee: general → dvander
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.
Attached file 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 
dvander@dvander:~/mozilla/tracemonkey/js/src$ ./Opt32/js -m brand.js 

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.
Avoiding the property cache makes the test case run in 113ms.
Attachment #491969 - Attachment is patch: false
Attached patch proposed fix (obsolete) — Splinter Review
Assignee: dvander → brendan
Attachment #492537 - Flags: review?(dvander)
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.

OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All
Target Milestone: --- → mozilla2.0b8
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!
Attachment #492537 - Flags: review?(dvander) → review+
Any reason not to land this?
Keywords: perf
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.

Attachment #492537 - Attachment is obsolete: true
Attachment #497000 - Flags: review+

Whiteboard: fixed-in-tracemonkey
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.