Closed Bug 857706 Opened 7 years ago Closed 7 years ago

Always allocate the self-hosting global object in the tenured generation

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch v0 (obsolete) — Splinter Review
Breaking this out from the main GGC patch, as requested.
Attachment #732958 - Flags: review?(wmccloskey)
I wanted you to split this out because I don't understand it. First, I don't think it does what's described here. The object being allocated at the line you changed is not the global. I think it's some sort of self-hosting thing, but I'm not sure.

Second, does this patch actually have anything to do with singleton types, or are you just using this enum to mean "don't put it in the nursery"? If so, can we change the name of that enum? Maybe to TenuredObject or something? Do any of the existing uses have anything to do with singleton types?
(In reply to Bill McCloskey (:billm) from comment #1)
> I wanted you to split this out because I don't understand it. First, I don't
> think it does what's described here. The object being allocated at the line
> you changed is not the global. I think it's some sort of self-hosting thing,
> but I'm not sure.

Correct: it's an object all globals have that self-hosted functions and objects get cloned into upon first usage. Allocating it in the tenured generation certainly makes sense, just as it does for all the other objects created during global initialization.

The actual global's allocation happens in GlobalObject::create, fwiw.

> 
> Second, does this patch actually have anything to do with singleton types,
> or are you just using this enum to mean "don't put it in the nursery"? If
> so, can we change the name of that enum? Maybe to TenuredObject or
> something? Do any of the existing uses have anything to do with singleton
> types?

+1
Comment on attachment 732958 [details] [diff] [review]
v0

(In reply to Till Schneidereit [:till] from comment #2)
> (In reply to Bill McCloskey (:billm) from comment #1)
> > I wanted you to split this out because I don't understand it. First, I don't
> > think it does what's described here. The object being allocated at the line
> > you changed is not the global. I think it's some sort of self-hosting thing,
> > but I'm not sure.
> 
> Correct: it's an object all globals have that self-hosted functions and
> objects get cloned into upon first usage. Allocating it in the tenured
> generation certainly makes sense, just as it does for all the other objects
> created during global initialization.
> 
> The actual global's allocation happens in GlobalObject::create, fwiw.

Thanks, Till!

> > 
> > Second, does this patch actually have anything to do with singleton types,
> > or are you just using this enum to mean "don't put it in the nursery"? If
> > so, can we change the name of that enum? Maybe to TenuredObject or
> > something? Do any of the existing uses have anything to do with singleton
> > types?
> 
> +1

MaybeSingleton is for the cases in the frontend where we allocate an object at parse time before we know whether we will need to make it a singleton later during analysis. As you surmise, I've been using it to also mean StartTenured because it has the exact semantics we want. Adding a second, more sensible, name for these uses has been on my todo list for awhile now: I suppose now is as good a time as any.
Attachment #732958 - Flags: review?(wmccloskey)
Summary: Always allocate the global object in the tenured generation → Always allocate the self-hosting global object in the tenured generation
Attached patch v1Splinter Review
This adds the NewObjectKind TenuredObject and uses it where we were mis-using MaybeSingletonObject. The two existing mis-uses were for template objects in ion: they are fixed inline.

I also manually verified the status of the global object: it got set to tenured allocation when we fixed up all the singleton allocation paths.
Assignee: general → terrence
Attachment #732958 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #733065 - Flags: review?(wmccloskey)
Attachment #733065 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/ad371c559524
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.