922283 - Remove the self hosted class mechanism

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

This looks like dead code, and it is inlined by IonBuilder in a way that is incompatible with off thread MIR construction.

Comment 1

[Shu-yu Guo [: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.

Comment 2

[Shu-yu Guo [:shu]]

Niko and Felix, are you guys using the NewClassPrototype or NewObjectWithClassPrototype intrinsics in any typed object patches?

Comment 3

[Niko Matsakis [:nmatsakis]]

No. I do use `HasSameClass`, I think, but I don't need to be using it.

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

(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.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: more extensive removal

Remove the entire self hosted class mechanism.

Comment 6

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b0e4dee6b1a

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8b0e4dee6b1a

Comment 8

[Andy Wingo [:wingo]]

(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 :-((

Comment 9

[Shu-yu Guo [:shu]]

(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.

Comment 10

[Andy Wingo [:wingo]]

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.