Closed Bug 922283 Opened 11 years ago Closed 11 years ago

Remove the self hosted class mechanism

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
This looks like dead code, and it is inlined by IonBuilder in a way that is incompatible with off thread MIR construction.
Attachment #812221 - Flags: review?(shu)
Comment on attachment 812221 [details] [diff] [review]
patch

Review of attachment 812221 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but we should also remove the NewClassPrototype intrinsic and the related SelfHostedClass mechanism if we're removing NewObjectWithClassPrototype. The Unsafe{Get,Set}ReservedSlot and HaveSameClass intrinsics can probably stay since they are independently useful.

Let me also double check with nmatsakis and pnkfelix that they aren't using these intrinsics in any unlanded patches.
Attachment #812221 - Flags: review?(shu)
Niko and Felix, are you guys using the NewClassPrototype or NewObjectWithClassPrototype intrinsics in any typed object patches?
Flags: needinfo?(pnkfelix)
Flags: needinfo?(nmatsakis)
No. I do use `HasSameClass`, I think, but I don't need to be using it.
Flags: needinfo?(nmatsakis)
(In reply to Shu-yu Guo [:shu] from comment #2)
> Niko and Felix, are you guys using the NewClassPrototype or
> NewObjectWithClassPrototype intrinsics in any typed object patches?

Nope.
Flags: needinfo?(pnkfelix)
Remove the entire self hosted class mechanism.
Assignee: nobody → bhackett1024
Attachment #812221 - Attachment is obsolete: true
Attachment #812660 - Flags: review?(shu)
Summary: Remove NewObjectWithClassPrototype intrinsic → Remove the self hosted class mechanism
Attachment #812660 - Flags: review?(shu) → review+
https://hg.mozilla.org/mozilla-central/rev/8b0e4dee6b1a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(In reply to Shu-yu Guo [:shu] from comment #2)
> Niko and Felix, are you guys using the NewClassPrototype or
> NewObjectWithClassPrototype intrinsics in any typed object patches?

I was using NewObjectWithClassPrototype in bug 919948 :-((
(In reply to Andy Wingo [:wingo] from comment #8)
> (In reply to Shu-yu Guo [:shu] from comment #2)
> > Niko and Felix, are you guys using the NewClassPrototype or
> > NewObjectWithClassPrototype intrinsics in any typed object patches?
> 
> I was using NewObjectWithClassPrototype in bug 919948 :-((

Argh! Sorry, in what way were you using it? Perhaps we can back out Brian's removal and do something else to fix the OMTC issue.
In reply to Shu-yu Guo [:shu] from comment #9)

It's all right; I worked around it.  Sorry for the noise.

More details: Parts of iterators are becoming self-hosted, because the for-of protocol is specified to do a number of method calls on the iterable object, and it's best in the long term to keep the VM in JavaScript-land.  Iterators have private state.  I was declaring the class prototypes in C++, then creating them as needed from self-hosted code via NewObjectWithClassPrototype.

When I rebased that patch, it compiled fine (!), but gave an inscrutable error at runtime.  This is of course due to the fact that everything was fine, but the intrinsics I was using were gone.  It would be nice if we could figure out a way to make unbound variable references cause a compile-time error in self-hosted JS.

To fix the problem, I introduced new, specific NewArrayIterator / HasArrayIteratorClass intrinsics.  Once we figure out what the right pattern is there, we should probably switch those to use some more generic intrinsics.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: