Last Comment Bug 887652 - Limit number of empty chunks after a GC.
: Limit number of empty chunks after a GC.
Status: RESOLVED FIXED
[s=2013.06.28] [b2g-crash][MemShrink]...
: crash, perf
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- critical (vote)
: mozilla25
Assigned To: Gregor Wagner [:gwagner]
:
Mentors:
Depends on:
Blocks: 884394
  Show dependency treegraph
 
Reported: 2013-06-26 23:11 PDT by Gregor Wagner [:gwagner]
Modified: 2013-07-19 03:00 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
leo+
wontfix
wontfix
fixed
fixed
wontfix
wontfix
fixed


Attachments
patch (2.19 KB, patch)
2013-06-26 23:11 PDT, Gregor Wagner [:gwagner]
terrence: review+
Details | Diff | Splinter Review

Description Gregor Wagner [:gwagner] 2013-06-26 23:11:17 PDT
Created attachment 768168 [details] [diff] [review]
patch

+++ This bug was initially created as a clone of Bug #884394 +++
Comment 1 Terrence Cole [:terrence] 2013-06-27 10:46:17 PDT
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.
Comment 2 Justin Lebar (not reading bugmail) 2013-06-27 11:00:32 PDT
Thanks for turning this around so quickly, Gregor and Terrence.
Comment 3 Gregor Wagner [:gwagner] 2013-06-27 18:53:07 PDT
(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 Ed Morley [:emorley] 2013-06-28 07:02:28 PDT
https://hg.mozilla.org/mozilla-central/rev/20a2994fe7e1
Comment 5 Mike Lee [:mlee] 2013-06-28 08:45:18 PDT
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
Comment 6 Gregor Wagner [:gwagner] 2013-06-29 19:21:16 PDT
(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.
Comment 7 Alex Keybl [:akeybl] 2013-07-02 16:53:00 PDT
(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.
Comment 8 Justin Lebar (not reading bugmail) 2013-07-02 17:01:16 PDT
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 Desigan Chinniah [:cyberdees] [:dees] [London - GMT] 2013-07-07 14:17:29 PDT
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 Justin Lebar (not reading bugmail) 2013-07-07 14:24:32 PDT
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 Jason Smith [:jsmith] 2013-07-07 16:43:30 PDT
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+.
Comment 12 Gregor Wagner [:gwagner] 2013-07-08 17:06:01 PDT
(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 Jason Smith [:jsmith] 2013-07-08 17:15:12 PDT
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.
Comment 14 Alex Keybl [:akeybl] 2013-07-09 16:09:56 PDT
Blocks a (new) blocker, so leo+
Comment 15 Gregor Wagner [:gwagner] 2013-07-09 16:53:47 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5b34e0cda635

Note You need to log in before you can comment on or make changes to this bug.