Till Schneidereit [:till] Reporter
	Description • 9 years ago

With this, we can self-host the entire implementation code of builtins. Avoiding the need to even define a Class would be nice, and is possible, but doesn't need to happen for this to be useful.

	Till Schneidereit [:till] Reporter
	Comment 1 • 9 years ago

This works in the interpreter, but as soon as it enters the JITs, it reverts to creating normal objects instead of ones with the Class' layout and proto. I'll create a patch for testing this in a bit.

	Michael Wu [:mwu]
	Comment 2 • 9 years ago

Nice!

For DOM purposes, I don't think we would ever do this without specifying a Class, so that's fine.

	Jan de Mooij [:jandem]
	Comment 3 • 9 years ago

(In reply to Till Schneidereit [:till] from comment #1)
> This works in the interpreter, but as soon as it enters the JITs, it reverts
> to creating normal objects instead of ones with the Class' layout and proto.
> I'll create a patch for testing this in a bit.

If you can attach this patch I can take a look :)

	Till Schneidereit [:till] Reporter
	Comment 4 • 9 years ago

(In reply to Jan de Mooij [:jandem] from comment #3)
> If you can attach this patch I can take a look :)

Sorry for the delay :/


I realized that it's about as simple as I could make it with a specially crafted test case to instead just apply the patch in bug 911216 and then run the following script:


function test() {
    for (var i = 13; i--;) {
        new Promise(function(res, rej) {resolve = res, reject = rej});
    }
}

test();


I get a failed assert like this when the code tries to set a reserved slot, because the instance it operates on doesn't have the expected number of slots:
Assertion failure: index < (((getClass())->flags >> 8) & (((uint32_t)1 << (8)) - 1)), at /Users/till/mozilla/mozilla-inbound/js/src/vm/NativeObject.h:851
Segmentation fault: 11

	Jan de Mooij [:jandem]
	Comment 5 • 9 years ago

Comment on attachment 8693579 [details] [diff] [review]
Add support for self-hosted constructors

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

(In reply to Till Schneidereit [:till] from comment #1)
> This works in the interpreter, but as soon as it enters the JITs, it reverts
> to creating normal objects instead of ones with the Class' layout and proto.

To fix Baseline, you have to fix CreateThis in jit/VMFunctions.cpp to call CreateThisForSelfHostedConstructor if fun is a self-hosted constructor. (Now's probably a good time to factor this create-this logic out somewhere instead of duplicating it in a few places...)

For Ion, we have to fix IonBuilder::createThis. The fast paths there (createThisScriptedSingleton, createThisScriptedBaseline) are not used because the callee's proto doesn't match the template object's proto (which is expected, but we should probably fix these checks to do the right thing for self-hosted constructors).

Then we end up in IonBuilder::createThisScripted, that doesn't do what we want for self-hosted constructors. I think IonBuilder::createThis should not call createThisScripted if target->isSelfHostedConstructor(). Instead, it should just use MCreateThis if createThisScriptedSingleton and createThisScriptedBaseline don't work. 

I hope this helps.

	Eric Faust [:efaust]
	Comment 6 • 9 years ago

Do we have any idea what the ergonomics of using this would look like from self-hosted code? I don't see any tests or usage in the patch. How would I nominate the class to the MakeConstructor call in self-hosted JS?

	Morgan Phillips [:mrrrgn]
	Comment 7 • 8 years ago

Say Till, do you think you'll be able to get to this soon? If not, mind letting me pick up where you left off (granted that you don't mind me pestering you)?