Last Comment Bug 673551 - GC: Add environment variable for mark stack size
: GC: Add environment variable for mark stack size
Status: RESOLVED FIXED
[mentor=billm][lang=c++]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Bill McCloskey (:billm)
:
Mentors:
Depends on: 661690
Blocks: 685054 707313 939475
  Show dependency treegraph
 
Reported: 2011-07-22 14:26 PDT by Bill McCloskey (:billm)
Modified: 2013-11-17 00:55 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
jit_test.py support for '|jit-test| markstack=...' magic comment (2.70 KB, patch)
2011-12-02 14:18 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
Allow JS_MARK_STACK_SIZE env var to overide OBJECT_MARK_STACK_SIZE (2.69 KB, patch)
2011-12-12 08:18 PST, Jonathan "Duke" Leto
no flags Details | Diff | Review
add mark stack limit (based off larch) (9.65 KB, patch)
2012-02-13 17:52 PST, Bill McCloskey (:billm)
igor: review+
Details | Diff | Review

Description Bill McCloskey (:billm) 2011-07-22 14:26:12 PDT
+++ This bug was initially created as a clone of Bug #661690 +++

Igor has suggested that we should have some sort of option for controlling the GC mark stack sizes. This way we could artificially trigger delayed marking. See bug 661690 for more information.
Comment 1 Wes Kocher (:KWierso) 2011-07-22 14:33:12 PDT
From bug 661690, for whoever works on this (I did the patch in 661690, but I'm going to be out of town for a while):

The mark stack sizes are set with constants like OBJECT_MARK_STACK_SIZE in js/src/jsgc.h. These would need to become options that could be set from the command line, and also maybe from about:config. The command-line parsing for the JavaScript shell is done in js/src/shell/js.cpp.


In addition, it would be nice to have these parameters to control the mark stack size:
  javascript.options.gc.mark_stack_size.objects
  javascript.options.gc.mark_stack_size.ropes
  javascript.options.gc.mark_stack_size.xml
  javascript.options.gc.mark_stack_size.large_objects
These would all be integers. They would be used in place of constants, like 
OBJECT_MARK_STACK_SIZE, that are defined in jsgc.h.
Comment 2 Jonathan "Duke" Leto 2011-10-11 12:36:28 PDT
To clarify: Does this bug want options that can be controlled via about:config, command-line, or both?
Comment 3 Bill McCloskey (:billm) 2011-10-11 13:51:27 PDT
Now that I think about this, an environment variable might be best. That would work for both the shell and the browser, and we wouldn't need to change our test harnesses at all.
Comment 4 Bill McCloskey (:billm) 2011-10-11 22:39:42 PDT
Duke, I had a few more ideas.

First, it probably makes sense to allow the mark stack size to be specified like this:
  JS_MARK_STACK_SIZE=<obj-size>[,<large-obj-size>[,<rope-size>[,<xml-size>]]]
The brackets mean something is optional. The object stack and large object stack are the most important, so those should go first.

Also, it's important not to allocate any memory during a collection. (Since the whole point of GC is to reclaim memory, it's often invoked in low-memory situations, so allocations are likely to fail.) To get started, you might want to use the current stack sizes as maximums. That way you don't have to change how memory is allocated. If you have more time, you could allocate the memory dynamically when the runtime is created or something.
Comment 5 Jason Orendorff [:jorendorff] 2011-12-02 14:15:31 PST
This is a great idea!

In fact it is more than a great idea; it's necessary. When we add code that is not tested--hard to test, even--we are regressing test coverage. In the long run, I think that's a very serious regression. We should not tolerate something like that for long.

Contrary to comment 3, I think we should have jit_test.py harness support for it. It's trivial, and that way we can write regression tests for specific bugs (like bug 707313), and run *only* those tests under a smaller GC stack, not all tests.
Comment 6 Jason Orendorff [:jorendorff] 2011-12-02 14:18:10 PST
Created attachment 578724 [details] [diff] [review]
jit_test.py support for '|jit-test| markstack=...' magic comment

