650367 - Need selftest for WeakValueHashtable

Status: VERIFIED FIXED

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

Description

[Brent Baker]

Currently there is no usage in the shell for WeakValueHashtable so we will need to create a selftest that uses this object. Create selftest so that we can get code coverage of this via the shell.

Comment 1

[Lars T Hansen]

Attachment: Patch

Comment 2

[Lars T Hansen]

Sorry, not a patch - just a file.

Comment 3

[Tamarin Bot]

changeset: 6188:3070ff6dee1f
user:      Brent Baker <[e-mail]>
summary:   Bug 650367: add selftest for WeakValueHashtable (p=lhansen,r=rulohani,brbaker)

http://hg.mozilla.org/tamarin-redux/rev/3070ff6dee1f

Comment 4

[Chris Peyer]

The selftest is failing on linux-arm:

['start', 'avmplus', 'builtins']
['test', 'avmplus', 'builtins', 'WeakValueHashTable']
['failure', 'avmplus', 'builtins', 'WeakValueHashTable', 'sum >= 250 && sum <= 300', 'ST_avmplus_builtins.st', 60]
['end', 'avmplus', 'builtins']

full log: http://tamarin-builds.mozilla.org/tamarin-redux/builders/linux-arm-test/builds/396/steps/Testsuite_Selftest_Release/logs/stdio

Comment 5

[Lars T Hansen]

Yes, there is a weakness in the test: an aggressive compiler can clear out the stack variable that holds onto the string that should not be collected.  It needs to be anchored more carefully.

Comment 6

[Lars T Hansen]

Attachment: Lock the "permanent" object

Comment 7

[Brent Baker]

Comment on attachment 528016 [details] [diff] [review]
Lock the "permanent" object

I am running this patch through the sandbox now.

Comment 8

[Brent Baker]

Comment on attachment 528016 [details] [diff] [review]
Lock the "permanent" object

Review of attachment 528016 [details] [diff] [review]:

(In reply to comment #7)
> Comment on attachment 528016 [details] [diff] [review]
> Lock the "permanent" object
> 
> I am running this patch through the sandbox now.

Selftest is still failing on linux-arm

Comment 9

[Lars T Hansen]

In that case you need to modify the test to print out the actual value of "sum", since I don't understand what's going on.

Comment 10

[Brent Baker]

(In reply to comment #9)
> In that case you need to modify the test to print out the actual value of
> "sum", since I don't understand what's going on.

On linux arm, with the patch to lock the object (attachment #528016 [details] [diff] [review]), the sum is 0 at the end of the loop

ST_avmplus_builtins.st
AvmLog("sum=%d\n",sum);
// Retain at least 250, but it would be unreasonable to retain more than 300
%%verify sum >= 250 && sum <= 300

selftest output
['test', 'avmplus', 'builtins', 'WeakValueHashTable']
sum=0
['failure', 'avmplus', 'builtins', 'WeakValueHashTable', 'sum >= 250 && sum <= 300', 'ST_avmplus_builtins.st', 63]

Comment 11

[Brent Baker]

Release build has a sum=0
Debug build has a sum=251

Comment 12

[Tamarin Bot]

changeset: 6214:eeb68bddb214
user:      Brent Baker <[e-mail]>
summary:   Bug 616125: add testing around XML.setNotification() (r=treilly)

http://hg.mozilla.org/tamarin-redux/rev/eeb68bddb214

Comment 13

[Brent Baker]

(In reply to comment #12)
> changeset: 6214:eeb68bddb214
> user:      Brent Baker <[e-mail]>
> summary:   Bug 616125: add testing around XML.setNotification() (r=treilly)
> 
> http://hg.mozilla.org/tamarin-redux/rev/eeb68bddb214

This is the second line of the commit message, which makes much more sense on the context of this bug:

"Bug 650367: disable WeakValueHashtable() selftest on ARM until issue is resolved"

Comment 14

[Lars T Hansen]

(In reply to comment #11)
> Release build has a sum=0
> Debug build has a sum=251

That is incomprehensible to me...

Comment 15

[Lars T Hansen]

(In reply to comment #14)
> (In reply to comment #11)
> > Release build has a sum=0
> > Debug build has a sum=251
> 
> That is incomprehensible to me...

No longer.  Bug 654052 logged to deal with a bug in the lock API.

I managed to repro the problem on Mac Desktop release builds, I'll post a new patch here.

Comment 16

[Lars T Hansen]

Attachment: Lock the "permanent" object, v2

With this update, we expose on Mac release builds the ARM problem, which is fixed in bug #654052, and that patch is required to test the present patch.

Comment 17

[Brent Baker]

Comment on attachment 529424 [details] [diff] [review]
Lock the "permanent" object, v2

Looks good, but please regen the testcase with the "%%ifndef    VMCFG_ARM" removed from the *.st file so that the testcase is run on ARM.

I ran this through the sandbox along with the patch from 654052 and everything ran cleanly.

Comment 18

[Tamarin Bot]

changeset: 6249:0d858bab2de1
user:      Lars T Hansen <[e-mail]>
summary:   Fix 650367 - Need selftest for WeakValueHashtable (r=brbaker)

http://hg.mozilla.org/tamarin-redux/rev/0d858bab2de1

Comment 19

[Brent Baker]

tamarin-redux-serrano brbaker$ hg log -r 380d7261b801
changeset:   6285:380d7261b801
user:        Lars T Hansen <[e-mail]>
date:        Mon May 02 17:09:58 2011 +0200
summary:     Fix 650367 - Need selftest for WeakValueHashtable (r=brbaker)