Closed
Bug 597864
Opened 15 years ago
Closed 14 years ago
Javascript strangely slow/ locking up browser ( and tabs)
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla2.0b8
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: spider, Assigned: brendan)
Details
(Keywords: perf, Whiteboard: fixed-in-tracemonkey)
Attachments
(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
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•15 years ago
|
||
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
Status: UNCONFIRMED → NEW
Component: General → JavaScript Engine
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → general
![]() |
||
Comment 2•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
blocking2.0: --- → ?
Updated•15 years ago
|
blocking2.0: ? → betaN+
![]() |
||
Updated•14 years ago
|
Assignee: general → dvander
Status: NEW → ASSIGNED
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.
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.
Avoiding the property cache makes the test case run in 113ms.
Assignee | ||
Updated•14 years ago
|
Attachment #491969 -
Attachment is patch: false
Assignee | ||
Comment 9•14 years ago
|
||
Assignee: dvander → brendan
Attachment #492537 -
Flags: review?(dvander)
Assignee | ||
Comment 10•14 years ago
|
||
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
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+
Comment 12•14 years ago
|
||
Any reason not to land this?
Assignee | ||
Comment 13•14 years ago
|
||
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
Attachment #492537 -
Attachment is obsolete: true
Attachment #497000 -
Flags: review+
Assignee | ||
Comment 14•14 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Comment 15•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•