Closed Bug 887652 Opened 11 years ago Closed 11 years ago

Limit number of empty chunks after a GC.

Categories

(Core :: JavaScript Engine, defect)

ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla25
blocking-b2g leo+
Tracking Status
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: gwagner, Assigned: gwagner)

References

Details

(Keywords: crash, perf, Whiteboard: [s=2013.06.28] [b2g-crash][MemShrink][js4b2g][LeoVB+])

Attachments

(1 file)

Attached patch patchSplinter Review
+++ This bug was initially created as a clone of Bug #884394 +++
Attachment #768168 - Attachment is patch: true
Attachment #768168 - Attachment mime type: text/x-patch → text/plain
Attachment #768168 - Flags: review?(wmccloskey)
Whiteboard: u=MarketPlace , [b2g-crash][MemShrink:P2] → u=MarketPlace , [b2g-crash][MemShrink]
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+
Thanks for turning this around so quickly, Gregor and Terrence.
Whiteboard: u=MarketPlace , [b2g-crash][MemShrink] → u=MarketPlace , [b2g-crash][MemShrink][js4b2g]
Status: NEW → ASSIGNED
blocking-b2g: --- → leo?
Whiteboard: u=MarketPlace , [b2g-crash][MemShrink][js4b2g] → u=MarketPlace , [b2g-crash][MemShrink][js4b2g][fixed-in-birch]
Assignee: general → anygregor
(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 :)
https://hg.mozilla.org/mozilla-central/rev/20a2994fe7e1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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)
Whiteboard: u=MarketPlace , [b2g-crash][MemShrink][js4b2g][fixed-in-birch] → [s=2013.06.28 u=MarketPlace] [b2g-crash][MemShrink][js4b2g][fixed-in-birch]
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]
Blocks: 884394
No longer depends on: 884394
(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)
(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? → -
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.
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.
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.
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+.
(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.
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?
Blocks a (new) blocker, so leo+
blocking-b2g: leo? → leo+
Whiteboard: [s=2013.06.28] [b2g-crash][MemShrink][js4b2g][fixed-in-birch] → [s=2013.06.28] [b2g-crash][MemShrink][js4b2g]
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.