Limit number of empty chunks after a GC.

RESOLVED FIXED in Firefox 25, Firefox OS v1.1hd

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gwagner, Assigned: gwagner)

Tracking

({crash, perf})

unspecified
mozilla25
ARM
Gonk (Firefox OS)
crash, perf
Points:
---

Firefox Tracking Flags

(blocking-b2g:leo+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Created attachment 768168 [details] [diff] [review]
patch

+++ This bug was initially created as a clone of Bug #884394 +++
(Assignee)

Updated

4 years ago
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]

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
blocking-b2g: --- → leo?
Whiteboard: u=MarketPlace , [b2g-crash][MemShrink][js4b2g] → u=MarketPlace , [b2g-crash][MemShrink][js4b2g][fixed-in-birch]
(Assignee)

Updated

4 years ago
Assignee: general → anygregor
(Assignee)

Comment 3

4 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

4 years ago
https://hg.mozilla.org/mozilla-central/rev/20a2994fe7e1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Comment 5

4 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

4 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

4 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

4 years ago
Blocks: 884394
No longer depends on: 884394
(Assignee)

Comment 6

4 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

4 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? → -
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+.
(Assignee)

Comment 12

4 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.
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+
(Assignee)

Comment 15

4 years ago
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5b34e0cda635
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
(Assignee)

Updated

4 years ago
Whiteboard: [s=2013.06.28] [b2g-crash][MemShrink][js4b2g][fixed-in-birch] → [s=2013.06.28] [b2g-crash][MemShrink][js4b2g]
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

4 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.