remove AutoObjectRooter, AutoIdRooter, AutoValueRooter

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
8 years ago
6 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Other Branch
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

107.08 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
No longer needed. Most calls to AutoArrayRooter aren't needed either (except one).

I get a couple failures. Probably a typo. Will look.

REGRESSIONS
    js1_5/GC/regress-390078.js
    js1_5/Regress/regress-203278-1.js
    js1_6/extensions/regress-475144.js
    js1_5/Array/regress-157652.js
    js1_5/Array/regress-330812.js
    js1_7/extensions/regress-455982-01.js
    js1_7/extensions/regress-455982-02.js
    js1_7/regress/regress-469239-01.js
    js1_8/extensions/regress-452476.js
    js1_8/regress/regress-466787.js
    js1_8/regress/regress-477581.js
    js1_8_5/regress/regress-555246-1.js
    js1_8_1/trace/trace-test.js
    js1_5/Regress/regress-422348.js
(Assignee)

Updated

8 years ago
Depends on: 516832
(Assignee)

Comment 1

8 years ago
Created attachment 451228 [details] [diff] [review]
patch
Assignee: general → gal
(Assignee)

Comment 2

8 years ago
Easy fix. Typo. I used *objp instead of obj in ObjectToIterator in jstracer.cpp.
(Assignee)

Comment 3

8 years ago
Passes all tests, crashes SS (during GC). This points to a lack of GC coverage in our test suite. We should probably will file a separate bug on that. I will look into the crash.
(Assignee)

Comment 4

