389318 - ActionMonkey: Convert JSRuntime into a MMgc::GCRoot

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ed Lee :Mardak]

Attachment: v1

JSRuntime will be the main MMgc::GCRoot with a MMgc::GC per runtime (there's only one JSRuntime for Firefox).

One benefit of making JSRuntime a GCRoot is that constants js(NaN|(Negative|Positive)Infinity)|emptyString and other GCF_LOCKable (hash) items are pointed from JSRuntime, so they won't get GCed.

JSRuntime is being made into a class that inherits from GCRoot instead of a plain struct, so we overload the new operator to keep the existing C initialization code working.

Comment 1

[Jason Orendorff [:jorendorff]]

Comment on attachment 273484 [details] [diff] [review]
v1

Leave the memset() in JS_NewRuntime(), with the comment.  |operator new| isn't logically the right place for initializing data members.

Also, style nit-- "if (!gc) return NULL;" should be split into two lines.  (In the existing codebase, the score is 2523 to 11.)

r+ with those changes.

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #1)
> Also, style nit-- "if (!gc) return NULL;" should be split into two lines.  (In
> the existing codebase, the score is 2523 to 11.)

Rationale, probably recorded in README.html in js/src, is debugability: it's hard to breakpoint the then clause if the if-condition and then clause are all on one line.

/be

Comment 3

[Ed Lee :Mardak]

Attachment: v1.1

(In reply to comment #1)
> Leave the memset() in JS_NewRuntime(), with the comment.  |operator new| isn't
> logically the right place for initializing data members.
Done. Restored comment and noted in the constructor that things shouldn't be inited there.

> Also, style nit-- "if (!gc) return NULL;" should be split into two lines. 
Split.

Comment 4

[Ed Lee :Mardak]

Pushed to hg/actionmonkey
remote: added 1 changesets with 2 changes to 2 files
changeset 3731:  	94b3c27b4efe

Comment 5

[Ed Lee :Mardak]

Attachment: Keep JSRuntime as a struct

changeset 3733:  	f2043f7a7344

Comment 6

[Ed Lee :Mardak]

Attachment: Fix BUILD_OPT mac build breakage

MMgc.h includes some stuff that eventually includes /usr/include/AssertMacros.h which #define check(assertion) -- this conflicts with |check| in jsobj.cpp

jsobj.cpp:4231:63: error: macro "check" passed 5 arguments, but takes just 1

so undef it from GCSpinLockMac.h

Comment 7

[Ed Lee :Mardak]

Attachment: Fix BUILD_OPT mac build breakage - as checked in

remote: added 1 changesets with 1 changes to 1 files
changeset 110:  	a3c17f1d57c8

Comment 8

[Brendan Eich [:brendan]]

Sounds like a trail of name-collision tears. It would be good to avoid over-including mega-headers. Windows.h is a notorious one we've avoided in the past. Sounds like Mac has its own monster headers with invasive names. Suggest a patch to MMgc to avoid this?

/be