memory leak when a property is deleted

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: Franck, Assigned: Igor Bukanov)

Tracking

(Depends on: 1 bug, {memory-leak, regression})

Trunk
memory-leak, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9
Build Identifier: Last CVS trunk (2007-11-20)

It seems that adding then deleting a same property on an object make a memory leak. ( approximately 10 Bytes )

Reproducible: Always

Steps to Reproduce:
var obj = {};
for (;;) {

	for ( i = 0; i < 10000; i++ ) {

		obj[123] = 456;
		delete obj[123];
	}
	gc();
}




If the reference of obj is lost (by creating the obj inside the first loop), the memory leak disappear.

Updated

11 years ago
Keywords: mlk
(Reporter)

Comment 1

11 years ago
Note that this memory leak appears also when you use Arrays:
  ...
  arr.push(456);
  arr.pop();
  ...

Comment 2

11 years ago
I can't reproduce this with a trunk JS shell, is this browser-only?
(Reporter)

Comment 3

11 years ago
It's SpiderMonkey-only. 
I just updated my cvs copy with the latest sources, and the leak is still there.
I use the 'top' command on linux and the "Task Manager" under window to see the process memory growing.

Comment 4

11 years ago
Created attachment 290512 [details]
brief run of testcase using massif to display heap usage
Assignee: general → crowder
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: blocking1.9?

Comment 5

11 years ago
Not really a -leak- in the strictest sense; the attached massif graph shows the heap utilization.  Valgrind's memcheck tool describes the full heap being eventually released.  More likely what's happening here is that we are gradually consuming more and more heap which we hang on to.  Having not debugged this at all yet, it's possible that this is by-design at that the growth would eventually plateau as arena space got reused.  That said, I'm not sure.  I will take a closer look in the morning.
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk

Comment 6

11 years ago
Created attachment 290513 [details]
longer run

This longer run shows no plateau.  :(
(Reporter)

Comment 7

11 years ago
Btw, I cannot reproduce this "kind of leak" with SpiderMonkey 1.7 ( ftp://ftp.mozilla.org/pub/mozilla.org/js/js-1.7.0.tar.gz )
OS: All → Windows XP
Hardware: All → PC
Version: Trunk → unspecified

Updated

11 years ago
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk
(Assignee)

Comment 9

11 years ago
To Brian:

My bet is that it is caused by bug 363603. Fill free to reassign this bug to me.

Updated

11 years ago
Assignee: crowder → igor
Status: ASSIGNED → NEW
(Assignee)

Comment 10

11 years ago
Created attachment 290587 [details] [diff] [review]
fix v1

My bad: in the optimization patch for bug 363557 I went too far with code simplification and removed the update of map->freeslot in js_FreeSlot.

Sorry for not taking this bug immediately.
Attachment #290587 - Flags: review?(brendan)

Comment 11

11 years ago
Patch fixes the bug for me, if brendan doesn't have time to r+ you can have mine, Igor (though I don't think I can help with a+).  This patch should land as soon as possible, though.
Comment on attachment 290587 [details] [diff] [review]
fix v1

Sorry I missed this when reviewing the patch for bug 363603.

/be
Attachment #290587 - Flags: review?(brendan)
Attachment #290587 - Flags: review+
Attachment #290587 - Flags: approval1.9+
(Assignee)

Updated

11 years ago
Blocks: 363603
(Assignee)

Comment 13

11 years ago
I checked in the pathc from comment 10 to the CVS trunk:

Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.398; previous revision: 3.397
done

To Franck:

Thanks for finding this regression!
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Can we get a unit test for this somehow?  The prospect of these sorts of bugs slipping out into releases is what keeps me sniffing glue.
Flags: in-testsuite?

Comment 15

11 years ago
I was trying to work out some way for the program to measure itself in a test for this using mallinfo, but didn't have great luck (and that test wouldn't be portable), because mallinfo's reporting doesn't seem very predictable.

Updated

11 years ago
Keywords: regression
Removing the nom here as the patch already has approval.
Flags: blocking1.9?
(Assignee)

Comment 17

11 years ago
(In reply to comment #14)
> Can we get a unit test for this somehow? 

Here is a performance-based test that tries to observe the effects of the bug on GC timing. Since the the bug causes the growth of the slot array, it would take much longer to scan the array for GC after the loop then for an object that was not subject of repeated add/delete. 

function test()
{
    var n = 1 << 22;
    var o = {};
    do {
        o[0] = 0;
        delete o[0]; 
    } while (--n != 0);

    gc();
    var time = Date.now();
    gc();
    time = Date.now() - time;

    o = {};
    o[0] = 0;
    delete o[0]; 
    gc();
    var time2 = Date.now();
    gc();
    time2 = Date.now() - time2;
    print(time+" "+time2);
    if (time > 2 && time > time2 * 5)
        throw "A possible leak is observed";
}

test();
Backed out to see if this fixes the oranges.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded. Looks like it was mw22's test in bug 396024 that caused it.

Checking in js/src/jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.400; previous revision: 3.399
done
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 20

11 years ago
Checking in regress-404755.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-404755.js,v  <--  regress-404755.js
initial revision: 1.1
Flags: in-testsuite? → in-testsuite+

Comment 21

11 years ago
verified fixed 2007-12-09 1.9.0 linux|mac|win
Status: RESOLVED → VERIFIED

Updated

8 years ago
Depends on: 544603
You need to log in before you can comment on or make changes to this bug.