(Not taking the bug. This is just a patch for jit_test.py. It's only fair to put up the code after asserting in comment 5 that this is “trivial”. :-)
Comment 7 Jonathan "Duke" Leto 2011-12-02 19:07:24 PST
 Thanks for the patch to jit_test.py ! Hacking on this now after a major tuit shortage.
Comment 8 Jason Orendorff [:jorendorff] 2011-12-09 12:06:47 PST
Ping. Is anything going on here? Should I take?
Comment 9 Jonathan "Duke" Leto 2011-12-09 12:11:10 PST
I am working on a patch. If I don't have a patch attached to this bug by Monday morning, feel free to steal from me :)
Comment 10 Jonathan "Duke" Leto 2011-12-12 08:18:13 PST
Created attachment 580916 [details] [diff] [review]
Allow JS_MARK_STACK_SIZE env var to overide OBJECT_MARK_STACK_SIZE
Comment 11 Jonathan "Duke" Leto 2011-12-12 08:32:22 PST
My attached patch incorporates the changes to jit_test.py, because the above patch did not apply cleanly for me, so I manually applied it.

Please let me know if I am on the right track, or totally off my rocker.

It causes one jit_test to fail, but that is because the test only looks at OBJECT_MARK_STACK_SIZE and not the JS_MARK_STACK_SIZE env var.
Comment 12 Jonathan "Duke" Leto 2011-12-12 08:33:19 PST
I am using a fork of double/mozilla-central on Github to hack on this, you can view a branch diff here: https://github.com/leto/mozilla-central/compare/master...bug673551
Comment 13 Jonathan "Duke" Leto 2011-12-12 09:48:38 PST
These are the failures I am seeing.

FAILURES:
    -m /home/leto/git/mozilla-central/js/src/jit-test/tests/basic/bug656261.js
    -m -n /home/leto/git/mozilla-central/js/src/jit-test/tests/basic/bug656261.js

It seems that we need a testing function that returns the value of the JS_MARK_STACK_SIZE env var so we can either skip or modify the above test to work correctly. Does that sound about right?
Comment 14 Bill McCloskey (:billm) 2011-12-27 18:55:51 PST
Hi Duke,

Sorry for the delay. I realize I didn't explain well enough what's needed here. The mark stack is defined in js/src/jsgc.h (the MarkStack class). This class has a field limit, which is what really defines how big the mark stack can get. The MarkStack object is a field of GCMarker, and we create the GCMarker as a local variable in the MarkAndSweep function of jsgc.cpp.

So to fix this bug, the mark stack environment variable needs to be passed into the GCMarker constructor, and then into the MarkStack constructor, where it would be used to set the limit field.

Your patch adjusts the constant in the shell, which is also needed. However, you should be able to call atoi directly on the return value of getenv. Copying it to the mark_stack_size variable is not needed. Also, since mark_stack_size is a string literal, it's allocated in a write-protected part of memory. So when you copy into it, the program will crash. I suspect that's why you're getting test failures--those happen to be the tests that use InternalConst.

Please let me know if this is too much for you. I'm sorry I didn't do a better job of explaining the scope of the project up front.
Comment 15 Jonathan "Duke" Leto 2011-12-28 08:39:29 PST
Howdy Bill,

Your more detailed explanation makes sense, but I still have a few questions.

Also I have gotten rid of the mark_stack_size string literal and now call atoi directly on getenv.

I am looking inside the MarkAndSweep function in jsgc.cpp and on line 2698 :

https://github.com/leto/mozilla-central/blob/master/js/src/jsgc.cpp#L2698

If I understand correctly, I will need to modify that line to be something like:

GCMarker gcmarker(cx, stack_size);

and then do something similar in the MarkStack constructor, so that I can use stack_size to calculate the limit variable in the constructor here:

https://github.com/leto/mozilla-central/blob/bug673551/js/src/jsgc.h#L1617

Does that sound about right?
Comment 16 Bill McCloskey (:billm) 2011-12-28 13:32:40 PST
That sounds exactly right.
Comment 17 Jason Orendorff [:jorendorff] 2012-01-27 00:46:41 PST
Duke, do you have time to finish this bug today? If not, please assign it to me or billm.
Comment 18 Jonathan "Duke" Leto 2012-01-30 15:12:24 PST
Sorry, all I have is this branch which compiles but is not fully functional:

https://github.com/leto/mozilla-central/compare/master...bug673551

You can grab it as a patch like this:

https://github.com/leto/mozilla-central/compare/master...bug673551.patch

Sorry, I won't have any more time to iterate on this patch in the near future. The task turned out to be bigger than it seemed at first :)
Comment 19 Jason Orendorff [:jorendorff] 2012-02-01 07:09:18 PST
Bill, Andrew, can either of you take this?
Comment 20 Bill McCloskey (:billm) 2012-02-01 10:12:13 PST
I'll try to get this done soon.
Comment 21 Jason Orendorff [:jorendorff] 2012-02-13 08:05:33 PST
OMG, this bug called me last night at 2AM, crying uncontrollably.  I got to hear about the mixed messages, the broken promises, the twisting in the wind, complicated and insane theories about how it was to blame for your feelings. Two hours of this. Oh and in the background it had "Don't You Forget About Me" by Simple Minds playing on repeat. It was horrible. 

Tomorrow is Valentine's Day. If you need me I will be over at this bug's house holding a box of tissue and suffering. Unless you want to patch things up.
Comment 22 Bill McCloskey (:billm) 2012-02-13 17:52:20 PST
Created attachment 596877 [details] [diff] [review]
add mark stack limit (based off larch)

Sorry, baby, but sometimes a man's gotta have some space. I was out ridin' my motorcycle like always, when that buzzkill sheriff flashed his lights at me. I flipped him the bird, and he chased me all the way into Buzzard County. I had to lay low with Uncle Merle until the heat died down.

But don't worry, baby, I'll make it up to you.
Comment 23 Igor Bukanov 2012-02-14 00:39:34 PST
Comment on attachment 596877 [details] [diff] [review]
add mark stack limit (based off larch)

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

BTW, does everything still work if you set the size limit to 0?

::: js/src/jsgc.h
@@ +1668,5 @@
>          size_t newcap = cap * 2;
>          if (newcap == 0)
>              newcap = 32;
> +        if (cap == sizeLimit)
> +            return false;

Move the last 2 lines right after cap is initialized before size_t newcap.
Comment 25 Marco Bonardo [::mak] 2012-02-25 02:19:29 PST
https://hg.mozilla.org/mozilla-central/rev/fad8d2718cd0
Comment 26 Jesse Ruderman 2013-11-17 00:55:22 PST
Bisection results may be confusing. See bug 889559 comment 4.

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