Closed Bug 673551 Opened 13 years ago Closed 12 years ago

GC: Add environment variable for mark stack size

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: billm, Assigned: billm)

References

Details

(Whiteboard: [mentor=billm][lang=c++])

Attachments

(1 file, 2 obsolete files)

add mark stack limit (based off larch)
9.65 KB, patch
igor
: review+
Details | Diff | Splinter Review 
+++ 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.
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.
Whiteboard: mentor=billm
To clarify: Does this bug want options that can be controlled via about:config, command-line, or both?
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.
Summary: GC: Add about:config options for mark stack size → GC: Add environment variable for mark stack size
Assignee: general → jonathan
Status: NEW → ASSIGNED
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.
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.
Blocks: 707313
(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”. :-)
 Thanks for the patch to jit_test.py ! Hacking on this now after a major tuit shortage.
Ping. Is anything going on here? Should I take?
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 :)
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.
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
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?
Whiteboard: mentor=billm → [mentor=billm][lang=c++]
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.
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?
That sounds exactly right.
Duke, do you have time to finish this bug today? If not, please assign it to me or billm.
Blocks: 685054
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 :)
Bill, Andrew, can either of you take this?
Assignee: jonathan → general
I'll try to get this done soon.
Assignee: general → wmccloskey
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.
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.
Attachment #580916 - Attachment is obsolete: true
Attachment #596877 - Flags: review?(igor)
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.
Attachment #596877 - Flags: review?(igor) → review+
https://hg.mozilla.org/mozilla-central/rev/fad8d2718cd0
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 939475
Bisection results may be confusing. See bug 889559 comment 4.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: