See bug 460336 comment 14. Bug 460336 comment 7 gives an implementation of Array.prototype.join in JavaScript.

Bug 200505 made Array.join faster, so self hosting goes from 2.5% slower to 4x slower.

For what it's worth, on current tm tip the benchmark in bug 200505 shows that the self-hosted join from bug 460336 comment 7 is about 2x slower with -j -m. Curiously, the testcase is faster with either -j alone or -m alone. Filed bug 602479 on that.

Note also bug 602132.

I would like to find out what is required to do this and if it might be doable by me. Some question from irc: 1) When should this compiled/parsed - I think we should have one file with the js implementation and one with bytecode we load and distribute (xdr?) 2) What is about comparments 3) When and how do define this functions on the global object 4) (more a todo) Making the look native

I think self-hosting is really cool, but I wanted to point out that we keep optimizing Array.prototype.join b/c it keeps showing up (realistically or not) in benchmarks. The most recent patch took us from being slower than everyone to faster (bug 587257 comment 36) by making rather low-level optimizations that may be difficult to achieve in jitted JS. Not impossible, I'm just saying you should think about what the jit will be able to do vs. C++.

Please note that this question wasn't specifically about ".join". I know that v8 uses C++ code for exactly this in some cases.

Oḱay after this came up again, today, I am going to try to prototype something.

Just parroting irc for posterity: see the patch in bug 460904 comment 47. It addressed some of the same problems as would be faced with this bug, viz., how to load self-hosted JS and avoid the security/correctness hazard of content contaminating the environment in which the self-hosted JS is run.

I am not working on this right now.

I've modified js_DefineFunction and JSFunctionSpec to support self-hosting using minified one-line scripts. Currently, the JS source is simply embedded as a string in the JSFunctionSpec struct, but that could be changed to an #include and some build-time minification. As an example, I've added a self-hosted version of forEach. The resulting shell and browser work as expected and pass all tests, but the resulting performance numbers are weird: In the shell, the numbers are as expected: looping over an Array with 10.000.000 entries improves from ~1750ms to ~480ms. In the browser, however, the loop now takes almost 3s, or about the same time as in the shell without "-m -n". Based on that, the code doesn't get JITted for some reason. Other than an explanation for that, I'd be interested in feedback on the general approach I've taken.

Oh, and I forgot to mention that instead of re-compiling the source each time, it should be compiled once and then cloned on each consecutive use.

Comment on attachment 633753 [details] [diff] [review] First draft patch of self-hosting support Review of attachment 633753 [details] [diff] [review]: ----------------------------------------------------------------- I love how delightfully simple this patch is, nice work on that. I think, unfortunately, we need a bit more machinery to defend against content corrupting our self-hosted function. The main issues I see in forEach are that the Object and TypeError properties on the global object are mutable. To address this, I think we need a special "self-hosted" compilation mode (triggered by js_DefineFunctions) that allows a magic syntax to let self-hosted JS do what it needs to do (like get the original Object and TypeError values). This feature would also be necessary to self-host other natives (in particular, String.prototype.replace will want to call String.prototype.match (or an optimized version that doesn't create garbage)). To wit, v8 self-hosted JS uses magic syntax extensively. It would also be good to understand what is preventing jitting. I'd break right before a call to forEach and step through the JSOP_CALL (in particular, mjit::CanMethodJIT) to see what is going on.

(In reply to Luke Wagner [:luke] from comment #12) > Comment on attachment 633753 [details] [diff] [review] > First draft patch of self-hosting support > > Review of attachment 633753 [details] [diff] [review]: > ----------------------------------------------------------------- > > I love how delightfully simple this patch is, nice work on that. I think, > unfortunately, we need a bit more machinery to defend against content > corrupting our self-hosted function. The main issues I see in forEach are > that the Object and TypeError properties on the global object are mutable. > To address this, I think we need a special "self-hosted" compilation mode > (triggered by js_DefineFunctions) that allows a magic syntax to let > self-hosted JS do what it needs to do (like get the original Object and > TypeError values). This feature would also be necessary to self-host other > natives (in particular, String.prototype.replace will want to call > String.prototype.match (or an optimized version that doesn't create > garbage)). To wit, v8 self-hosted JS uses magic syntax extensively. That's a very good point. I guess one way to get around using special syntax would be to store references to the original values in a closure. Unfortunately, that would require not only compiling but also running the script immediately, so introducing a special compilation mode is in fact easier to implement. I will look into that next. > > It would also be good to understand what is preventing jitting. I'd break > right before a call to forEach and step through the JSOP_CALL (in > particular, mjit::CanMethodJIT) to see what is going on. Will do, thanks for the pointer. And thanks for the feedback in general, that's very helpful indeed!

Till, thanks for picking this up! Not only will this help performance for these methods, the self-hosting capability should be very valuable for future library work, like ECMAScript internationalization, and maybe even just to simplify some of our existing functions.

> It would also be good to understand what is preventing jitting. I'd break > right before a call to forEach and step through the JSOP_CALL (in > particular, mjit::CanMethodJIT) to see what is going on. Turns out it does get jitted after all: I just didn't know that code executed in the Scratchpad isn't jitted at all. Executing my simple benchmark in normal content script makes it 3x as fast as with the native forEach implementation. (In reply to David Mandelin from comment #14) > Till, thanks for picking this up! Not only will this help performance for > these methods, the self-hosting capability should be very valuable for > future library work, like ECMAScript internationalization, and maybe even > just to simplify some of our existing functions. Thanks David. I want to make self-hosting as easy to use as possible precisely so that it can be used heavily. Hopefully, that should get us into a position where the transition from a prototype implementation to a production-ready one is only a small bit of special syntax (plus tests) away.

So I thought about the problem of getting the original values of other builtins in self-hosted functions some more. AFAICT, new syntax doesn't get us that much closer to really solving this problem: we still don't (yet) store these original values anywhere. So how about we instead add a frozen, non-enumerable object "__builtins__" to the global object that gets initialized as the last step of GlobalObject::initStandardClasses and contains the original values of all builtins we care about in any self-hosted code? If exposing this object to content script is considered a problem, then we could hide it in some way and add special self-hosting-only syntax to reach it. Having a way to get at original values for builtins seems desirable to me even in content script, but I guess that would need to be proposed to TC39 or the W3C/ WHATWG or so.

(In reply to Till Schneidereit [:till] from comment #16) > AFAICT, new syntax doesn't get us that much closer to really solving this > problem: we still don't (yet) store these original values anywhere. We do, actually in the slots of the global object. We already have to do this since, e.g., ({}) always uses the original value of Object.prototype, not the current. There are nice typed accessors like js::GlobalObject::getOrCreateObjectPrototype and not-so-nice accessors that touch the slots directly (in, e.g., js_GetClassPrototype). Another benefit of magic syntax is that you can call magic conversion functions as necessary: v8 does this a lot, e.g., to test the int/double representation which not so cheap/easy to test from pure JS.

(In reply to Luke Wagner [:luke] from comment #17) > (In reply to Till Schneidereit [:till] from comment #16) > > AFAICT, new syntax doesn't get us that much closer to really solving this > > problem: we still don't (yet) store these original values anywhere. > > We do, actually in the slots of the global object. We already have to do > this since, e.g., ({}) always uses the original value of Object.prototype, > not the current. There are nice typed accessors like > js::GlobalObject::getOrCreateObjectPrototype and not-so-nice accessors that > touch the slots directly (in, e.g., js_GetClassPrototype). Ah, nice, thanks! So these values are deep copies of what's available to content scripts, i.e. all of their properties and their prototypes aren't changed if the content script counterpart is changed? So say I have the syntax "#builtin#Function.prototype.call", which would, roughly, mean "get the original value of "Function" and then look up "prototype.call" on it. If I understand correctly, the steps to do that are the following: 1. get current global 2. use js_GetClassPrototype to get the prototype of Function 3. continue with the normal lookup of "prototype" on Function, etc. I'm still not entirely sure how to get from a class name used in the script to the corresponding JSProtoKey. Do we have some kind of lookup table for that, or would I need to create one? Other than that, adding support for special lookup syntax sounds pretty doable. > > Another benefit of magic syntax is that you can call magic conversion > functions as necessary: v8 does this a lot, e.g., to test the int/double > representation which not so cheap/easy to test from pure JS. Right, I hadn't considered that. We would have to add more syntax for that, though, right? Meaning just adding support for "#builtin#" (or something similar) wouldn't gain us access to other internal engine functionality?

(In reply to Till Schneidereit [:till] from comment #18) Great questions! > Ah, nice, thanks! So these values are deep copies of what's available to > content scripts, i.e. all of their properties and their prototypes aren't > changed if the content script counterpart is changed? They are just copies of the single value. Thus, every single value that was needed "the original Function.prototype.call", "the original Object.prototype", etc would need to be acquired manually. Btw, a good reference is v8's self-hosted forEach: http://code.google.com/p/v8/source/browse/trunk/src/array.js#1062 Everything in CAPS and prefixed with % is magic; I guess the CAPS generate inline ops whereas the % is for naming and calling builtin functions?

Till, just wanted to say hello: I'm the editor of the ECMAScript Internationalization API Specification, and am just starting to work with Mozilla on implementing this API in SpiderMonkey. The idea is to combine my partial JavaScript implementation of the API with the ICU library, which provides the bulk of the functionality. Integrating the JavaScript code into SpiderMonkey requires support for self-hosted JavaScript code, so I'm really glad you're working on this! I haven't worked on SpiderMonkey before, so I'll need some time to come up to speed, and I'll then come back for a more detailed discussion. Do you have a requirements/design document by any chance, or is all relevant information in this bug? Some pointers for background: http://norbertlindenberg.com/2012/02/ecmascript-internationalization-api/ http://wiki.ecmascript.org/doku.php?id=globalization:specification_drafts http://norbertlindenberg.com/ecmascript/Intl.js

hey norbert, this all sounds great! after looking at the usage of self-hosting in v8 in some more detail, i've come to really understand how much library api can be implemented in js itself. hence, i plan on working towards making self-hosting as easy as possible, with the ideal outcome being to match v8 in this regard. if things go as planned, it should be possible to just code in js while using %identifier syntax to safely reference builtins. thus, looking at the link in comment 19 should give you a good idea of what to expect. i'm on vacation next week, so don't expect any progress until july, but i'm looking forward to working with you once i'm back.

Here's a first crack at defining the prerequisites for self-hosting the Intl module: - installing Intl and its constructors and related objects before any application code runs, but after ECMAScript built-ins are made available. - access to HasProperty (ES 5.1, 8.12.6), ToBoolean (ES 5.1, 9.2), ToNumber (ES 5.1, 9.3), ToUint32 (ES 5.1, 9.6), ToString (ES 5.1, 9.8), ToObject (ES 5.1, 9.9), CheckObjectCoercible (ES 5.1, 9.10). - access to standard implementations of ECMAScript functions such as Array.prototype.indexOf, and ability to call them safely, as discussed in several comments above. - access to functions that I'll implement in C++, such as compareStrings, formatNumber, and formatDateTime. - ability to create functions that only implement [[Call]], not [[Construct]]. - "internal properties": properties on objects that outside code cannot access, that aren't affected by setters on Object.prototype, but that are visible across the Intl package. Operations: set value (which also adds the property), get value, test presence. Not needed: delete. The use of internal properties is not limited to objects created within the Intl module - the spec requires that objects created by application code can be initialized with the internal properties of Collator, NumberFormat, DateTimeFormat. - "Record" type: Container of properties similar to Object, but not affected by changes to Object.prototype (I can probably implement this myself if necessary). - "List" type: Similar to Array, but not affected by changes to Array.prototype. Operations: push, indexOf, convert to normal Array (I may be able to implement this myself if necessary). - ability to keep source code in readable form in a separate text file - minified code could be generated at build time, but cannot be the source. The Intl module currently stands at over 2000 lines…

(In reply to Norbert Lindenberg from comment #22) > Here's a first crack at defining the prerequisites for self-hosting the Intl > module: Thanks for the detailed list! Some inline comments below: > - "internal properties": properties on objects that outside code cannot > access, that aren't affected by setters on Object.prototype, but that are > visible across the Intl package. Operations: set value (which also adds the > property), get value, test presence. Not needed: delete. The use of internal > properties is not limited to objects created within the Intl module - the > spec requires that objects created by application code can be initialized > with the internal properties of Collator, NumberFormat, DateTimeFormat. Ideally, we'd use private name objects to implement this, instead of using some proprietary functionality. Bug 645416 has a very bit-rotted patch implementing them. > > - "Record" type: Container of properties similar to Object, but not affected > by changes to Object.prototype (I can probably implement this myself if > necessary). Would Object.create(null) work for this or do you need an instance with the prototype set to an unmodified version of Object.prototype? > > - "List" type: Similar to Array, but not affected by changes to > Array.prototype. Operations: push, indexOf, convert to normal Array (I may > be able to implement this myself if necessary). This requirement should be met by providing access to an unmodified version of Array.prototype as explained in comment 19. > > - ability to keep source code in readable form in a separate text file - > minified code could be generated at build time, but cannot be the source. > The Intl module currently stands at over 2000 lines… This is an important requirement in general. Ideally, we will be able to re-use much of V8's preprocessing stuff or at least implement something very similar.

Hey, Till. How's it going? Do you need any help from us?

(In reply to David Mandelin [:dmandelin] from comment #24) > Hey, Till. How's it going? Do you need any help from us? Hey David, thanks for asking! I'm close to having a first, rough, implementation of support for calling intrinsics with the syntax "%intrinsicsName(args)". I'll combine that with the self-hosting stuff I already did and attach the results tomorrow to get some feedback. After that, I'll have lots of questions about both the specifics of my implementation and about how best to integrate JS source into the build process.

An update on the requirements in comment 22: - I got Record and List to work based on Object.create(null) and original Array methods, so these two are off the list. - I found I need a few more capabilities to meet the requirements for built-in functions, which are a bit different from the properties of normal functions. See (3a) below. - Dave suggested that I prioritize the requirements a bit. So here's the updated list: (1) Needed to get up and running: (1a) Ability to keep source code in readable form in a separate text file - minified code could be generated at build time, but cannot be the source. (1b) Installing Intl and its constructors and related objects before any application code runs, but after ECMAScript built-ins are made available. (1c) Access to standard implementations of ECMAScript functions such as Array.prototype.indexOf, and ability to call them safely, as discussed in several comments above. (1d) Access to functions that I'll implement in C++, such as compareStrings, formatNumber, and formatDateTime. (2) Needed to provide a secure environment: (2a) "Internal properties": properties on objects that outside code cannot access, that aren't affected by setters on Object.prototype, but that are visible across the Intl package. Operations: set value (which also adds the property), get value, test presence. Not needed: delete. The use of internal properties is not limited to objects created within the Intl module - the spec requires that objects created by application code can be initialized with the internal properties of Collator, NumberFormat, DateTimeFormat. I looked at the Harmony proposal for private names, and it looks like they should meet my needs. (3) Needed to pass compliance tests: (3a) Ability to create functions that only implement [[Call]], not [[Construct]], do not have a prototype property, and have a length property that can be first set and then be made non-writable. (4) Needed for better performance and less redundancy: (4a) Access to HasProperty (ES 5.1, 8.12.6), ToBoolean (ES 5.1, 9.2), ToNumber (ES 5.1, 9.3), ToUint32 (ES 5.1, 9.6), ToString (ES 5.1, 9.8), ToObject (ES 5.1, 9.9), CheckObjectCoercible (ES 5.1, 9.10).

Not completely there, yet, but I'm very close to having all the pieces needed to demonstrate how this can all work. Unfortunately, I'm out of time for today, so I'll attach the preliminary results tomorrow. (In reply to Norbert Lindenberg from comment #26) > So here's the updated list: > > (1) Needed to get up and running: > > (1a) Ability to keep source code in readable form in a separate text file - > minified code could be generated at build time, but cannot be the source. The exact mechanism of how to process the readable source at build time is what I'm still the least clear about. My hope is, though, to be able to pretty much use what V8 has for that - if that's not prevented by licensing issues, that is. > > (1b) Installing Intl and its constructors and related objects before any > application code runs, but after ECMAScript built-ins are made available. This will work in much the same way as the setup of other built-ins, with all built-ins having the option of being implemented using self-hosted JS or JSNatives. > > (1c) Access to standard implementations of ECMAScript functions such as > Array.prototype.indexOf, and ability to call them safely, as discussed in > several comments above. I currently have the ability of getting builtin functions via the %_Name syntax. As that syntax only allows for getting functions as if they were globals, they have to be called with the intended this-object as an argument. Example: Instead of calling myArray.indexOf(foo) you have to call %_ArrayIndexOf(myArray, foo) The final part that missing before I can demonstrate a simple example is a mechanism for invoking the indexOf builtin with the this-object properly set. My current thinking is that that will look very similar to the js_generic_native_method_dispatcher defined in jsapi.cpp. > > (1d) Access to functions that I'll implement in C++, such as compareStrings, > formatNumber, and formatDateTime. This will work in the same way as accessing other builtin functions. > > (2) Needed to provide a secure environment: > > (2a) "Internal properties": properties on objects that outside code cannot > access, that aren't affected by setters on Object.prototype, but that are > visible across the Intl package. Operations: set value (which also adds the > property), get value, test presence. Not needed: delete. The use of internal > properties is not limited to objects created within the Intl module - the > spec requires that objects created by application code can be initialized > with the internal properties of Collator, NumberFormat, DateTimeFormat. I > looked at the Harmony proposal for private names, and it looks like they > should meet my needs. The way I have currently implemented access to builtin functions, they're set as properties of an "intrinsics holder" JSObject in a slot of the globalObject. This holder object can be used as the scope for storing internal properties. One possible way to access them would be by introducing a function %_GetInternal("name"). > > (3) Needed to pass compliance tests: > > (3a) Ability to create functions that only implement [[Call]], not > [[Construct]], do not have a prototype property, and have a length property > that can be first set and then be made non-writable. I'll have to talk to Luke or get help on #jsapi for that as I don't know if and how that's possible in SpiderMonkey. > > (4) Needed for better performance and less redundancy: > > (4a) Access to HasProperty (ES 5.1, 8.12.6), ToBoolean (ES 5.1, 9.2), > ToNumber (ES 5.1, 9.3), ToUint32 (ES 5.1, 9.6), ToString (ES 5.1, 9.8), > ToObject (ES 5.1, 9.9), CheckObjectCoercible (ES 5.1, 9.10). These can most likely be made available as %_HasProperty, etc.

(In reply to Till Schneidereit [:till] from comment #25) > (In reply to David Mandelin [:dmandelin] from comment #24) > > Hey, Till. How's it going? Do you need any help from us? > > Hey David, thanks for asking! I'm close to having a first, rough, > implementation of support for calling intrinsics with the syntax > "%intrinsicsName(args)". I'll combine that with the self-hosting stuff I > already did and attach the results tomorrow to get some feedback. Sweet. By the way, since this is not exposed to content, and no one's using it just yet, you should feel free to land it in pieces and fix it up as you go. We can just document which pieces are solid enough to be used (more to the point, that you don't mind people using at that point). No need to create one giant patch (in fact, incremental development is greatly preferred.)

These three patches add support for invoking functions with the syntax "%functionName". To make a function available via this syntax, it needs to be added to the intrinsic_functions array. Setting the flag JSFUN_INTRINSIC_METHOD means that the call is routed through js_generic_native_method_dispatcher to use the first argument as the |this| value. To store the intrinsic functions, I added a new slot to global objects, containing an "intrinsics holder". This can be used to store not only the functions themselves, but also any internal state that might be required, which can then be retrieved by using a (yet to be introduced) intrinsic function along the lines of %_GetInternal("propname"). Problems that I'm aware of: - the intrinsic functions stuff should probably be moved into its own source file, or at least moved to a different part of GlobalObject.cpp - I haven't yet added the flag that enables the intrinsic call syntax - it's currently valid in all JS code. That's because I'm not sure how to set it for tests in the shell and because I want to wait for bug 771705 to land before adding the compile mode Things that I'm unsure about: - the whole approach of using a JSObject as a "holder" for intrinsic functions and then sort of borrowing them. I'm not sure if there's a problem associated with that such as too much memory overhead or something - how to integrate this with the JITs. I really don't know much of anything about the JITs at all, so I'll probably ask lots of questions on #jsapi

So I managed to at least add basic JM support for JSOP_CALLINTRINSIC. With all patches applied, the attached testcase takes about 15ms without a thisarg (which makes it not use %_CallFunction) and about 260ms with one (which does make it use %_CallFunction). For comparison, using the native Array.prototype.forEach (e.g. by commenting out line 47 of the testcase), both cases take about 180ms for me. Commenting out/in lines 35 and 36, respectively (thereby replacing %_CallFunction with the specialized, but forgeable Function.prototype.call) takes the times to 15ms and 25ms without and with thisarg. It seems to me like it should be possible to completely remove the lookup for %_CallFunction and instead emit code that immediately invokes the given function with the correct object as the |this| value. In difference to Function.prototype.call, this shouldn't even need a guard, as we guarantee %_CallFunction to always mean the same. For other %_-functions, we should at least be able to cache the returned JSFunction* without any guards, as it has to be stable anyway.

Oh, and the patch series passes shell and jit tests, but I didn't yet try the browser.

This worked out pretty nicely, getting the forEach benchmark to run just as fast with a receiver as without one: both cases now take ~15ms instead of ~185ms on my computer. For rewriting the argument nodes to work, I had to change EmitCallOrNew to derive argc from the number of emitted argument OPs. I'm not sure if simply deleting the fun and reveicer nodes like this isn't too dirty, really. As far as I can tell, the other options would involve either changing EmitNameOp to also receive the parent node and properly delete the child nodes from that, or to have a feedback mechanism from EmitNameOp to emitCallOrNew to tell it to omit the first and last arg nodes. Oh, and the atom for _CallFunction should probably be store somewhere instead of atomizing the char[] on each encounter of JSOP_INTRINSIC_NAME

With all patches applied, the attached benchmark executes both loops in ~15ms for me. That's compared to ~185ms each with the built-in Array.prototype.forEach and 120ms/47ms in V8 with their self-hosted Array.prototype.forEach.

Wow, this is looking great! Nice job jumping into the front-end. I'll start with some high-level questions/comments: With the %_CallFunction change you made in the last patch, can we avoid using generic natives altogether and the associated changes in the third patch? Instead of having a holder object, I think we could just use a plain array of HeapPtr<JSFunction> stored in the JSCompartment (since there is (now) one per global). This avoids bringing in various messy bits of objects. Later, when we need access to global properties, instead of %_GetInternal("foo"), I think we could just use %_Foo for a statically enumerated set of global properties. I think the main open question is how to load the self-hosted content. Scripts are per-compartment, so the simplest thing would be to re-parse every time we created a compartment. I think this would have non-trivial size/space overhead (esp. as we add more self-hosted content). Instead, I was thinking that we could: - when we initialize the runtime, parse the JS (without COMPILE_N_GO set) into the atoms compartment (analogous to how we do with staticStrings.init()), - when creating a new compartment, the HeapPtr<JSFunction> values start out 'null' - on the first call to GlobalObject::getIntrinsic for a given function, call js::CloneInterpretedFunction on the corresponding function in the atoms compartment to create a clone in the running compartment. This way, we only pay for the self-hosted functions that are actually used. Also, CloneInterpretedFunction is much faster than parsing. One remaining question is how to store the source for the runtime to parse. File access in the browser is complicated and scary, so perhaps you can just add a build step that creates a char array in some generated source file (eventually, after minification, etc). Any good JS minifiers written in Python? Lastly, while small patches are good, it is a bit hard to review when the early patches require later patches to understand; could you fold them together for future reviews? Second, could we separate out the actual self-hosted content (parse_intrinsic_names.js and shell.js) from the machinery? Probably jwalden would be a good reviewer for the latter.

(In reply to Luke Wagner [:luke] from comment #42) > Wow, this is looking great! Nice job jumping into the front-end. I'll > start with some high-level questions/comments: > > With the %_CallFunction change you made in the last patch, can we avoid > using generic natives altogether and the associated changes in the third > patch? Possibly, yes. We will, however, still need something like JSOP_CALLINTRINSIC for the other, non-specialcased, intrinsic function we want to call. I will check how many and which of these we'll probably need in addition to js_fun_call and whether they all work without the generic natives thing. > > Instead of having a holder object, I think we could just use a plain array > of HeapPtr<JSFunction> stored in the JSCompartment (since there is (now) one > per global). This avoids bringing in various messy bits of objects. Later, > when we need access to global properties, instead of %_GetInternal("foo"), I > think we could just use %_Foo for a statically enumerated set of global > properties. Ok, that sounds doable. As I want to keep the syntax identical to what V8 is using as much as possible, I will check what they're doing. But syntax aside, I agree about using a different storing mechanism. > > I think the main open question is how to load the self-hosted content. > Scripts are per-compartment, so the simplest thing would be to re-parse > every time we created a compartment. I think this would have non-trivial > size/space overhead (esp. as we add more self-hosted content). Instead, I > was thinking that we could: > - when we initialize the runtime, parse the JS (without COMPILE_N_GO set) > into the atoms compartment (analogous to how we do with > staticStrings.init()), > - when creating a new compartment, the HeapPtr<JSFunction> values start out > 'null' > - on the first call to GlobalObject::getIntrinsic for a given function, > call js::CloneInterpretedFunction on the corresponding function in the atoms > compartment to create a clone in the running compartment. That's about what I had in mind, yes. There's one problem with it, though: not using CNG means that TI won't work for self-hosted scripts. My forEach test-case takes about 3x to 4x as long without TI enabled, so this might be a problem. I will ask bhackett about that. > > This way, we only pay for the self-hosted functions that are actually used. > Also, CloneInterpretedFunction is much faster than parsing. One remaining > question is how to store the source for the runtime to parse. File access > in the browser is complicated and scary, so perhaps you can just add a build > step that creates a char array in some generated source file (eventually, > after minification, etc). Any good JS minifiers written in Python? V8 uses a Python script to preprocess, minify and compress the js sources and embed them into a header file. I want to look into taking that script, modifying is as little as possible. > > Lastly, while small patches are good, it is a bit hard to review when the > early patches require later patches to understand; could you fold them > together for future reviews? Second, could we separate out the actual > self-hosted content (parse_intrinsic_names.js and shell.js) from the > machinery? Probably jwalden would be a good reviewer for the latter. Makes sense, I will combine the next version of the patches for review. Those JS files aren't actually self-hosted content: they were attempts at adding a proper test-case. I removed them from the last round of patches. Actual self-hosted content is coming up.

(In reply to Till Schneidereit [:till] from comment #43) > Possibly, yes. We will, however, still need something like > JSOP_CALLINTRINSIC for the other, non-specialcased, intrinsic function we > want to call. Good point > I will check how many and which of these we'll probably need > in addition to js_fun_call and whether they all work without the generic > natives thing. My thinking was: if code wants to supply 'this', it should use %_CallFunction. Note, if there was some intrinsic _Foo we wanted to call supplying a 'this', we could write: %_CallFunction(thisValue, %_Foo). I noticed v8 has lots of uses like %_CallFunction(obj.receiver, ObjectToString); do you know if ObjectToString is a dynamically-resolved name or is it looked up in some static table? > There's one problem with it, though: > not using CNG means that TI won't work for self-hosted scripts. My forEach > test-case takes about 3x to 4x as long without TI enabled, so this might be > a problem. Good point. When we do the CloneInterpretedFunction, I think we could set COMPILE_N_GO on the newly-cloned script since, by construction, it hasn't been specialized to any global. > V8 uses a Python script to preprocess, minify and compress the js sources > and embed them into a header file. I want to look into taking that script, > modifying is as little as possible. I like your style :) Being able to reuse v8's self-hosted js with minimal effort sounds awesome.

Comment on attachment 643074 [details] [diff] [review] interpret op code for intrinsic function calls, rebased Review of attachment 643074 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/GlobalObject.cpp @@ +232,5 @@ > return NULL; > self->setThrowTypeError(throwTypeError); > > + RootedObject intrinsicsHolder(cx); > + intrinsicsHolder = JS_NewObject(cx, NULL, NULL, self); This can be passed as a second argument to the constructor above. @@ +236,5 @@ > + intrinsicsHolder = JS_NewObject(cx, NULL, NULL, self); > + if (!intrinsicsHolder) > + return NULL; > + self->setIntrinsicsHolder(intrinsicsHolder); > + JS_DefineFunctions(cx, intrinsicsHolder, intrinsic_functions); Need to check for failure.

Comment on attachment 643071 [details] [diff] [review] parse syntax for intrinsic function calls, rebased Review of attachment 643071 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsopcode.tbl @@ +224,5 @@ > OPDEF(JSOP_ENDINIT, 92, "endinit", NULL, 1, 0, 0, 19, JOF_BYTE) > OPDEF(JSOP_INITPROP, 93, "initprop", NULL, 5, 2, 1, 3, JOF_ATOM|JOF_PROP|JOF_SET|JOF_DETECTING) > OPDEF(JSOP_INITELEM, 94, "initelem", NULL, 1, 3, 1, 3, JOF_BYTE|JOF_ELEM|JOF_SET|JOF_DETECTING) > OPDEF(JSOP_INITELEM_INC,95, "initelem_inc", NULL, 1, 3, 2, 3, JOF_BYTE|JOF_ELEM|JOF_SET) > +OPDEF(JSOP_INTRINSIC_NAME,96, "intrinsic_name", NULL, 5, 0, 1, 19, JOF_ATOM|JOF_NAME|JOF_TYPESET) Consistency would seem to dictate that this should be one word: JSOP_INSTRINSICNAME.

@Benjamin: Thanks for the reviews. The next patch will address your feedback. (In reply to Luke Wagner [:luke] from comment #44) > My thinking was: if code wants to supply 'this', it should use > %_CallFunction. Note, if there was some intrinsic _Foo we wanted to call > supplying a 'this', we could write: %_CallFunction(thisValue, %_Foo). I > noticed v8 has lots of uses like %_CallFunction(obj.receiver, > ObjectToString); do you know if ObjectToString is a dynamically-resolved > name or is it looked up in some static table? Looking into V8's self-hosting some more, I think you're right: they're using %_CallFunction in precisely the way you're proposing. I will remove the GENERIC_METHOD stuff. > Good point. When we do the CloneInterpretedFunction, I think we could set > COMPILE_N_GO on the newly-cloned script since, by construction, it hasn't > been specialized to any global. But won't that code then need to be reparsed? IIUC, CNG leads to other bytecode being emitted, right? Also, at if we want to reuse V8's self-hosted content, we need to enable the functions to directly access each other, without any special syntax. And for the internationalization work, simply accessing fields would simplify things quite a bit. > I like your style :) Being able to reuse v8's self-hosted js with minimal > effort sounds awesome. Indeed :D

Comment on attachment 643074 [details] [diff] [review] interpret op code for intrinsic function calls, rebased Review of attachment 643074 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.h @@ +2509,5 @@ > > +#define JSFUN_INTRINSIC_METHOD 0x2000 /* Use same mechanism as for > + JSFUN_GENERIC_LAMBDA to use first > + argument as |this| parameter for > + intrinsic method-like functions */ I don't think this modification to the API is necessary or desirable. The use is pretty specialized, it's not clear from the comment or the code what the semantics are, and JSFUN_GENERIC_NATIVE is already a pretty ugly piece of the API which it'd be nice to see removed if embedders aren't using it. ::: js/src/jsinterpinlines.h @@ +364,5 @@ > + JSOp op = JSOp(*pc); > + RootedPropertyName name(cx); > + name = GetNameFromBytecode(cx, script, pc, op); > + if (!name) > + return false; GetNameFromBytecode is infallible, no return check needed. ::: js/src/vm/GlobalObject.cpp @@ +60,5 @@ > +test_fun(JSContext *cx, unsigned argc, Value *vp) > +{ > + printf("called\n"); > + return JS_TRUE; > +} rm @@ +66,5 @@ > namespace js { > > +JSFunctionSpec intrinsic_functions[] = { > + JS_FN("_ApplyFunction", js_fun_apply, 2,JSFUN_INTRINSIC_METHOD), > + JS_FN("_CallFunction", js_fun_call, 1,JSFUN_INTRINSIC_METHOD), Instead of using JSFUN_INTRINSIC_FUNCTION, you can define new natives which wrap js_fun_apply / js_fun_call. Maybe move the guts of js_generic_native_method_dispatcher into a header to avoid duplicating code.

Comment on attachment 643075 [details] [diff] [review] Basic JM support for JSOP_CALLINTRINSIC, rebased Review of attachment 643075 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I don't see any cases for the INTRINSIC opcodes in analyzeTypesBytecode in jsinfer.cpp. Those will be needed, otherwise the type information will be incorrect and you'll get a crash in debug mode. ::: js/src/jsanalyze.cpp @@ +314,2 @@ > case JSOP_CALLNAME: > + case JSOP_CALLINTRINSIC: The INTRINSIC opcodes do not depend on the scope chain, so they're more like GNAME opcodes than NAME opcodes and should go with those cases below (will allow more optimization).

Comment on attachment 643076 [details] [diff] [review] special-casing of %_CallFunction in the BytecodeEmitter, rebased Review of attachment 643076 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +1724,5 @@ > break; > case JSOP_INTRINSIC_NAME: > + if (pn->atom() == js_Atomize(cx, "_CallFunction", strlen("_CallFunction"))) { > + isCallFunction = true; > + op = JSOP_CALLNAME; This assignment to op is a bit confusing, it doesn't look like the variable will be used at all later in the function when isCallFunction is set. Can you set op = JSOP_CALLINTRINSIC unconditionally in this case? Also, the js_Atomize needs a NULL check, or (better, but don't know how hard this is) "_CallFunction" could be an atom in the runtime's atomState. @@ +1764,5 @@ > + pn->pn_next = pn->pn_next->pn_next; > + funNode = pn->pn_next; > + ParseNode *pn2; > + for (pn2 = pn->pn_next; pn2->pn_next->pn_next; pn2 = pn2->pn_next); > + pn2->pn_next = NULL; This looks OK, but I'm worried about the modification later on which removes the use of pn_count. The transformations done here should preserve the correctness of the pn_count information for the parse nodes, so that the later change isn't necessary.

(In reply to Brian Hackett (:bhackett) from comment #48) > I don't think this modification to the API is necessary or desirable. The > use is pretty specialized, it's not clear from the comment or the code what > the semantics are, and JSFUN_GENERIC_NATIVE is already a pretty ugly piece > of the API which it'd be nice to see removed if embedders aren't using it. Agreed. Luke also pointed that out and I have since removed it in my patch queue. What's more, the later changes to the bytecode for specializing Function.call make even the one current use-case go away. > Instead of using JSFUN_INTRINSIC_FUNCTION, you can define new natives which > wrap js_fun_apply / js_fun_call. Maybe move the guts of > js_generic_native_method_dispatcher into a header to avoid duplicating code. As said above, this isn't needed anymore at all and has been removed entirely. > Looks good, but I don't see any cases for the INTRINSIC opcodes in > analyzeTypesBytecode in jsinfer.cpp. Those will be needed, otherwise the > type information will be incorrect and you'll get a crash in debug mode. They're contained in another patch. At Luke's request (and after seeing that the combined diff isn't all that big), I have since folded the entire patch queue into one patch. Subsequent versions will reflect that. > The INTRINSIC opcodes do not depend on the scope chain, so they're more like > GNAME opcodes than NAME opcodes and should go with those cases below (will > allow more optimization). Cool, will change to that. > ::: js/src/frontend/BytecodeEmitter.cpp > @@ +1724,5 @@ > > break; > > case JSOP_INTRINSIC_NAME: > > + if (pn->atom() == js_Atomize(cx, "_CallFunction", strlen("_CallFunction"))) { > > + isCallFunction = true; > > + op = JSOP_CALLNAME; > > This assignment to op is a bit confusing, it doesn't look like the variable > will be used at all later in the function when isCallFunction is set. Can > you set op = JSOP_CALLINTRINSIC unconditionally in this case? Also, the > js_Atomize needs a NULL check, or (better, but don't know how hard this is) > "_CallFunction" could be an atom in the runtime's atomState. Adding "_CallFunction" as an atom is on my todo list - always re-atomizing is a crutch to get things going for now. > > @@ +1764,5 @@ > > + pn->pn_next = pn->pn_next->pn_next; > > + funNode = pn->pn_next; > > + ParseNode *pn2; > > + for (pn2 = pn->pn_next; pn2->pn_next->pn_next; pn2 = pn2->pn_next); > > + pn2->pn_next = NULL; > > This looks OK, but I'm worried about the modification later on which removes > the use of pn_count. The transformations done here should preserve the > correctness of the pn_count information for the parse nodes, so that the > later change isn't necessary. Apparently, that's not the case: I got a failing ASSERT because pn_count wasn't correct anymore. I couldn't see any way to fix that up from within EmitNameOp - hence the change in EmitCallOrNew.

(In reply to Till Schneidereit [:till] from comment #47) > > Good point. When we do the CloneInterpretedFunction, I think we could set > > COMPILE_N_GO on the newly-cloned script since, by construction, it hasn't > > been specialized to any global. > > But won't that code then need to be reparsed? When we initially parse the script without CNG (at runtime-init time), the parser will refrain from making any assumptions about the contents of the global. Thus, if we clone the script into a compartment, it is safe to add CNG b/c the cloned script makes no assumptions and will henceforth only be run with a single global. > Also, at if we want to reuse V8's self-hosted > content, we need to enable the functions to directly access each other, > without any special syntax. And for the internationalization work, simply > accessing fields would simplify things quite a bit. Hrm, yes, !CNG would produce JSOP_NAME for all inter-function references which means a totally dynamic lookup which is a chance for content to corrupt things. We definitely need to bind, at compile-time, all references between self-hosted functions. Yesterday I was talking with billm about this and we thought that we should assert (when compiling self-hosted code) that there are no unbound name accesses in self-hosted code since it would almost certainly be a bug. I think there is a simple solution that addresses both these concerns: in BindNameToSlot (in the emitter, where we try to specialize names to local, formal, global access), for any unbound name (the dn->pn_cookie.isFree() branch) where we would try to specialize to a global in CNG code, in self-hosted compiling mode we lookup that atom in the static table of self-hosted functions/globals; if we find it, we emit the equivalent of %_CallFunction, if not, we error out (which should never occur).

This version is pretty much complete. It fully handles all %funName cases and contains a special-case for %_CallFunction that compiles it to optimal bytecode directly calling the given function in the correct scope. The only thing still missing is hiding the special syntax behind a flag. I'm waiting on bug 771705 for that.

Applied ontop of the previous patch, this one embeds a self-hosted version of Array#forEach and correctly installs it on the Array prototype. Things I still have to work on: - cloning functions into a special object instead of the target compartment's global to hide them from content. - externalizing JS code and automatically minifying it during the build process. Numbers: The following script took ~185ms on trunk and takes ~5ms to ~6ms with the patch applied: var arr = new Array(1000000); for (var i = 0; i < arr.length; ++i) arr[i] = i; var start = new Date; arr.forEach(function f(val, idx, list) {list[idx] = val * val;}, {}); print(new Date - start);

This version works in a full browser build, too, and adds preliminary support for Array.forEach. The in-browser numbers are a little bit worse than the in-shell ones: 10ms in top-level code, 16ms in functions for the same benchmark. That's still a 18x and 11x speedup, respectively. And for comparison, in Chrome 21 the testcase takes between 100ms and 160ms (with highly fluctuating numbers, for some reason).

Comment on attachment 645087 [details] [diff] [review] Support parsing %funName() syntax, emitting correct bytecode and handling it in the interpreter and mjit Review of attachment 645087 [details] [diff] [review]: ----------------------------------------------------------------- Looking good, once the "//TODO: add check for self-hosting parse mode" gets sorted out. ::: js/src/frontend/Parser.cpp @@ +6626,5 @@ > + return NULL; > + } > + > + PropertyName *name = tokenStream.currentToken().name(); > + ParseNode *node = NameNode::create(PNK_NAME, name, this, this->tc); To catch errors, could this do a table lookup and issue an error if 'name' isn't in some list of intrinsics? ::: js/src/vm/GlobalObject.h @@ +350,5 @@ > + JSFunction *getIntrinsicFunction(JSContext *cx, PropertyName *name) { > + Value holder = getSlotRef(INTRINSICS); > + Value fun; > + holder.toObject().getProperty(cx, name, &fun); > + return fun.toObject().toFunction(); Could you write this as: Value fun = NullValue(); DebugOnly<bool> ok = HasDataProperty(cx, holder.toObject(), NameToId(name), &fun); JS_ASSERT(ok); return fun.toObject().toFunction(); this should allow fuzzers etc to catch mischieve on the holder object and, should it occur in the wild, prevent exploitation via a safe crash at NULL.

(In reply to Luke Wagner [:luke] from comment #56) Thanks for the quick review! > Looking good, once the "//TODO: add check for self-hosting parse mode" gets > sorted out. I have now added a flag "allowIntrinsicsCalls" to the parser that gets set through the new CompilerOptions struct. That means that when only applying this patch, there's no way to actually parse the new syntax at all, which should hopefully make it landeable, right? > > ::: js/src/frontend/Parser.cpp > > + PropertyName *name = tokenStream.currentToken().name(); > > + ParseNode *node = NameNode::create(PNK_NAME, name, this, this->tc); > > To catch errors, could this do a table lookup and issue an error if 'name' > isn't in some list of intrinsics? It can, but I'm not sure where to put that. The simplest thing would be to just look at the JSFunctionSpec[] in GlobalObject.cpp, but I'm a bit hesitant to expose that through the header and include that header into Parser.cpp. Should I maybe extract the self-hosting stuff into a new .h/.cpp pair?

Comment on attachment 646691 [details] [diff] [review] Support parsing %funName() syntax, emitting correct bytecode and handling it in the interpreter and mjit, v2 Review of attachment 646691 [details] [diff] [review]: ----------------------------------------------------------------- Almost done, just one non-trivial nit below. Landing this earlier sounds fine. Regarding the early-checking-of-intrinsic names, how about adding a GlobalObject::hasIntrinsicFunction(PropertyName*)? ::: js/src/frontend/BytecodeEmitter.cpp @@ +1749,5 @@ > op = JSOP_CALLNAME; > break; > + case JSOP_INTRINSICNAME: > + op = JSOP_CALLINTRINSIC; > + if (pn->atom() == js_Atomize(cx, "_CallFunction", strlen("_CallFunction"))) Could you put _CallFunction into jsatom.tbl so we don't have to re-atomize on every execution? @@ +1786,5 @@ > + if (!EmitTree(cx, bce, pn->pn_next)) > + return false; > + pn->pn_next = pn->pn_next->pn_next; > + lastArgNode->pn_next = NULL; > + return true; Since you are removing a link from the list, I think you should decrement pn_count to match it. Otherwise it is just sitting there, waiting for some unwary code to trip over it. I know, e.g., that we have various functions that generically traverse the parse tree. Also, this would allow you to remove the 'argc' change in EmitCallOrNew. I guess the problem is that we don't have a pointer to the list node. This suggests hoisting this special-casing logic out of EmitNameOp and into the PNK_NAME case of EmitCallOrNew. I think this would be an improvement since, logically, this transformation pertains to calls, not names. ::: js/src/jsapi.h @@ +4952,5 @@ > const char *filename; > unsigned lineno; > bool compileAndGo; > bool noScriptRval; > + bool allowIntrinsicsCalls; from irc: don't forget to init in CompileOptions ctor ::: js/src/jsopcode.tbl @@ +224,5 @@ > OPDEF(JSOP_ENDINIT, 92, "endinit", NULL, 1, 0, 0, 19, JOF_BYTE) > OPDEF(JSOP_INITPROP, 93, "initprop", NULL, 5, 2, 1, 3, JOF_ATOM|JOF_PROP|JOF_SET|JOF_DETECTING) > OPDEF(JSOP_INITELEM, 94, "initelem", NULL, 1, 3, 1, 3, JOF_BYTE|JOF_ELEM|JOF_SET|JOF_DETECTING) > OPDEF(JSOP_INITELEM_INC,95, "initelem_inc", NULL, 1, 3, 2, 3, JOF_BYTE|JOF_ELEM|JOF_SET) > +OPDEF(JSOP_INTRINSICNAME,96, "intrinsic_name", NULL, 5, 0, 1, 19, JOF_ATOM|JOF_NAME|JOF_TYPESET) Could you keep these two ops together (say, JSOP_UNUSED8/9?)

Comment on attachment 645243 [details] [diff] [review] Embed self-hosted JS and install it into standard classes, v2 Review of attachment 645243 [details] [diff] [review]: ----------------------------------------------------------------- Nice, sorry for the slow review. ::: js/src/jsapi.cpp @@ +925,5 @@ > JS_ASSERT(onOwnerThread()); > > delete_(debugScopes); > > + gcRootsHash.remove((void *)selfHostedGlobal_.address()); This doesn't seem necessary. @@ +4715,2 @@ > { > + JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_CLONE_FUNOBJ_SCOPE); You might also want to report an error if fun->compileAndGo and !parent->isGlobal(). This API is pretty ill defined and it might be accessible to chrome code (?) so should be defensive. ::: js/src/jscntxt.cpp @@ +220,5 @@ > + RootedObject savedGlobal(cx, JS_GetGlobalObject(cx)); > + if (!(selfHostedGlobal_ = JS_NewGlobalObject(cx, &self_hosting_global_class, NULL))) > + return false; > + JS_AddObjectRoot(cx, selfHostedGlobal_.address()); > + JS_SetGlobalObject(cx, selfHostedGlobal_); The JS_SetGlobalObject calls here and saving of the old global aren't necessary here, they control cx->globalObject which is basically irrelevant to the engine's behavior (it is used by the DOM / xpconnect in pretty screwed up ways and needs to die). @@ +258,5 @@ > + { > + return NULL; > + } > + } > + RootedObject clone(cx, JS_CloneFunctionObject(cx, &funVal.toObject(), targetGlobal)); Saving the targetGlobal before the AutoCompartment doesn't seem necessary, you could just use cx->global() here. ::: js/src/jscntxt.h @@ +406,5 @@ > #ifdef JS_METHODJIT > js::mjit::JaegerRuntime *jaegerRuntime_; > #endif > > + js::RootedObject selfHostedGlobal_; This needs to be a plain JSObject* or HeapPtrObject, using RootedObject and specifying the cx as NULL during construction will crash in builds where exact rooting is performed. ::: js/src/jsfun.cpp @@ +1481,5 @@ > gop = NULL; > sop = NULL; > } > > + if (native) { This logic needs asserts and comments to explain what the JSFunctionSpec invariants are. The indenting for the js_NewFunction call is also broken.

(In reply to Luke Wagner [:luke] from comment #58) > Review of attachment 646691 [details] [diff] [review]: > ----------------------------------------------------------------- Thanks again for the fast review, much appreciated! I have addressed all of your feedback, added a few asserts and pushed the result to try: https://tbpl.mozilla.org/?tree=Try&rev=553ffa207edc

Try run for 553ffa207edc is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=553ffa207edc Results (out of 16 total builds): success: 16 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tschneidereit@gmail.com-553ffa207edc

Green on try.

(In reply to Brian Hackett (:bhackett) from comment #59) > Comment on attachment 645243 [details] [diff] [review] > Embed self-hosted JS and install it into standard classes, v2 > > Review of attachment 645243 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice, sorry for the slow review. No worries, I know how busy you are. > > + gcRootsHash.remove((void *)selfHostedGlobal_.address()); > > This doesn't seem necessary. I get a leak warning on shell shutdown without it. > The JS_SetGlobalObject calls here and saving of the old global aren't > necessary here, they control cx->globalObject which is basically irrelevant > to the engine's behavior (it is used by the DOM / xpconnect in pretty > screwed up ways and needs to die). It seems like the shell uses it in some capacity as well: it crashes in NewContext without it. > > + js::RootedObject selfHostedGlobal_; > > This needs to be a plain JSObject* or HeapPtrObject, using RootedObject and > specifying the cx as NULL during construction will crash in builds where > exact rooting is performed. It only ever gets constructed with a proper cx and lets me get away with not explicitly removing the object root during runtime destruction. Should I still change it, and if so, do you think a plain JSObject* is ok given that it shouldn't ever be reassigned during a runtime's lifetime?

This version addresses bhackett's feedback and adds support for using externally defined JS scripts as self-hosted code. To do that, it imports a few Python scripts from V8 and adds a make target to use them to create a header (selfhosting.out.h) containing the minified version of (for now) builtin/array.js. I tried changing the V8 scripts as little as possible, so there's quite a lot of stuff in there that we don't use yet, or in some cases can't even use. Case in point: build/macros.py defines a lot of macros that use intrinsics we haven't (yet) defined. As my makefile-foo is far too weak for Mozilla's makefiles, this setup for now requires manual invokation of "make prepare-self-hosting" after changes to the self-hosted code. Apart from that, this code works for Array.prototype.forEach, but has one final, pretty big, hole to it: the self-hosted functions can't refer to each other. Because of that Array.forEach is currently implemented using a reference to Array.prototype.forEach, which can be changed by user-code. Ideally, the code would just be able to refer to ArrayForEach, but that's not possible as it gets installed such that its |this| is Array, not the original scope it got compiled in.

If you are messing with the build system, make sure to get a review from a build system peer like khuey or Ted Mielczarek.

(In reply to Andrew McCreight [:mccr8] from comment #65) > If you are messing with the build system, make sure to get a review from a > build system peer like khuey or Ted Mielczarek. I'm not really planning to mess with it myself, at the very least not without consulting with someone who knows a great deal more about it. :) But yes: I'll make sure to get that reviewed by the right people, especially in light of the planned changes to the build system.

Here's where we stand with the latest couple of patches in terms of requirements for the internationalization stuff: > (1a) Ability to keep source code in readable form in a separate text file - > minified code could be generated at build time, but cannot be the source. check. Just add a file intl.js or similar in the "builtin" dir and change the makefile target "prepare-self-hosting" to include it in selfhosting.out.h. (The exact place to edit might change a bit.) > > (1b) Installing Intl and its constructors and related objects before any > application code runs, but after ECMAScript built-ins are made available. For now, this can be implemented using a JSFunctionSpec[] as is done in jsarray.cpp & co. We might want to enable installation of methods and properties from within self-hosted code later on, but this would add quite a bit of complication, so I think we should first land what we have now. > > (1c) Access to standard implementations of ECMAScript functions such as > Array.prototype.indexOf, and ability to call them safely, as discussed in > several comments above. All required functions have to be installed using the JSFunctionSpec intrinsic_functions[] in GlobalObject.cpp for now. I toyed with a way to make all self-hosted functions automatically available using %name (e.g. %ArrayForEach for the currently implemented Array.prototype.forEach), but I hope that I can instead ad a way to just refer to them by their normal name. See the last paragraph of comment 64. > > (1d) Access to functions that I'll implement in C++, such as compareStrings, > formatNumber, and formatDateTime. These definitely have to be added to the intrinsic_functions[] array, which should be straight-forward. > > (2) Needed to provide a secure environment: > > (2a) "Internal properties": properties on objects that outside code cannot > access, that aren't affected by setters on Object.prototype, but that are > visible across the Intl package. Operations: set value (which also adds the > property), get value, test presence. Not needed: delete. The use of internal > properties is not limited to objects created within the Intl module - the > spec requires that objects created by application code can be initialized > with the internal properties of Collator, NumberFormat, DateTimeFormat. I > looked at the Harmony proposal for private names, and it looks like they > should meet my needs. Again, see the last paragraph of comment 64. Alternatively, a holder object for these properties could be accessed using something like %_Internals().propName, where %_Internals just returns a normal object that's not accessible to client code. > > (3) Needed to pass compliance tests: > > (3a) Ability to create functions that only implement [[Call]], not > [[Construct]], do not have a prototype property, and have a length property > that can be first set and then be made non-writable. I haven't looked into this, yet. > > (4) Needed for better performance and less redundancy: > > (4a) Access to HasProperty (ES 5.1, 8.12.6), ToBoolean (ES 5.1, 9.2), > ToNumber (ES 5.1, 9.3), ToUint32 (ES 5.1, 9.6), ToString (ES 5.1, 9.8), > ToObject (ES 5.1, 9.9), CheckObjectCoercible (ES 5.1, 9.10). Some of these should be implemented as intrinsic functions, some as macros in "build/macros.py". For now, this file is just a verbatim copy from V8, so it contains a lot of macros we can't use without further work. All in all, I think there's not that much more work to be done to get things ready for usage in implementing Intl.

Comment on attachment 646902 [details] [diff] [review] Support parsing %funName() syntax, emitting correct bytecode and handling it in the interpreter and mjit, v3 Review of attachment 646902 [details] [diff] [review]: ----------------------------------------------------------------- Nicely done, thanks for addressing the comments! The parse-node mutation during emitting makes me nervous, but I can't think of any way it could go wrong, atm, nor a simpler way that doesn't involve mutation, so let's try it out. ::: js/src/frontend/BytecodeEmitter.cpp @@ +5341,5 @@ > if (!EmitNameOp(cx, bce, pn2, callop)) > return false; > break; > + case PNK_INTRINSICNAME: > + pn2->setKind(PNK_NAME); Can you add a comment before setKind to the effect of: "This setKind and the branch below mean that emitting an intrinsic name destructively mutates the parse tree. This is ok because there are currently no cases where a parse sub-tree is emitted twice." ::: js/src/frontend/Parser.cpp @@ +6504,5 @@ > + } > + > + PropertyName *name = tokenStream.currentToken().name(); > + if (!(name == context->runtime->atomState._CallFunctionAtom || > + context->global()->hasIntrinsicFunction(context, name))) Since the 'hasIntrinsicFunction' call is inside the parens, could you align 'context' with 'name'?

I took another stab at changing EmitCallOrNew. This version doesn't do any parse-node mutation. Instead, it emits the args inside the special-casing block for %_CallFunction and adds a flag `emitArgs` that guards against re-emitting them below. While this adds a small amount of code-duplication, it feels much cleaner and prevents any problems with the rewritten parse-node down the road. I also removed the mutation of the PNK and added the required conditions to all places that asserted because of that. I'm fairly certain that the restricted usage of INTRINSICNAME makes these additions sufficient, but even if not, any problems arising from other uses will be caught very early on as all self-hosted code is parsed on startup, anyway. All additional changes are in BytecodeEmitter.cpp, with all except the ASSERT modifications within EmitCallOrNew.

Comment on attachment 647491 [details] [diff] [review] Support parsing %funName() syntax, emitting correct bytecode and handling it in the interpreter and mjit, v4 Review of attachment 647491 [details] [diff] [review]: ----------------------------------------------------------------- Excellent choice! I was considering requesting a non-mutating version in the last review, but I couldn't come up with a good non-purely-aesthetic reason. ::: js/src/frontend/BytecodeEmitter.cpp @@ +5356,5 @@ > + * > + * argc is set to the amount of actually emitted args and the > + * emitting of args below is disabled by setting emitArgs to false. > + */ > + JS_ASSERT(pn->pn_count > 2); I think we need to dynamically check this since it is not ensured by parsing. @@ +5366,5 @@ > + if (!EmitTree(cx, bce, pn2->pn_next)) > + return false; > + bool oldInForInit = bce->inForInit; > + bce->inForInit = false; > + for (ParseNode *pn3 = pn2->pn_next->pn_next; pn3 != funNode; pn3 = pn3->pn_next) { Could you write: ParseNode *receiver = pn2->pn_next; above and then write the for-init: for (ParseNode *argpn = receiver->pn_next; ... (also s/pn3/argpn/)?

> Excellent choice! I was considering requesting a non-mutating version in > the last review, but I couldn't come up with a good non-purely-aesthetic > reason. I didn't want to be the one setting the precedent that we now do parse-node mutation in the bce :)

Comment on attachment 647570 [details] [diff] [review] Support parsing %funName() syntax, emitting correct bytecode and handling it in the interpreter and mjit, v5, carrying r+luke hrmpf

(In reply to Luke Wagner [:luke] from comment #73) > https://hg.mozilla.org/integration/mozilla-inbound/rev/f84910a8474a Unfortunately had to backout for xpcshell crashes: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f84910a8474a eg https://tbpl.mozilla.org/php/getParsedLog.php?id=14009262&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/ba24181ef04a

This appears to just be a assertion failure when initializing the INTRINSICS object. That's pretty weird. I'll have a quick look, maybe it's something stupid.

Trivial fix for the xpcshell-tests bustage.

This version adds support for directly accessing functions defined in self-hosted code from other self-hosted code. For example. Array.forEach is implemented in terms of Array.prototype.forEach, which it refers to as "ArrayForEach". As a side-effect, all intrinsic functions can now be accessed with or without the leading "%", with the exception of %_CallFunction, which gets compiled to special bytecode. Additionally, during global initialization one can install objects into the self-hosting holder such that they can be accessed by self-hosted code. In the current builtin/array.js, this is used to access the |Object()| function in line 4, which would normally not be resolvable from self-hosted code. As far as I can tell, this completes the set of features required to get most self-hosted code up and running. I'll look into proper build system integration next and deal with the more exotic requirements of the Intl implementation afterwards.

This version adds proper integration with the build system and some more proper exceptions. The only outstanding issues are: - making toString on self-hosted function show [native code] instead of the real function body. - show proper names in exceptions. E.g., invoking [].forEach() currently prints "self-hosted:5: TypeError: missing argument 0 when calling function ArrayForEach"

After a hacking session with evilpie, this is pretty much ready, I think. Exceptions now show the correct function names and omit frames from within self-hosted functions from the stack. Also, toString() correctly returns [native code] as the body. Tests are all passing except for one that I can't explain at all: jit-test/tests/jaeger/bug738525.js crashes with Assertion failure: allocated(), at ../gc/Heap.h:506 during shutdown of the shell. I reduced the problem to simply running the script "it.customNative = 1". I'll ping luke and bhackett about that problem. @gerv: I've added the following files that where imported from V8: js/src/jsmin.py js/src/macros.py js/src/js2c.py For now, changes are restricted to js2c.py, but we'll certainly have to change macros.py, too, at some point. @ted: Please review the changes to Makefile.in, which should be pretty much as we discussed on IRC. @bhackett: There's lots of new stuff compared to the version of the patch I f?'d you on. Please don't hesitate if you feel like asking me to change lots of stuff - I want to make this as clean as possible.

Just wondering if you have done any before/after performance testing with sunspider/v8/Kraken. Also, are there are any other functions which would help these benchmarks if self-hoisted.

(In reply to Michael Clackler from comment #82) > Just wondering if you have done any before/after performance testing with > sunspider/v8/Kraken. Also, are there are any other functions which would > help these benchmarks if self-hoisted. This won't help with these benchmarks, as they don't use Array#forEach, no. After the infrastructure for self-hosting lands, we'll look into moving more functionality over, so there might or might not be small wins to be had.

Importing files from V8 is file as long as you update the list of files in about:license above the V8 license, or in some way make sure it is still accurate. Gerv

Comment on attachment 648397 [details] [diff] [review] Embed self-hosted JS and install it into standard classes, v6 Review of attachment 648397 [details] [diff] [review]: ----------------------------------------------------------------- This seems to be going really well. I looked closely at the array.js file, to see what's it like coding with the new feature. I have a few questions/comments below. ::: js/src/builtin/array.js @@ +1,1 @@ > +function ArrayForEach(fun/*, receiver*/) { I suppose we don't have jit support for rest arguments yet, but we should do some followup work to get that going and change this function to use 'em. @@ +3,5 @@ > + %ThrowNoConstructorError(); > + if (arguments.length == 0) > + %ThrowMissingArgError(0); > + if (this == null || (typeof fun) !== 'function') > + %ThrowValueError(0, 22); Definitely want to do some followup to replace '22' with something better. It seems like if we had that we could also avoid (or embrace, depending on how it's done!) having one intrinsic for each kind of error to report. Is the first argument to ThrowValueError the argument position? Shouldn't it be 1 in the 'fun' mismatch case? Does (typeof fun) !== 'function' get fully optimized by the jits? If not, we might want to add an intrinsic for that at some point. @@ +8,5 @@ > + var array = Object(this); > + var len = array.length; > + var receiver = arguments.length > 1 ? arguments[1] : null; > + for (var i = 0; i < len; i++) { > + if (i in array) I'm also curious about the performance of this bit. Would it help to have a special case for dense arrays? @@ +9,5 @@ > + var len = array.length; > + var receiver = arguments.length > 1 ? arguments[1] : null; > + for (var i = 0; i < len; i++) { > + if (i in array) > + %_CallFunction(receiver, array[i], i, array, fun); What's the _ for? @@ +17,5 @@ > +function ArrayStaticForEach(list, fun, receiver) { > + if (arguments.length < 2) > + %ThrowMissingArgError(1); > + %_CallFunction(list, fun, receiver, ArrayForEach); > +} \ No newline at end of file Last thing - do we have an established style for JS code in the JS engine? If not, I like the idea of 4 spaces to match Python and our C++ code.

(In reply to David Mandelin [:dmandelin] from comment #85) > Comment on attachment 648397 [details] [diff] [review] > Embed self-hosted JS and install it into standard classes, v6 > > Review of attachment 648397 [details] [diff] [review]: > ----------------------------------------------------------------- > > This seems to be going really well. I looked closely at the array.js file, > to see what's it like coding with the new feature. I have a few > questions/comments below. > > ::: js/src/builtin/array.js > @@ +1,1 @@ > > +function ArrayForEach(fun/*, receiver*/) { > > I suppose we don't have jit support for rest arguments yet, but we should do > some followup work to get that going and change this function to use 'em. The |receiver| argument is commented out because |forEach| is specified as only having one argument. I don't think rest would gain us anything here. > > @@ +3,5 @@ > > + %ThrowNoConstructorError(); > > + if (arguments.length == 0) > > + %ThrowMissingArgError(0); > > + if (this == null || (typeof fun) !== 'function') > > + %ThrowValueError(0, 22); > > Definitely want to do some followup to replace '22' with something better. > It seems like if we had that we could also avoid (or embrace, depending on > how it's done!) having one intrinsic for each kind of error to report. I have started implementing a better approach already: Using a Python script, the new approach enables direct usage of the enum entry's symbol as a string. I.e., |22| becomes "JSMSG_NOT_FUNCTION". And I agree about not letting different error factory functions proliferate. > > Is the first argument to ThrowValueError the argument position? Shouldn't it > be 1 in the 'fun' mismatch case? Good point: That way I can also throw errors regarding the builtin function itself using the same function. > > Does (typeof fun) !== 'function' get fully optimized by the jits? If not, we > might want to add an intrinsic for that at some point. I added an intrinsic because this test doesn't entirely cover the test as specified. > > @@ +8,5 @@ > > + var array = Object(this); > > + var len = array.length; > > + var receiver = arguments.length > 1 ? arguments[1] : null; > > + for (var i = 0; i < len; i++) { > > + if (i in array) > > I'm also curious about the performance of this bit. Would it help to have a > special case for dense arrays? > > @@ +9,5 @@ > > + var len = array.length; > > + var receiver = arguments.length > 1 ? arguments[1] : null; > > + for (var i = 0; i < len; i++) { > > + if (i in array) > > + %_CallFunction(receiver, array[i], i, array, fun); > > What's the _ for? This way, the name matches what V8 uses, making it easier to reuse their code. > > @@ +17,5 @@ > > +function ArrayStaticForEach(list, fun, receiver) { > > + if (arguments.length < 2) > > + %ThrowMissingArgError(1); > > + %_CallFunction(list, fun, receiver, ArrayForEach); > > +} > \ No newline at end of file > > Last thing - do we have an established style for JS code in the JS engine? > If not, I like the idea of 4 spaces to match Python and our C++ code. Yes, I changed that, too. The original indenting was an artifact of using V8 code. Also, the new code will contain comments about the different spec steps implemented in each part of the code. I'm still debugging an issue with the new error reporting and will attach a new patch once that's done.

> I'm also curious about the performance of this bit. Would it help to have a > special case for dense arrays? I forgot this part: We might check that, yes. It gains another 10%, so maybe it's worth it.

(In reply to Till Schneidereit [:till] from comment #86) > (In reply to David Mandelin [:dmandelin] from comment #85) > > Comment on attachment 648397 [details] [diff] [review] > > Embed self-hosted JS and install it into standard classes, v6 > > > > Review of attachment 648397 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This seems to be going really well. I looked closely at the array.js file, > > to see what's it like coding with the new feature. I have a few > > questions/comments below. > > > > ::: js/src/builtin/array.js > > @@ +1,1 @@ > > > +function ArrayForEach(fun/*, receiver*/) { > > > > I suppose we don't have jit support for rest arguments yet, but we should do > > some followup work to get that going and change this function to use 'em. > > The |receiver| argument is commented out because |forEach| is specified as > only having one argument. I don't think rest would gain us anything here. I'm not sure I understand. ES5.1 gives a second argument, |thisArg|, and your implementation handles it using |arguments|. I was just saying that |arguments| is "bad", and that rest arguments are much nicer, but I think you are correct for now in using |arguments|, because our jits know how to make it fast, but not rest args. But we should switch to rest args someday. > > @@ +3,5 @@ > > > + %ThrowNoConstructorError(); > > > + if (arguments.length == 0) > > > + %ThrowMissingArgError(0); > > > + if (this == null || (typeof fun) !== 'function') > > > + %ThrowValueError(0, 22); > > > > Definitely want to do some followup to replace '22' with something better. > > It seems like if we had that we could also avoid (or embrace, depending on > > how it's done!) having one intrinsic for each kind of error to report. > > I have started implementing a better approach already: Using a Python > script, the new approach enables direct usage of the enum entry's symbol as > a string. I.e., |22| becomes "JSMSG_NOT_FUNCTION". Sounds great. > > @@ +9,5 @@ > > > + var len = array.length; > > > + var receiver = arguments.length > 1 ? arguments[1] : null; > > > + for (var i = 0; i < len; i++) { > > > + if (i in array) > > > + %_CallFunction(receiver, array[i], i, array, fun); > > > > What's the _ for? > > This way, the name matches what V8 uses, making it easier to reuse their > code. Ah, good idea. > Also, the new code will contain comments about the different spec steps > implemented in each part of the code. Cool. > > I'm also curious about the performance of this bit. Would it help to have a > > special case for dense arrays? > > I forgot this part: We might check that, yes. It gains another 10%, so maybe > it's worth it. I'd say that's worth it. It definitely shouldn't block your present work, though.

> I'm not sure I understand. ES5.1 gives a second argument, |thisArg|, and > your implementation handles it using |arguments|. I was just saying that > |arguments| is "bad", and that rest arguments are much nicer, but I think > you are correct for now in using |arguments|, because our jits know how to > make it fast, but not rest args. But we should switch to rest args someday. Right, I forgot that ...rest is spec'd as not being counted for determining Function#length. I agree: we should investigate the performance of changing to that. > > > I'm also curious about the performance of this bit. Would it help to have a > > > special case for dense arrays? > > > > I forgot this part: We might check that, yes. It gains another 10%, so maybe > > it's worth it. > > I'd say that's worth it. It definitely shouldn't block your present work, > though. Thinking about this some more, I don't think we can special-case this in a satisfactory way, after all. Given that we execute a function that can freely modify the array we're operating on, we'd have to check whether the array is still dense after each step. If that's cheaper than |i in array|, then we should do it, sure.

This version changes various aspects of the self-hosting mechanism: @ted: I've moved the scripts that are invoked during make into the "config" directory. Seems to me that that location makes more sense. @gerv: I've changed about:license to add info about files in "js/src/config" being under the V8 license and refreshed the time range of the license. I hope these changes are ok. @bhackett: This version contains a complete, by-the-spec self-hosted implementation of Array.prototype.forEach with proper comments and all code as I think it should be. Of note, errors can now be thrown with just one function, ThrowError, and can be specified using their enum keys from js.msg. To get proper decompilation of values (e.g. for JSMSG_NOT_FUNCTION), I had to change js_DecompileValueGenerator to add the ability to decompile the expression used to create the value when invoking the self-hosted function, not ThrowError.

Argh, forgot to add "config/embedjs.py" to hg.

(In reply to Till Schneidereit [:till] from comment #89) > > > > I'm also curious about the performance of this bit. Would it help to have a > > > > special case for dense arrays? > > > > > > I forgot this part: We might check that, yes. It gains another 10%, so maybe > > > it's worth it. > > > > I'd say that's worth it. It definitely shouldn't block your present work, > > though. > > Thinking about this some more, I don't think we can special-case this in a > satisfactory way, after all. Given that we execute a function that can > freely modify the array we're operating on, we'd have to check whether the > array is still dense after each step. If that's cheaper than |i in array|, > then we should do it, sure. Good point. I checked the current C++ implementation and it doesn't special-case dense arrays (which you probably already saw), probably for that exact reason. And now that you made me think about it more carefully, I think we can teach the jits to hoist |i in array|.

> Good point. I checked the current C++ implementation and it doesn't > special-case dense arrays (which you probably already saw), probably for > that exact reason. And now that you made me think about it more carefully, I > think we can teach the jits to hoist |i in array|. I filed bug 780786 and bug 780787 for that.

Comment on attachment 649118 [details] [diff] [review] Embed self-hosted JS and install it into standard classes, v8 Review of attachment 649118 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +1180,5 @@ > + * JSOP_GETGNAME. This causes the lookup to be redirected to the > + * special intrinsics holder in the global object, into which any > + * missing objects are cloned lazily upon first access. > + */ > + case JSOP_NAME: *op = bce->selfHostingMode ? JSOP_INTRINSICNAME : JSOP_GETGNAME; break; It would be good if we could assert that no plain JSOP_NAME is emitted when in self hosting mode, as these lookups will be directed to the normal global and is definitely not what the self hosted code wants to do. @@ +1185,5 @@ > case JSOP_SETNAME: *op = JSOP_SETGNAME; break; > case JSOP_INCNAME: *op = JSOP_INCGNAME; break; > case JSOP_NAMEINC: *op = JSOP_GNAMEINC; break; > case JSOP_DECNAME: *op = JSOP_DECGNAME; break; > case JSOP_NAMEDEC: *op = JSOP_GNAMEDEC; break; Similarly, can you assert that none of the modification cases below is hit when in self hosting mode? ::: js/src/frontend/BytecodeEmitter.h @@ +119,5 @@ > > const bool hasGlobalScope:1; /* frontend::CompileScript's scope chain is the > global object */ > > + const bool selfHostingMode:1; /* emit special opcodes for global lookups */ Comment is vague, maybe point to Parser::selfHostingMode. ::: js/src/frontend/Parser.h @@ +55,5 @@ > const bool compileAndGo:1; > > /* > * Self-hosted scripts can use the special syntax %funName(..args) to call > + * internal functions and emit special opcodes for global lookups. Can you expand on this comment and describe the effect of the intrinsicname opcodes, or direct to another comment which does? ::: js/src/jsapi.cpp @@ +934,5 @@ > delete_(debugScopes); > > + JS_RemoveObjectRootRT(this, selfHostedGlobal_.address()); > + > +// gcRootsHash.remove((void *)selfHostedGlobal_.address()); Stray commented line.

(In reply to Brian Hackett (:bhackett) from comment #94) > Comment on attachment 649118 [details] [diff] [review] > Embed self-hosted JS and install it into standard classes, v8 > > Review of attachment 649118 [details] [diff] [review]: Thanks for the review! The new version addresses the comments. Additionally, it changes enough things that I think a re-review is in order. Most importantly, a sanity check for the changes to js_DecompileValueGenerator and jsop_this would be very much welcome. I introduced the latter because not wrapping |this| is required for correctly implementing ToObject without using "use strict", which unfortunately makes things quite a lot slower. Also, I still haven't figured out how to prevent the test failure mentioned in comment 81: jit-test/tests/jaeger/bug738525.js crashes with Assertion failure: allocated(), at ../gc/Heap.h:506 during shutdown of the shell. I reduced the problem to simply running the script "it.customNative = 1". Finally, I have removed any actual self-hosted code and will add that as a follow-up patch (to be attached in a minute).

Comment on attachment 650251 [details] [diff] [review] Embed self-hosted JS and install it into standard classes, v9 I can't help much with the js_DecompileValueGenerator change, that code is a hack anyways so the change is fine so long as it does the right thing in the cases you've tested and will fail safe when the value isn't found. The jsop_this change is fine. The allocated() failure means that corrupt memory is being accessed somewhere.

Comment on attachment 649118 [details] [diff] [review] Embed self-hosted JS and install it into standard classes, v8 Review of attachment 649118 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, my review was slow going here, so I'll post my half-finished review of this patch. ::: js/src/Makefile.in @@ +848,5 @@ > +# Prepage self-hosted JS code for embedding > +export:: selfhosted.out.h > + > +SELFHOSTING_SRCS = \ > + $(srcdir)/builtin/array.js Current Makefile style is two spaces at the beginning of lines after a continuation, and no tabs anywhere they're not required for Makefile syntax. Also, you have a trailing space at the end of this line. If you think this list will grow in the future, you can stick a continuation on this line and use $(NULL) as the last line, so that adding new entries is easy. Finally, we're trying to work on our Makefile style, and one of our (as-yet-unwritten) new conventions is to use lowercase for Makefile variables that are file-local. (Compared to uppercase for variables that have special meaning to the build system.) @@ +855,5 @@ > + $(SELFHOSTING_SRCS) \ > + $(srcdir)/js.msg \ > + $(srcdir)/config/macros.py \ > + $(srcdir)/config/js2c.py \ > + $(srcdir)/config/embedjs.py Similarly for the indentation here. If the indentation bothers you, you can assign the deps to a variable and use that instead, like: selfhosted_out_h_deps := \ $(SELFHOSTING_SRCS) \ ... selfhosted.out.h: $(selfhosted_out_h_deps) @@ +856,5 @@ > + $(srcdir)/js.msg \ > + $(srcdir)/config/macros.py \ > + $(srcdir)/config/js2c.py \ > + $(srcdir)/config/embedjs.py > + $(PYTHON) $(srcdir)/config/embedjs.py selfhosted.out.h $(srcdir)/js.msg \ You should use $@ instead of repeating the output filename here. ::: js/src/config/embedjs.py @@ +1,3 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. I'm not sure that I agree that putting these in config/ makes the most sense. I would just put them in js/src/builtin next to the input code they're using. (We tend to only put generic build-system stuff in config/.) @@ +5,5 @@ > +# This utility converts JS files containing self-hosted builtins into a C > +# header file that can be embedded into SpiderMonkey. > +# > +# It expects error messages in the JS code to be referenced by their C enum > +# keys as literals. You've got a weird mix of indentation in this file. You have both tabs and spaces, for one. You should follow PEP-8 style and use four-space indent everywhere. You also have a couple of trailing spaces in the comments here. @@ +11,5 @@ > +import json, re, sys, os, js2c, fileinput > + > +def replaceErrorMsgs(source_files, messages_file, output_file): > + messages = buildMessagesTable(messages_file) > + with open(output_file, 'w') as output: You'll need to stick "from __future__ import with_statement" before your other imports for this to work right, because we still support Python 2.5 for now. @@ +20,5 @@ > + table = {} > + for line in fileinput.input(messages_file): > + if not line.startswith('MSG_DEF('): > + continue > + match = re.match(r"MSG_DEF\(([\w_]+),\s*(\d+)", line) Seems a little redundant to have the startswith check and then the re.match. You could just run the re.match and do: if not match: continue Also, if you're going to run this regex multiple times you might want to use re.compile and save the compiled regex.

And now that I have internet again, here's the previously-mentioned patch that's actually using all this stuff for something.

This version addresses all feedback and fixes the regression mentioned in comment 95. @ted: Thanks for the in-depth review, much appreciated. I've addressed your comments so this should be ready to land if you don't have additional issues.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=95837ce42e9b

Try run for abd7c89b1a9f is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=abd7c89b1a9f Results (out of 229 total builds): exception: 2 success: 186 warnings: 26 failure: 15 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tschneidereit@gmail.com-abd7c89b1a9f

I'll debug the linux opt build failures on Monday: for some reason the xpcshell crashes while during precompilation. The other failures mostly look like they're infrastructure problems, but I guess the next try run will tell.

Comment on attachment 650699 [details] [diff] [review] Embed self-hosted JS and install it into standard classes, v10 Review of attachment 650699 [details] [diff] [review]: ----------------------------------------------------------------- r=me with a few fixes. ::: js/src/Makefile.in @@ +823,5 @@ > ifdef HAVE_LINUX_PERF_EVENT_H > pm_linux.$(OBJ_SUFFIX): CXXFLAGS += $(LINUX_HEADERS_INCLUDES) > endif > > +# Prepage self-hosted JS code for embedding "Prepare", presumably? @@ +827,5 @@ > +# Prepage self-hosted JS code for embedding > +export:: selfhosted.out.h > + > +selfhosting_srcs = \ > + $(NULL) I think you're missing something here... (also, you should always use := for assignment unless you know you need otherwise). ::: js/src/builtin/embedjs.py @@ +4,5 @@ > + > +# This utility converts JS files containing self-hosted builtins into a C > +# header file that can be embedded into SpiderMonkey. > +# > +# It expects error messages in the JS code to be referenced by their C enum A few lines here have trailing whitespace. @@ +7,5 @@ > +# > +# It expects error messages in the JS code to be referenced by their C enum > +# keys as literals. > + > +import json, re, sys, os, js2c, fileinput You need to: from __future__ import with_statement for Python 2.5 compat here. @@ +10,5 @@ > + > +import json, re, sys, os, js2c, fileinput > + > +def replaceErrorMsgs(source_files, messages_file, output_file): > + messages = buildMessagesTable(messages_file) Python that you're writing (and not just importing wholesale from a third-party) should use 4-space indentation, not tabs. @@ +41,5 @@ > + source_files = sys.argv[4:] > + combined_file = 'combined.js' > + replaceErrorMsgs(source_files, messages_file, combined_file) > + js2c.JS2C([combined_file, macros_file], [output_file], { 'TYPE': 'CORE', 'COMPRESSION': 'off' }) > + os.remove(combined_file) I mentioned the inconsistent indentation last time, please fix it. 4-space indentation everywhere.

Fixes exception in xpcshell and addresses feedback. Apparently, compiling an empty string isn't advisable. Why that causes a Linux-only abort in `make package` (specifically while the xpcshell is running precompile_cache.js and creating a jscontext for that) is beyond me, but at least adding some placeholder code to this base patch instead of any actual self-hosted code fixes it. @ted: sorry for the unaddressed feedback in the last patch. I seem to have applied some of the fixes to the wrong patch in my queue. Pushed to try here: https://tbpl.mozilla.org/?tree=Try&rev=fb992c734c77 and ready to go if that turns out green.

till, I finally managed to upload the content of dist/bin to http://people.mozilla.org/~raliiev/bin.tar.bz2

Comment on attachment 650571 [details] [diff] [review] Convert some array extras to self-hosted JS code, wip Review of attachment 650571 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/array.js @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/* ES5 15.4.4.16. */ > +function ArrayEvery(fun/*, receiver*/) { Using the same variable names as in the ES5 spec would make comparison of the algorithms a bit easier (e.g., fun <-> callbackfn). @@ +10,5 @@ > + /* Step 2-3. */ > + var len = TO_UINT32(array.length); > + > + /* Step 4. */ > + if (arguments.length == 0) The comparison operator == should be avoided unless you really need its type coercion. Use === instead. JavaScript style guides seem to agree that if/while/for/try statements should always use braces: http://javascript.crockford.com/code.html#compound%20statements http://docs.jquery.com/JQuery_Core_Style_Guidelines#Blocks @@ +11,5 @@ > + var len = TO_UINT32(array.length); > + > + /* Step 4. */ > + if (arguments.length == 0) > + %ThrowError(JSMSG_MISSING_FUN_ARG, 0, 'Array.prototype.every'); Would it be possible to use a function name that indicates the kind of error to be thrown (here TypeError)? @@ +16,5 @@ > + if (!IsCallable(fun)) > + %ThrowError(JSMSG_NOT_FUNCTION, fun); > + > + /* Step 5. */ > + var receiver = arguments.length > 1 ? arguments[1] : void 0; Is "void 0" the recommended way of safely writing "undefined"? I was thinking of just defining my own undefined variable... @@ +23,5 @@ > + /* Step a (implicit), and d. */ > + for (var i = 0; i < len; i++) { > + /* Step b */ > + if (i in array) { > + /* Step c.i-ii. */ Actually all of step c. @@ +24,5 @@ > + for (var i = 0; i < len; i++) { > + /* Step b */ > + if (i in array) { > + /* Step c.i-ii. */ > + if (!!%_CallFunction(receiver, array[i], i, array, fun) !== true) I'd write the condition either as "%ToBoolean(%_CallFunction(...)) === false" to stay close to the spec, or simply as "!%_CallFunction(...)". @@ +33,5 @@ > + /* Step 8. */ > + return true; > +} > + > +function ArrayStaticEvery(list, fun/*, receiver */) { Are the static functions just for testing? They're not in ES5... @@ +41,5 @@ > + return %_CallFunction(list, fun, receiver, ArrayEvery); > +} > + > +/* ES5 15.4.4.17. */ > +function ArraySome(fun/*, receiver*/) { Most code is duplicated between every, some, and forEach. Would it make sense to have them call a common sub-function, passing in the callback result value that should cause early exit and the value that should be returned at completion? @@ +140,5 @@ > + > + /* Step 7-8. */ > + /* Step a (implicit), and d. */ > + for (var i = 0; i < len; i++) { > + print(i, i in array); rm @@ +145,5 @@ > + /* Step b */ > + if (i in array) { > + /* Step c.i-iii. */ > + var mappedValue = %_CallFunction(receiver, array[i], i, array, fun); > + Object.defineProperty(i, {value:mappedValue, writable:true, enumerable:true, configurable:true}); Missing "A" as first argument.

This version finally passes a full try run on all platforms and is rebased to current m-c. Carrying the r+es as the changes are fairly small.

Comment on attachment 652560 [details] [diff] [review] Embed self-hosted JS and install it into standard classes, v12 Green on Try. https://tbpl.mozilla.org/?tree=Try&rev=64b46b642890 https://hg.mozilla.org/integration/mozilla-inbound/rev/b51c44f468d5

Comment on attachment 650571 [details] [diff] [review] Convert some array extras to self-hosted JS code, wip Review of attachment 650571 [details] [diff] [review]: ----------------------------------------------------------------- I found one conformance issue after applying this patch on top of mozilla-central rev d9923f7b8930 (which includes the other two patches): The self-hosted Array methods have prototype properties, which built-in non-constructor functions according to ES5 section 15 are not allowed to have: "None of the built-in functions described in this clause shall have a prototype property unless otherwise specified in the description of a particular function." ::: js/src/jsarray.cpp @@ +3423,5 @@ > JS_FN("slice", array_slice, 2,JSFUN_GENERIC_NATIVE), > > JS_FN("indexOf", array_indexOf, 1,JSFUN_GENERIC_NATIVE), > JS_FN("lastIndexOf", array_lastIndexOf, 1,JSFUN_GENERIC_NATIVE), > + {"forEach", NULL, 1,JSFUN_INTERPRETED, "ArrayForEach"}, Recent changes require the use of JSOP_NULLWRAPPER instead of NULL. @@ +3437,5 @@ > }; > > static JSFunctionSpec array_static_methods[] = { > JS_FN("isArray", array_isArray, 1,0), > + {"forEach", NULL, 1,JSFUN_INTERPRETED, "ArrayStaticForEach"}, Are these functions for testing? They're not part of ES5.

I tried to use self-hosted functions to implement the ECMAScript Internationalization API, but found that a number of constructs don't work yet: var locale = 'de'; // global variable → Assertion failure: *op == JSOP_NAME, at /Users/standards/Mozilla/self-hosted/js/src/frontend/BytecodeEmitter.cpp:1170 function Func1() { var alpha = "[a-zA-Z]", digit = "[0-9]", alphanum = "(" + alpha + "|" + digit + ")", singleton = "(" + digit + "|[A-WY-Za-wy-z])", duplicateSingleton = "-" + singleton + "-(.*-)?\\1(?!" + alphanum + ")"; // create an instance of a built-in constructor var duplicateSingletonRE = new RegExp(duplicateSingleton, "i"); } → TypeError: undefined is not a constructor function Func2() { return {}; // create an object to return } → Assertion failure: JS_ObjectIsFunction(__null, this), at /Users/standards/Mozilla/self-hosted/js/src/jsfun.h:208 function Func3() { // create an array as a separate step so that I can add elements // before returning it result = []; return result; } → Assertion failure: *op == JSOP_NAME, at /Users/standards/Mozilla/self-hosted/js/src/frontend/BytecodeEmitter.cpp:1180

Procedural question: This bug originally requested self-hosting support for "Array.prototype.join and other JS builtins"; it probably wasn't meant to provide support for complete modules such as the ECMAScript Internationalization API. Should we continue to use this bug for everything that's necessary for complete self-hosted modules, or keep this bug to something close to its original scope and file additional bugs for the rest? In any case, Till has already accomplished a lot, and at some point we should acknowledge that, whether through letting him close the ticket or otherwise...

Comment on attachment 650571 [details] [diff] [review] Convert some array extras to self-hosted JS code, wip Review of attachment 650571 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/Makefile.in @@ +848,5 @@ > # Prepage self-hosted JS code for embedding > export:: selfhosted.out.h > > +SELFHOSTING_SRCS = \ > + $(srcdir)/builtin/array.js SELFHOSTING_SRCS := \ $(srcdir)/builtin/array.js \ $(NULL) please. (Note two spaces instead of tab, and no trailing whitespace.)

And we should close this bug as soon as any builtin is running self-hosted, IMO.

(In reply to Norbert Lindenberg from comment #112) > Procedural question: This bug originally requested self-hosting support for > "Array.prototype.join and other JS builtins"; it probably wasn't meant to > provide support for complete modules such as the ECMAScript > Internationalization API. Should we continue to use this bug for everything > that's necessary for complete self-hosted modules, or keep this bug to > something close to its original scope and file additional bugs for the rest? > > In any case, Till has already accomplished a lot, and at some point we > should acknowledge that, whether through letting him close the ticket or > otherwise... Thanks :) I fully agree about closing this bug sooner rather than later. Thus, I'm doing so now, changing the title and a few details to reflect what actually happened here. I've created bug 784288 for tracking additional implementation work and follow-up fixes. Please file bugs blocking that one for additional tasks.