Closed Bug 389318 Opened 17 years ago Closed 17 years ago

ActionMonkey: Convert JSRuntime into a MMgc::GCRoot

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #273484 - Flags: review?(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.
Attachment #273484 - Flags: review?(jorendorff) → review+
(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
Attached patch v1.1Splinter Review
(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.
Attachment #273484 - Attachment is obsolete: true
Pushed to hg/actionmonkey
remote: added 1 changesets with 2 changes to 2 files
changeset 3731:  	94b3c27b4efe
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
changeset 3733:  	f2043f7a7344
Attached patch Fix BUILD_OPT mac build breakage (obsolete) — Splinter Review
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
Attachment #274054 - Flags: review?(jorendorff)
remote: added 1 changesets with 1 changes to 1 files
changeset 110:  	a3c17f1d57c8
Attachment #274054 - Attachment is obsolete: true
Attachment #274054 - Flags: review?(jorendorff)
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
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.