693807 - Assertion ((minorAllocationBudget > 0)) @ MMgc/GCPolicyManager.cpp:387

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Brent Baker]

Running the acceptance suite with float mq: 182:cee11acfbb86

Assertion failed: "((minorAllocationBudget > 0))" ("c:/jenkins/workspace/float-compile/config/debug/label/windows64/MMgc/GCPolicyManager.cpp":387)

Testcase was as3/Statements/Exceptions/.UserDefinedErrorsPackage2.as

Working on reproducing.
Windows64 debug build

Comment 1

[Brent Baker]

Assert happened again in the build system:

as3/Definitions/Classes/Ext/ExtPublicClassFin.abc : captured output: Assertion failed: "((minorAllocationBudget > 0))" ("c:/jenkins/workspace/float-compile/config/debug/label/windows/MMgc/GCPolicyManager.cpp":387)

I'll try and dig in a little more here

Comment 2

[Brent Baker]

The most recent assert (comment #1) happened on sj2swu269-win, just tracking to see if there is something about the hardware. This machine is an IaaS virtual machine.

Comment 3

[Brent Baker]

Attachment: minidump

Windows 32bit debug crashdump

This was generated with:
tamarin-redux: 6684:9e6897c7b7f6
patch queue: 318:8156a3dbb682

Combined source code in repo @ http://asteam.corp.adobe.com/hg/users/virgilp/float4/rev/ac4f5d56f83f

Comment 4

[Brent Baker]

Unblocking float/float4 as I have seen the workers-test also fail with the same issue. QRB please assign somebody to investigate this since this is starting to cause problems with the Jenkins runs and this has also been seen by an external user reported in the IRC room.

Oct. 28, 2011:
http://asteam.corp.adobe.com/irc/log/ttlogger/tamarin/20111028
20:15 <tLt> gah #3 0x0804ca4b in avmplus::_AvmAssertMsg (condition=0, message=0x8454544 "Assertion failed: \"((minorAllocationBudget > 0))\" (\"../MMgc/GCPolicyManager.cpp\":407)\n") at ../AVMPI/AvmAssert.h:72 #4 0x08362b08 in MMgc::GCPolicyManager::adjustPolicyForNextMinorCycle (this=0xb7517024) at ../MMgc/GCPolicyManager.cpp:407

Oct.31, 2011
http://asteam.corp.adobe.com/irc/log/ttlogger/tamarin/20111031
09:21 <tLt> hmm... 0x0804ca32 in avmplus::AvmAssertFail (message=0x8454544 "Assertion failed: \"((minorAllocationBudget > 0))\" (\"../MMgc/GCPolicyManager.cpp\":407)\n") at ../AVMPI/AvmAssert.h:66

The Oct. 31 conversation between tlt and Felix has more debugging information.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

If we could reproduce this on a vanilla linux build as tlt has claimed to do, that would be helpful for me.  (I find linux-on-vmware to be more usable than windows-on-vmware; that might just general to the same statement piped to sed -e s/-on-vmware//.)

Comment 6

[Lars T Hansen]

      minorAllocationBudget = int(aval)
where aval = min(INT_MAX, A())
where A = P * R / W()
where P = 0.005
      R = max(some computed value, 10*1024*1024)
      W = L_actual / ((1.0 - T) * (L_actual - 1.0))
where T = 0.6
      L_actual < 2.875 with default settings

"minorAllocationBudget" could be zero or negative only if "aval" is less than one.  That would require W() to be either negative or very large (possibly infinite).  That would require "L_actual" to be very close to one, or less than or equal to one:  If L_actual is in the range [0,1) then W is negative; if L_actual is less than zero then W is positive, if L_actual 1 then W is +Infinity.

The interesting part of the computation L_actual is this:

  double growth =
    (L_actual - 1) * (1 + timeInLastCollection/timeEndToEndLastCollection);
  if (growth > 1)
    growth = 1;
  L_actual = L_actual + growth;
  if (X != 0 && L_actual > X*L_selected)
    L_actual = X*L_selected;

(X is a constant, 1.15, and L_selected is in the range 1.125 to 2.5 with default settings.)

Clearly if "growth" were negative this could be a problem, but that requires L_actual already to be less than one or the ratio of times to be less than -1.

The two time measurements are times in ticks for collection phases, represented as doubles but originating from uint64_t variables (hence never negative).  Even if a thread migrates from one core to another and reads a bogus value (see MSDN docs for QueryPerformanceCounter) they will be positive.

At this point it looks like adding more asserts is the safest way to go to get more data about what's going on.

Comment 7

[Lars T Hansen]

The initial value of L_actual is 2.5, with default settings.

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Lars T Hansen from comment #6)
> At this point it looks like adding more asserts is the safest way to go to
> get more data about what's going on.

Okay I'll plan to swipe this bug tomorrow as a work-item to pepper the control flow here with asserts.

Comment 10

[Dan Schaffer]

I see the assertion in the workers Jenkins branch on windows 64 debug.  It seems more frequent with -Dinterp.  
Trying to test these assertions:
1) only happens on VLS images ?
2) only on Windows 7 ?
3) Is only with flags -Dinterp?
4) Is only on Workers shell? 

I tried some experiments.
1) took jenkins slave vls-windows-2 machine offline, (the machine where assertion triggered in previous build)

2) ran workers avmshell_d_64.exe on previous failed test:  ecma3/String/e15_5_4_4_3.abc, 1000 times in a row
no assertions

3) ran 1000 times in a row, on 6 different cygwin windows at the same time
no assertions

4) ran runtests.py (3000 tests)
no assertions

5) ran runtests.py (3000 tests) on 6 windows concurrently
no assertions

Not sure how the jenkins environment is different.  If you have some ideas for adding more assertions let me know.

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch B v1: back propogate assertion to isolate GIGO effect

You can see the logic behind this addition by walking backwards through the following bits of code:

     double GCPolicyManager::A() {
        return P * R / W();
     }

       double aval = A();
        if(aval > INT_MAX)
            aval = INT_MAX;
        remainingMinorAllocationBudget = int32_t(aval);
        minorAllocationBudget = remainingMinorAllocationBudget;
        GCAssert(minorAllocationBudget > 0);
 
Namely, a positive minor allocation budget = int32_t(aval) 
requires A() > 1.0
requires W() <= P*R

Dan or Brent: Can you apply this patch in a testing context where you have seen the problem arise and report back the effect?

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #11)
> Created attachment 599618 [details] [diff] [review]
> patch B v1: back propogate assertion to isolate GIGO effect

FYI, "GIGO effect" == "Garbage-In Garbage-Out effect"; just trying to root out what input is causing problems, that's all.

Comment 13

[Brent Baker]

Quick run using tamarin-redux 7212:98ddf7c038a8 plus this patch produced the following assertion, multiple times on windows VLS machines:

Assertion failed:
"((W() <= P*R))" ("c:/jenkins/workspace/tamarin-redux-ompile/484059c0/MMgc/GCPolicyManager.cpp":290)

Comment 14

[Brent Baker]

<bump>... Any updates? We are trying to get the build system moved over to Jenkins but all of the windows machines are VLS images and routinely run into this error causes noise in the build system.

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

At this point, we certainly cannot keep the assert while also allowing the load factor to go arbitrarily close to 1.  Lars's note from comment 6 was assuming the load would be at least 1.125, but this simply is not true in our test infrastructure.

So I'll just put in the obvious code to provide a floor of 1 for 'aval', the same way we provide a ceiling of INT_MAX for it.

Comment 16

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A: put floor on A value

Comment 17

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 618829 [details] [diff] [review]
patch A: put floor on A value

(Ruchi: if you won't get to it tonight, please let me know; I'll redirect the review to Brent Baker in that case.)

Comment 18

[Ruchi Lohani]

Comment on attachment 618829 [details] [diff] [review]
patch A: put floor on A value

Review of attachment 618829 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like everywhere else minorAllocationBudget has been floored to 1 except this case. 
Given Lars' explanation, this looks good to me.

Comment 19

[Tamarin Bot]

changeset: 7376:395c3f08a9fb
user:      Felix Klock II <[e-mail]>
summary:   Bug 693807: put floor on A value (r=rulohani).

http://hg.mozilla.org/tamarin-redux/rev/395c3f08a9fb