8 years ago
(gdb) b js_GC 
Breakpoint 1 at 0x1000507c1: file ../jsgc.cpp, line 3542.
(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /Users/gal/workspace/tracemonkey-repository/js/src/Darwin_OPT.OBJ/js 
Reading symbols for shared libraries . done
js> var d = Date.now(); load("t/3d-cube.js"); load("t/3d-morph.js"); load("t/3d-raytrace.js"); load("t/access-binary-trees.js"); load("t/access-fannkuch.js"); load("t/access-nbody.js"); load("t/access-nsieve.js"); load("t/bitops-3bit-bits-in-byte.js"); load("t/bitops-bits-in-byte.js"); load("t/bitops-bitwise-and.js"); load("t/bitops-nsieve-bits.js"); load("t/controlflow-recursive.js"); load("t/crypto-aes.js"); load("t/crypto-md5.js"); load("t/crypto-sha1.js"); load("t/date-format-tofte.js"); load("t/date-format-xparb.js"); load("t/math-cordic.js"); load("t/math-partial-sums.js"); load("t/math-spectral-norm.js"); load("t/regexp-dna.js"); load("t/string-base64.js"); load("t/string-fasta.js"); load("t/string-tagcloud.js"); load("t/string-unpack-code.js"); load("t/string-validate-input.js"); print(Date.now() - d);

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: 13 at address: 0x0000000000000000
0x00000001000e1a09 in JSObject::setProperty () at /Users/gal/workspace/tracemonkey-repository/js/src/jsobj.h:1607
1607	    return !!arrayobj->setProperty(cx, INT_TO_JSID(count), &v);
(gdb) 
(gdb) bt
#0  0x00000001000e1a09 in JSObject::setProperty () at /Users/gal/workspace/tracemonkey-repository/js/src/jsobj.h:1607
#1  0x00000001000e1a09 in MatchCallback (cx=0x100311fd0, count=0, p=<value temporarily unavailable, due to optimizations>) at ../jsstr.cpp:1607
#2  0x00000001000dfa19 in DoMatch (cx=0x100311fd0, vp=0x1004003b8, str=0x10567b1c0, g=@0x7fff5fbfed80, callback=0x1000e1970 <MatchCallback(JSContext*, unsigned long, void*)>, data=0x7fff5fbfedd8, flags=<value temporarily unavailable, due to optimizations>) at ../jsstr.cpp:1607
#3  0x00000001000e2dd4 in str_match (cx=0x100311fd0, argc=1, vp=0x1004003b8) at ../jsstr.cpp:1607
#4  0x000000010005d89c in js_Interpret (cx=0x100311fd0) at jsops.cpp:1607
#5  0x0000000100064bbc in js_Execute (cx=0x100311fd0, chain=0x101202000, script=<value temporarily unavailable, due to optimizations>, down=<value temporarily unavailable, due to optimizations>, flags=2, result=0x0) at jsinterp.cpp:1607
#6  0x000000010000efb0 in JS_ExecuteScript (cx=0x100311fd0, obj=<value temporarily unavailable, due to optimizations>, script=<value temporarily unavailable, due to optimizations>, rval=<value temporarily unavailable, due to optimizations>) at ../jsapi.cpp:1607
#7  0x0000000100004c51 in Load (cx=0x100311fd0, obj=0x101202000, argc=1, argv=<value temporarily unavailable, due to optimizations>, rval=<value temporarily unavailable, due to optimizations>) at ../../shell/js.cpp:1607
#8  0x0000000100065430 in js_Invoke (cx=0x100311fd0, args=@0x7fff5fbff570, flags=2) at jsinterp.cpp:1607
#9  0x00000001000547d6 in js_Interpret (cx=0x100311fd0) at jsops.cpp:1607
#10 0x0000000100064bbc in js_Execute (cx=0x100311fd0, chain=0x101202000, script=<value temporarily unavailable, due to optimizations>, down=<value temporarily unavailable, due to optimizations>, flags=2, result=0x7fff5fbff7c8) at jsinterp.cpp:1607
#11 0x000000010000efb0 in JS_ExecuteScript (cx=0x100311fd0, obj=<value temporarily unavailable, due to optimizations>, script=<value temporarily unavailable, due to optimizations>, rval=<value temporarily unavailable, due to optimizations>) at ../jsapi.cpp:1607
#12 0x0000000100003e07 in Process (cx=0x100311fd0, obj=0x101202000, filename=<value temporarily unavailable, due to optimizations>, forceTTY=<value temporarily unavailable, due to optimizations>) at ../../shell/js.cpp:1607
#13 0x0000000100007139 in shell (cx=0x100311fd0, argc=0, argv=0x7fff5fbff958, envp=0x7fff5fbff960) at ../../shell/js.cpp:1607
#14 0x0000000100007516 in main (argc=1, argv=0x7fff5fbff950, envp=0x7fff5fbff960) at ../../shell/js.cpp:1607

We die with jit off in regexp code. Has to be another typo. No gc occurred yet.

Comment 5

8 years ago
As a first step we should keep the classes and their usage but replace the implementation with asserts that the things are marked.
(Assignee)

Comment 6

8 years ago
Created attachment 451236 [details] [diff] [review]
patch

Easy fix. Another typo.
Attachment #451228 - Attachment is obsolete: true
Does this mean we can nix the nsContentUtils auto rooters too?

Comment 8

8 years ago
(In reply to comment #7)
> Does this mean we can nix the nsContentUtils auto rooters too?

It looks the answer is yes. The only potential source of troubles can comefrom a pattern like

nsAutoGCRoot root();

{
   JSAutoRequest r1;
   store_val_in_root;
}
...
{
   JSAutoRequest r2;
   get_val_in_root;
}

With conservative GC the above pattern would be unsafe since the GC does not scan the stack if thread is not in a request. But there is no such usage AFAICS.
Still a valid non-TM bug?

Comment 10

6 years ago
(In reply to Ryan VanderMeulen from comment #9)
> Still a valid non-TM bug?

Yes.
Summary: TM: remove AutoObjectRooter, AutoIdRooter, AutoValueRooter → remove AutoObjectRooter, AutoIdRooter, AutoValueRooter
We're converging on a plan to remove the conservative scanner. Brian has a static analysis for finding rooting bugs. It will be a lot of work to do this, but I think it's worth it.

It's not entirely clear what sort of rooting API we'll use in the future, but I think it will be easier to transform the old API into the new one rather than removing the old one and then putting in a new one.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX

Comment 12

6 years ago
(In reply to Bill McCloskey (:billm) from comment #11)
> We're converging on a plan to remove the conservative scanner. Brian has a
> static analysis for finding rooting bugs. It will be a lot of work to do
> this, but I think it's worth it.

I second that. It became rather clear for me that the conservative GC put too much requirements on an allocator and prevents many optimizations.
(Assignee)

Comment 13

6 years ago
As long it's an analysis at every checkin static analysis is strictly superior to conservative scanning. We should use it to infer a minimum set of rooting operations though instead of using these ugly and verbose manual rooting constructs.
You need to log in before you can comment on or make changes to this bug.