477576 - Excessive memory peak with the Dromaeo string test

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090208 Shiretoko/3.1b3pre ID:20090208033138
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090208 Minefield/3.2a1pre

OmegaX over at the mozillazine forums reported this.

1. Open the Windows Task Manager, View > Select Columns and select the Peak Memory Usage column
2. New profile, start firefox
3. Do the Dromaeo string test http://dromaeo.com/tests/dromaeo-object-string.html

The test finishes fine, but the Peak Memory Usage for the firefox.exe process shows over 1 GB of RAM is allocated. This does not happen for Opera 9.62 nor IE7.

The memory looks like it is released properly, but taking up so much memory seems odd to me. Is this expected behaviour? (The excessive memory usage is present even if javascript.options.jit.content is set to false too)

Comment 1

[Brendan Eich [:brendan]]

When did this regress, can someone bisect and zero in on the cset or tinderbox window? Thanks,

/be

Comment 2

[Steve England [:stevee]]

Peak memory stays around 80MB:
- Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008063003 Minefield/3.1a1pre

Peak memory hits >1GB:
- Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008070103 Minefield/3.1a1pre

moz-central, Changes pushed after 2008-06-30 02:00:00, before 2008-07-01 04:00:00
- http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-06-30+02&enddate=2008-07-01+04

Comment 3

[Daniel Holbert [:dholbert]]

FWIW, my changeset from that range ( 354aed1b6240 ) only touches nsPageFrame.cpp, so unless there's some print-previewing/printing going on, I don't think it's possible that it could be responsible for this regression.

Comment 4

[(not currently active) Ted Mielczarek]

stevee: can you visit about:buildconfig in each of those builds and get exact changesets for them?

Comment 5

[Steve England [:stevee]]

Unfortunately these builds predate the changeset info that is now present in about:buildconfig

Comment 6

[(not currently active) Ted Mielczarek]

Ah, bummer, didn't look closely enough at the Build IDs. :-/ You can double check application.ini, I know that landed before the about:buildconfig support.

Comment 7

[Steve England [:stevee]]

Nothing in either application.ini unfortunately.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

My changeset in that range was fixing a comment in a unit test. Not it!

Comment 9

[Benjamin Smedberg]

It appears that this test is JS-only, and doesn't really touch the DOM, right?

* http://hg.mozilla.org/mozilla-central/rev/265eb7adf867 is analysis-only
* http://hg.mozilla.org/mozilla-central/rev/2a894be5a1b6 did change a few autostrings to non-autostrings, but not anywhere this test would touch
* http://hg.mozilla.org/mozilla-central/rev/3590fdfcfe26 is annotation-only
* http://hg.mozilla.org/mozilla-central/rev/6d88fe34dc34 is annotation-only

My bets are on http://hg.mozilla.org/mozilla-central/rev/6d70e01afad2, bug 440558 which is GC-related, pushed by mrbkap.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

I decided to finally learn how to use hg bisect:

And the verdict is ...

Changeset 91aa2d65f92d did it in SpiderMonkey with a faster
js_PutCallObject :-)

Comment 11

[Brendan Eich [:brendan]]

Steven, that must be

http://hg.mozilla.org/mozilla-central/rev/91aa2d65f92d

Igor, can you take a look? Thanks,

/be

Comment 12

[Robert Sayre]

Igor, any clues here?

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> Igor, any clues here?

No: I do not the excessive memory usage on Linux neither ib the browser nor js shell. I think I will have a working WIndows setup later this week and check this again from Windows.

Comment 14

[David Mandelin [:dmandelin]]

Want me to take a peek at this? I have MacOS and Windows.

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> Want me to take a peek at this? I have MacOS and Windows.

That would be nice indeed.

Comment 16

[David Mandelin [:dmandelin]]

Attachment: Reduced test case

Comment 17

[David Mandelin [:dmandelin]]

Attachment: Patch

Much thanks to waldo and mrbkap for helping me with this.

I still don't *completely* understand the problem, but the basic idea is that in the old (non-memory-spiking) version, js_SetNative set gcPoke, so we GC'd on max-malloc and kept memory usage down. In the new version, this doesn't happen, so we never GC inside the tight loop (in the reduced test case). That in turn happens because the property slot is -1 in the new version, and some actual value in the old version. Apparently the old version assigned slots for properties of call objects, and the new version doesn't (which is presumably a good thing). mrbkap told me that given the way this is all supposed to work, CallPropertyOp is responsible for setting gcPoke, so that's what the patch does.

Comment 18

[Igor Bukanov]

David Mandelin wrote in comment #17:

> the property slot is -1 in the new version, and some actual
> value in the old version. Apparently the old version assigned slots for
> properties of call objects, and the new version doesn't (which is presumably a
> good thing).

Before the bug 411575 for a call object of a function frame that is still executing the assigned property slots duplicated the values stored in the frame itself. The duplicated values were not used until the frame finished the execution and js_PutCallObject was called.

It was rather inefficient as it meant that js_PutCallObject, for all the arguments and variables that were not yet exposed as properties of the call object, had to add the corresponding property to store the argument or variable value in the property slot. The bug 411575 addressed the inefficiency via storing the arguments and variables in a separated array to avoid populating the call object with properties on function's return.

This change made the property slot completely unnecessary and in the bug 411575 I removed them marking all argument and variables properties as shared. But as you correctly pointed out, in the new code I forgot about GC_POKE that was called for property slots to signal that a new garbage could be created leading to this regression.

Comment 19

[David Mandelin [:dmandelin]]

(In reply to comment #18)

Thanks for the the background. Pushed to TM as 185869c23f93.

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/185869c23f93

Comment 21

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/444589caf117