Closed
Bug 887652
Opened 11 years ago
Closed 11 years ago
Limit number of empty chunks after a GC.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
People
(Reporter: gwagner, Assigned: gwagner)
References
Details
(Keywords: crash, perf, Whiteboard: [s=2013.06.28] [b2g-crash][MemShrink][js4b2g][LeoVB+])
Attachments
(1 file)
2.19 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #884394 +++
Assignee | ||
Updated•11 years ago
|
Attachment #768168 -
Attachment is patch: true
Attachment #768168 -
Attachment mime type: text/x-patch → text/plain
Attachment #768168 -
Flags: review?(wmccloskey)
Updated•11 years ago
|
Whiteboard: u=MarketPlace , [b2g-crash][MemShrink:P2] → u=MarketPlace , [b2g-crash][MemShrink]
Comment 1•11 years ago
|
||
Comment on attachment 768168 [details] [diff] [review] patch Review of attachment 768168 [details] [diff] [review]: ----------------------------------------------------------------- Stealing review, since Bill is on PTO this week. Thanks, Gregor! I've been meaning to write a patch which would aggressively decommit unused chunks in the background, but have been too busy finishing up GGC to work on it. This is a nice interim solution. r=me ::: js/src/jsgc.cpp @@ +546,5 @@ > JS_ASSERT(chunk->unused()); > JS_ASSERT(!rt->gcChunkSet.has(chunk)); > JS_ASSERT(chunk->info.age <= MAX_EMPTY_CHUNK_AGE); > + if (releaseAll || chunk->info.age == MAX_EMPTY_CHUNK_AGE || > + freeChunkCount++ > MAX_EMPTY_CHUNK_COUNT) { Brace on new line since the if() is now two lines.
Attachment #768168 -
Flags: review?(wmccloskey) → review+
Comment 2•11 years ago
|
||
Thanks for turning this around so quickly, Gregor and Terrence.
Updated•11 years ago
|
Whiteboard: u=MarketPlace , [b2g-crash][MemShrink] → u=MarketPlace , [b2g-crash][MemShrink][js4b2g]
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•11 years ago
|
blocking-b2g: --- → leo?
Whiteboard: u=MarketPlace , [b2g-crash][MemShrink][js4b2g] → u=MarketPlace , [b2g-crash][MemShrink][js4b2g][fixed-in-birch]
Assignee | ||
Updated•11 years ago
|
Assignee: general → anygregor
Assignee | ||
Comment 3•11 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #2) > Thanks for turning this around so quickly, Gregor and Terrence. It's easy if you have an analysis like https://bugzilla.mozilla.org/show_bug.cgi?id=884394#c17 :)
Comment 4•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/20a2994fe7e1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 5•11 years ago
|
||
Gregor, We need a risk assessment for this patch. We understand that it's a JavaScript Garbage Collection change triggered by the Notes+ OOM bug 884394, but since that's not a blocker this patch could be introducing significant risk to all other affected apps and components. Thanks, Triage Team
Flags: needinfo?(anygregor)
Updated•11 years ago
|
Whiteboard: u=MarketPlace , [b2g-crash][MemShrink][js4b2g][fixed-in-birch] → [s=2013.06.28 u=MarketPlace] [b2g-crash][MemShrink][js4b2g][fixed-in-birch]
Updated•11 years ago
|
Whiteboard: [s=2013.06.28 u=MarketPlace] [b2g-crash][MemShrink][js4b2g][fixed-in-birch] → [s=2013.06.28] [b2g-crash][MemShrink][js4b2g][fixed-in-birch]
Updated•11 years ago
|
Assignee | ||
Comment 6•11 years ago
|
||
(In reply to Mike Lee [:mlee] from comment #5) > Gregor, > > We need a risk assessment for this patch. > > We understand that it's a JavaScript Garbage Collection change triggered by > the Notes+ OOM bug 884394, but since that's not a blocker this patch could > be introducing significant risk to all other affected apps and components. > > Thanks, > Triage Team This patch has zero risk and it should help with OOM situations.
Flags: needinfo?(anygregor)
Comment 7•11 years ago
|
||
(In reply to Gregor Wagner [:gwagner] from comment #6) > (In reply to Mike Lee [:mlee] from comment #5) > > Gregor, > > > > We need a risk assessment for this patch. > > > > We understand that it's a JavaScript Garbage Collection change triggered by > > the Notes+ OOM bug 884394, but since that's not a blocker this patch could > > be introducing significant risk to all other affected apps and components. > > > > Thanks, > > Triage Team > > This patch has zero risk and it should help with OOM situations. Needs to have a provable impact in order to block. Apologies.
blocking-b2g: leo? → -
Comment 8•11 years ago
|
||
I'm not arguing the blocking/not blocking decision, but can you help me understand what the criteria actually are? First mlee said that we wouldn't take a risky patch because the Notes+ OOM bug is not a blocker, but by asking what the risk was, he implied that we would take a low-risk patch. But now you're saying that we need "provable impact" in order to block. That's a new criterion, and I don't know what it means. We have provable impact on bug 884394, but that isn't a blocker. Do you mean we need provable impact on a blocker? Again, I'm not arguing about whether this should be able to land. But things are a lot less frustrating for us when we have clear, consistent rules about what can and can't land.
Comment 9•11 years ago
|
||
Hello team We as well as our partners are keen to progress work on the app — and there doesn't seen to be any movement for a few days. As a check-in I wondered if we are anywhere closer to solving for the issues at hand and being able to re-test Notes+. Thanks for all your work on this. It's greatly appreciated.
Comment 10•11 years ago
|
||
Desigan, This bug has been fixed in master. There is nothing more for engineering to do here. The only remaining question is whether we're going to uplift this patch to b2g18. So far release drivers has said no. If partners need this bug fixed for v1.1, please say so and work that out with release drivers. You'll probably need to needinfo someone in order to get their attention; try Alex Keybl.
Comment 11•11 years ago
|
||
Is this required to be fixed in order to have the notes packaged app with evernote integration work properly? If so, then yes, this should be leo+.
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #11) > Is this required to be fixed in order to have the notes packaged app with > evernote integration work properly? If so, then yes, this should be leo+. The notes app has many problems and this patch fixes one of it. I don't know if it is still needed after we fix all the others problems but waiting to land this patch until all other bugs are fixed is stupid. So right now I say yes this practically zero risk patch is needed to fix some of the problems related to the notes app and probably other bad OOM crashes that we see out there. As Justin mentioned, the engineering job is done. The experts say we should take the patch but release-drivers decided different.
Comment 13•11 years ago
|
||
Originally when this was triaged, I don't think we considered the notes app impact. I'm renoming to make sure that factor is considered - the evernote integration for 1.1 has to work for preinstalled notes app.
blocking-b2g: - → leo?
Assignee | ||
Comment 15•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5b34e0cda635
Assignee | ||
Updated•11 years ago
|
Whiteboard: [s=2013.06.28] [b2g-crash][MemShrink][js4b2g][fixed-in-birch] → [s=2013.06.28] [b2g-crash][MemShrink][js4b2g]
Comment 16•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/5b34e0cda635
status-b2g-v1.1hd:
--- → fixed
status-firefox23:
--- → wontfix
status-firefox24:
--- → wontfix
status-firefox25:
--- → fixed
Updated•11 years ago
|
Whiteboard: [s=2013.06.28] [b2g-crash][MemShrink][js4b2g] → [s=2013.06.28] [b2g-crash][MemShrink][js4b2g][LeoVB+]
You need to log in
before you can comment on or make changes to this bug.
Description
•