We use js_InitClass to initialize almost every standard class, which confines us to exactly that function's behavior. The standard classes have too much complexity and uniqueness to fit its confines, at least not without seriously distorting them: * Some classes (Math) have only "constructors", and for those we pass NULL as the constructor. This short-circuits constructor creation and headfakes the tail of js_InitClass into not assuming it has a constructor. * RangeError, etc. inherit from Error.prototype, not Object.prototype, so we copied almost all of js_InitClass into a second method, then made both js_InitClass and js_InitExceptionClasses call that method. * RangeError, etc. need special handling to add the handful of data properties on the Error prototypes because js_InitClass can't add actual data properties. * QName.prototype and Namespace.prototype are actual XML objects and not regular old objects, so we added JSCLASS_CONSTRUCT_PROTOTYPE to create a regular old object, then create the XML constructor, then *call the constructor* to create the real prototype object. * We decided StopIteration should be frozen, so we added JSCLASS_FREEZE_PROTO and JSCLASS_FREEZE_CTOR (but J_C_C is never used!) to freeze as quickly after initialization as possible. * Object and Function have to be created before anything else. There's an opaque recursive dance between js_InitObjectClass and js_InitFunctionClass to create Object.prototype, Object, Function.prototype, and Function in just the right sequence that we get everything set up. I have no idea how it bottoms out without error, with these self-consistent. * We want to brand all these built-ins that have function properties, we brand the created prototype (if it had methods) and the created method (if it had methods). But we have to be careful of the first hack (for prototype-less classes) and check that ctor != proto before maybe branding the ctor! * Because of js_InitClass's constrictions Array.prototype is created as a dense array, then on first property definition to a slow array, rather than being slow from the start. (bug 592296) * js_InitClass has special munging to have Function to have Function.prototype as its prototype. (How does creating Function work, if we don't have Function.prototype yet? I think but am not certain that it picks up an *older* Function.prototype -- because we init the Function class multiple times during this bootstrapping -- and then gets later overwritten with the actual, used Function.prototype before the whole thing returns.) * All the built-in stuff should be safe against deletion and enumeration re-resolving the builtins (it's not, but I digress), so the built-ins all save their constructor *twice* in the global object and the prototype once in the global object. (This is so []/{} get the original array/object prototypes.) Public JSClass flags expose this mechanism, but no external user should ever use them. I hope I have demonstrated that this is a house of cards. The built-in classes should implement their own bootstrapping. They shouldn't use js_InitClass/JS_InitClass, nor should they use publicly-exposed JSClass flags for tricky special-case behavior. It's only 50-75 lines of code and comments for most classes that js_InitClass handled. External code can still use JS_InitClass if it wants; they're straightforward. (Although we should remove most of these gazillion special cases once the standard stuff is specialized -- followups.) With these changes GlobalObject::getArrayPrototype and such become feasible. I also think it'll soon be possible to remove storage for the original values of the built-in constructors. Right now there's an incredible mess of code which does access them, but I think it comes to nothing if you squint hard enough. We have js::FindProto, js::FindClassObject, js_GetClassObject, js_GetClassPrototype, and I think a couple other methods that all do kind-of-almost-not-quite the same thing, and with their names all being sort of similar it's an undistinguishable mess, impossible to comprehend all at once. We can also stop shoehorning things like JSON and Math into this system with a little care. And an eventual holy grail: give global objects their standard classes and everything right from the start, no worries about is-this-inited, getArrayPrototype and such would be infallible after bootstrapping. But one step at a time: initialize the standard classes specially. Monster patch series to follow, more or less one patch per standard class de-generification. I suggest that the same person review all of these, because there's a certain ramp-up cost to understanding the first inlining of js_InitClass for one class, then you can build upon that for most of the rest of the classes. The final patch is trickiest and was not amenable to rewriting by inlining (due to the js_GetClassPrototype snarl and multiple-initialization), so it needs particular care in review. The whole series passes try on linux64 debug. I'll also post a bunde once everything's uploaded, as there are a couple patches underneath this series in my queue.

One step at a time seemed more prudent than making a method *and* rewriting it all at once.

All the typed array classes need it, and an atomstate atom is cheap.

More minor changes kept separate just because it might help.

...the idea being js_InitObject, js_InitFunction, and js_InitFAOClasses can all be next to each other to streamline unification.

...same reason.

...and here's where it's all headed. (At least as far as this bug's concerned.)

Impressive

Comment on attachment 530498 [details] [diff] [review] Initialize StopIteration specially >diff --git a/js/src/jsiter.cpp b/js/src/jsiter.cpp >--- a/js/src/jsiter.cpp >+++ b/js/src/jsiter.cpp >@@ -1557,11 +1557,43 @@ InitGeneratorClass(JSContext *cx, Global > static JSObject * > InitStopIterationClass(JSContext *cx, GlobalObject *global) >+ return false; Nuh-uh

One other thought that's occurred to me is that I should write an MDC article about how to add a new standard class after this change. I expect that'll help with regularizing the code for all of these, and it'll help if and when new standard classes get added in the future, as far as not having me be the bottleneck on the exact setup process or anything like that.

I think this is the wrong approach. There is already copy-paste programming going on, copying from js_InitClass to js_InitStringClass and js_InitRegExpClass, and not in trivial degree or kind (search for "The risk is particularly great"). We do not need more; we should have less. I understand sometimes it helps to inline-expand by hand, specialize, and then re-synthesize for minimal, almost-DRY code. I've seen njn do that particularly well. It's also true that parts of SpiderMonkey were commoned prematurely or over-aggressively, and all old code is suspect. There certainly are places to refactor, re-layer, and remove layers, e.g., to avoid useless tests on common paths. But that's not what is happening here, as far as I can tell. "Bootstrapping is special" is not a meaningful mantra and it does not justify so much duplication or so big a patch. And this bug should not hold up shorter term fixes (ES5 bugs such as bug 637994. Meanwhile, bug 593462 is waiting to be fixed, without needing code duplication at all, and with the win of removing lazy standard class initialization, by making JS_NewGlobalObject blindingly fast: memcpy plus pointer-fixup based on exemplars in a root compartment. Who will take that bug? /be

From a quick scan, there certainly is an incredible degree of duplication. But also there is an incredible number of special cases enumerated in comment 0 that I think its valuable to have explicated. Can we have both? I think so. The metaphor that is forcing itself into my head is this: Its a difficult thing to convert directly from one compressed audio format to another, so what we do is, say, go aac -> wav and then do wav -> mp3. I think this patch queue is like aac -> wav: it makes plain all the special cases and how they tie to the related language semantics, which is good but, just like a wav, its kinda large. I think, to land this, we would want to do the next step, wav -> mp3 and try to factor out commonality. This is what I think Brendan is getting at in comment 32 para 2. More plainly, I would try to iteratively factor out commonality between all these js_Init* functions in a way that preserves the explication of special cases by making different helpers or parameterizing them explicitly. I'm still saying this without intimate knowledge of the js_InitClass and argument-by-metaphor is fraud, but thats my 2 cents.

$ grep -B 15 -A 15 "The risk is particularly great" *.cpp jsobj.cpp- goto bad; jsobj.cpp- } jsobj.cpp- jsobj.cpp- /* jsobj.cpp- * Pre-brand the prototype and constructor if they have built-in methods. jsobj.cpp- * This avoids extra shape guard branch exits in the tracejitted code. jsobj.cpp- */ jsobj.cpp- if (fs) jsobj.cpp- proto->brand(cx); jsobj.cpp- if (ctor != proto && static_fs) jsobj.cpp- ctor->brand(cx); jsobj.cpp- jsobj.cpp- /* jsobj.cpp- * Make sure proto's emptyShape is available to be shared by objects of jsobj.cpp- * this class. JSObject::emptyShape is a one-slot cache. If we omit this, jsobj.cpp: * some other class could snap it up. (The risk is particularly great for jsobj.cpp- * Object.prototype.) jsobj.cpp- * jsobj.cpp- * All callers of JSObject::initSharingEmptyShape depend on this. jsobj.cpp- * jsobj.cpp- * FIXME: bug 592296 -- js_InitArrayClass should pass &js_SlowArrayClass jsobj.cpp- * and make the Array.prototype slow from the start. jsobj.cpp- */ jsobj.cpp- JS_ASSERT_IF(proto->clasp != clasp, jsobj.cpp- clasp == &js_ArrayClass && proto->clasp == &js_SlowArrayClass); jsobj.cpp- if (!proto->getEmptyShape(cx, proto->clasp, FINALIZE_OBJECT0)) jsobj.cpp- goto bad; jsobj.cpp- jsobj.cpp- if (clasp->flags & (JSCLASS_FREEZE_PROTO|JSCLASS_FREEZE_CTOR)) { jsobj.cpp- JS_ASSERT_IF(ctor == proto, !(clasp->flags & JSCLASS_FREEZE_CTOR)); jsobj.cpp- if (proto && (clasp->flags & JSCLASS_FREEZE_PROTO) && !proto->freeze(cx)) -- jsregexp.cpp- jsregexp.cpp- /* Add static properties to the RegExp constructor. */ jsregexp.cpp- if (!JS_DefineProperties(cx, ctor, regexp_static_props) || jsregexp.cpp- !JS_AliasProperty(cx, ctor, "input", "$_") || jsregexp.cpp- !JS_AliasProperty(cx, ctor, "multiline", "$*") || jsregexp.cpp- !JS_AliasProperty(cx, ctor, "lastMatch", "$&") || jsregexp.cpp- !JS_AliasProperty(cx, ctor, "lastParen", "$+") || jsregexp.cpp- !JS_AliasProperty(cx, ctor, "leftContext", "$`") || jsregexp.cpp- !JS_AliasProperty(cx, ctor, "rightContext", "$'")) { jsregexp.cpp- return NULL; jsregexp.cpp- } jsregexp.cpp- jsregexp.cpp- /* jsregexp.cpp- * Make sure proto's emptyShape is available to be shared by objects of jsregexp.cpp- * this class. JSObject::emptyShape is a one-slot cache. If we omit this, jsregexp.cpp: * some other class could snap it up. (The risk is particularly great for jsregexp.cpp- * Object.prototype.) jsregexp.cpp- * jsregexp.cpp- * All callers of JSObject::initSharingEmptyShape depend on this. jsregexp.cpp- */ jsregexp.cpp- if (!proto->getEmptyShape(cx, &js_RegExpClass, FINALIZE_OBJECT0)) jsregexp.cpp- return NULL; jsregexp.cpp- jsregexp.cpp- /* Install the fully-constructed RegExp and RegExp.prototype in global. */ jsregexp.cpp- if (!DefineConstructorAndPrototype(cx, global, JSProto_RegExp, ctor, proto)) jsregexp.cpp- return NULL; jsregexp.cpp- jsregexp.cpp- return proto; jsregexp.cpp-} jsregexp.cpp- -- jsstr.cpp- jsstr.cpp- /* Add properties and methods to the prototype and the constructor. */ jsstr.cpp- if (!JS_DefineFunctions(cx, proto, string_methods) || jsstr.cpp- !JS_DefineFunctions(cx, ctor, string_static_methods)) jsstr.cpp- { jsstr.cpp- return NULL; jsstr.cpp- } jsstr.cpp- jsstr.cpp- /* Pre-brand String and String.prototype for trace-jitted code. */ jsstr.cpp- proto->brand(cx); jsstr.cpp- ctor->brand(cx); jsstr.cpp- jsstr.cpp- /* jsstr.cpp- * Make sure proto's emptyShape is available to be shared by String jsstr.cpp- * objects. JSObject::emptyShape is a one-slot cache. If we omit this, some jsstr.cpp: * other class could snap it up. (The risk is particularly great for jsstr.cpp- * Object.prototype.) jsstr.cpp- * jsstr.cpp- * All callers of JSObject::initSharingEmptyShape depend on this. jsstr.cpp- */ jsstr.cpp- if (!proto->getEmptyShape(cx, &js_StringClass, FINALIZE_OBJECT0)) jsstr.cpp- return NULL; jsstr.cpp- jsstr.cpp- /* Install the fully-constructed String and String.prototype. */ jsstr.cpp- if (!DefineConstructorAndPrototype(cx, global, JSProto_String, ctor, proto)) jsstr.cpp- return NULL; jsstr.cpp- jsstr.cpp- return proto; jsstr.cpp-} jsstr.cpp- jsstr.cpp-JSFixedString * That's without this bug's patch. The branding code and its order with respect to prototype property additions is non-trivial enough to want in one place, not three or more places. /be

(In reply to comment #32) > The branding code and its order with respect to prototype property additions > is non-trivial enough to want in one place, not three or more places. Could you elaborate? My understanding is that branding just changes shape. That might deoptimize by preventing cache hits, or staying on trace, or what-have-you. But I hadn't thought it had any ordering constraints with respect to property additions, except that you want to do it after adding all properties to the prototype to best avail yourself of the branding advantage.

(In reply to comment #33) > (In reply to comment #32) > > The branding code and its order with respect to prototype property additions > > is non-trivial enough to want in one place, not three or more places. > > Could you elaborate? My understanding is that branding just changes shape. > That might deoptimize by preventing cache hits, or staying on trace, or > what-have-you. But I hadn't thought it had any ordering constraints with > respect to property additions, except that you want to do it after adding > all properties to the prototype to best avail yourself of the branding > advantage. That's what the comment says, and it's what I meant: /* * Pre-brand the prototype and constructor if they have built-in methods. * This avoids extra shape guard branch exits in the tracejitted code. */ if (fs) proto->brand(cx); if (ctor != proto && static_fs) ctor->brand(cx); Again, this code, and the code before and after it, should not be duplicated. If there are several js_InitClass variants that can be used for commoning, that is great. Do that. Taking the car apart and duplicating the parts into ten cars is not great. This is not an ASIC where replicating circuits can win. The summary of this bug says js_InitClass is contorted around standard classes, but there is only one key != JSProto_Null tests in the whole function, two more in DefineConstructorAndPrototype. That is not exactly "contorted", but it does suggest factoring that out to callers that pass key != JSProto_Null, if the relative order can be managed. The big bootstrapping test is commented: /* Bootstrap Function.prototype (see also JS_InitStandardClasses). */ if (ctor->getClass() == clasp) ctor->setProto(proto); This is making sure Function's [[Prototype]] is correct. It might be possible to hoist out to js_InitFunctionClass, but given what follows: /* Add properties and methods to the prototype and the constructor. */ if ((ps && !JS_DefineProperties(cx, proto, ps)) || (fs && !JS_DefineFunctions(cx, proto, fs)) || (static_ps && !JS_DefineProperties(cx, ctor, static_ps)) || (static_fs && !JS_DefineFunctions(cx, ctor, static_fs))) { goto bad; } getting Function correctly prototyped earliest seemed better. Whatever we do, it's not a cause for duplicating code, just moving it. The rest of DefineConstructorAndPrototype is conditioned by non-null tests of constructor, clasp->flags tests, optional parameter null tests (ps, fs), and the named bool (someone used a reference for an out param, boo). The ps and fs tests are not worth splitting more variant functions out, by code size (cycles do not matter testing these -- evidence to the contrary necessary to "optimize" non-prematurely). As comment 31 said, the patch queue for this bug creates "an incredible degree of duplication". We shouldn't need to go around and around on this point. It is obvious and it is peer review you are hearing, loud and clear. Please act on it by re-synthesizing a minimal-ish layered tree of helper functions. /be

I don't believe I was insistent on maintaining complete duplication here. My intent (which I may not have expressed properly here or on IRC) was to get this up, then hopefully get reviewer suggestions for improvements, and we could iterate to something somewhat smaller. I've started that iteration, and it seems to go okay so far. I'll report back with more. (In reply to comment #34) > The summary of this bug says js_InitClass is contorted around standard > classes, but there is only one key != JSProto_Null tests in the whole > function, two more in DefineConstructorAndPrototype. That is not exactly > "contorted", but it does suggest factoring that out to callers that pass key > != JSProto_Null, if the relative order can be managed. I don't see key != JSProto_Null being the only way the special needs of standard classes impose upon the logic; I listed other ways in comment 0. > The big bootstrapping test is commented: > > /* Bootstrap Function.prototype (see also JS_InitStandardClasses). */ > if (ctor->getClass() == clasp) > ctor->setProto(proto); It is indeed commented to explain its immediate purpose. However, I can't say I understand (except facially) how it exactly integrates into the current ordering of js_InitFunctionClass, js_InitObjectClass, and js_InitFunctionAndObjectClasses, nor into how they call each other, nor into how these calls all bottom out. Some of that bottoming out occurs (if memory serves) in js_FindClassPrototype and js_GetClassPrototype, or maybe js::FindClassProto and one other js:: function. It's pretty spread out. And then there's the matter of that recursion actually creating multiple sets of {Function,Object}{,.prototype}, and installing them in the global multiple times > This is making sure Function's [[Prototype]] is correct. It might be > possible to hoist out to js_InitFunctionClass, but given what follows: > > /* Add properties and methods to the prototype and the constructor. */ > if ((ps && !JS_DefineProperties(cx, proto, ps)) || > (fs && !JS_DefineFunctions(cx, proto, fs)) || > (static_ps && !JS_DefineProperties(cx, ctor, static_ps)) || > (static_fs && !JS_DefineFunctions(cx, ctor, static_fs))) { > goto bad; > } > > getting Function correctly prototyped earliest seemed better. Whatever we > do, it's not a cause for duplicating code, just moving it. Whatever your feelings about the rest of the patches here (some of which I agree with, just haven't finished responding yet), could you please take a look at attachment 530519 [details] [diff] [review]? > As comment 31 said, the patch queue for this bug creates "an incredible > degree of duplication". We shouldn't need to go around and around on this > point. It is obvious and it is peer review you are hearing, loud and clear. > Please act on it by re-synthesizing a minimal-ish layered tree of helper > functions. I will. I made only one comment here, plus a few comments on IRC, discussing it. *Please* don't think I'm being antagonistic merely for antagonism's sake, or that I think what's here is what must be done in all exactness. If this comment were on a Get Satisfaction-styled site, my mood right now would be :-(. Or worse. I really don't want to engage in a long, drawn-out argument here, when changes will end up being made without needing to have it.

Sigh, I accidentally hit the Submit button too soon. But I guess I said pretty much everything I was going to say, except to add this to the end of the "Whatever your feelings" paragraph: "It eliminates that recursion and simplifies the complex bootstrapping cases. I can understand that code. I think anyone working on the JS engine could understand it and could explain the ordering of its actions, particularly when aided by the comments about 'create this now', 'create this next', etc. (Some of the bits like branding or whatever could be hidden a little, sure, but they're not the complexity I see inherent in reusing js_InitClass for Function and Object.)" and this to the end of the last paragraph: "When I say I'd like to not get into an argument, what I mean is that, based on the way the discussion is proceeding, I'm going to feel like I'm having one. (You might or might not feel that way, I don't know. For all I know you might think I'm just trying to duck a battle I 'know' I will 'lose'. Or you might not. Point is, how you feel when arguing with me and how I feel when arguing with you are probably different things.) It's not to say I absolutely disagree with your every contention. It's just to say that, based on the way I feel, I'm reaching the limit of my emotional stores, and if I continue trying to say what I think right at that moment, I'm going to feel completely emotionally drained. Basically, I'd like a little temporary distance for sanity's sake until I can recharge a bit. Does that make any sense?"

(In reply to comment #35) > I don't believe I was insistent on maintaining complete duplication here. > My intent (which I may not have expressed properly here or on IRC) was to > get this up, then hopefully get reviewer suggestions for improvements, and > we could iterate to something somewhat smaller. Remember that time you wanted me to use feedback? instead of r? :-|. I'll let jorendorff comment too. > I've started that iteration, and it seems to go okay so far. I'll report > back with more. Sounds good. > (In reply to comment #34) > > The summary of this bug says js_InitClass is contorted around standard > > classes, but there is only one key != JSProto_Null tests in the whole > > function, two more in DefineConstructorAndPrototype. That is not exactly > > "contorted", but it does suggest factoring that out to callers that pass key > > != JSProto_Null, if the relative order can be managed. > > I don't see key != JSProto_Null being the only way the special needs of > standard classes impose upon the logic; I listed other ways in comment 0. Let's go through that list: > * Some classes (Math) have only "constructors", and for those we pass NULL as > the constructor. This short-circuits constructor creation and headfakes the > tail of js_InitClass into not assuming it has a constructor. No, into defining a global property. That's not fake and it is not a bug. Sure, we could do Math (and StopIteration, you didn't mention that) differently. But the old way intentionally shared prototype creation and property definition code, and this bullet uses headfakey language to seem to suggest a bug. > * RangeError, etc. inherit from Error.prototype, not Object.prototype, so we > copied almost all of js_InitClass into a second method, then made both > js_InitClass and js_InitExceptionClasses call that method. Yes, so what? This is something Jason did to fix a bug. Should he have copied all the code into jsexn.cpp? No, that would be too much duplication. > * RangeError, etc. need special handling to add the handful of data > properties on the Error prototypes because js_InitClass can't add actual data > properties. No, js_InitClass can add data properties to class prototypes. Why did you think that it could not? > * QName.prototype and Namespace.prototype are actual XML objects and not > regular old objects, so we added JSCLASS_CONSTRUCT_PROTOTYPE to create a > regular old object, then create the XML constructor, then *call the > constructor* to create the real prototype object. No, neither QName.prototype nor Namespace.prototype is an XML object: js> QName.prototype ({uri:"", localName:""}) js> typeof QName.prototype "object" js> typeof Namespace.prototype "object" js> Namespace.prototype ({prefix:"", uri:""}) js> Namespace.prototype.constructor function Namespace() {[native code]} js> typeof <>hi</> "xml" js> typeof <p>text</p> "xml" The JSCLASS_CONSTRUCT_PROTOTYPE flag is a generic option, nothing to do with XML objects. It happens to be used in our source tree only by jsxml.cpp, but it's a jsapi.h feature of > 6 years' standing. > * We decided StopIteration should be frozen, so we added JSCLASS_FREEZE_PROTO > and JSCLASS_FREEZE_CTOR (but J_C_C is never used!) to freeze as quickly after > initialization as possible. Hrm, I reviewed the patch and asked Andreas to remove JSCLASS_FREEZE_CTOR. Did that followup patch bounce? Anyway, this is another jsapi.h feature, not a bug. > * Object and Function have to be created before anything else. There's an > opaque recursive dance between js_InitObjectClass and js_InitFunctionClass to > create Object.prototype, Object, Function.prototype, and Function in just the > right sequence that we get everything set up. I have no idea how it bottoms > out without error, with these self-consistent. What? You attended MIT. Recursion does bottom out here. It's a small-world codebase. I think you're (head-)faking. :-( > * We want to brand all these built-ins that have function properties, we > brand the created prototype (if it had methods) and the created method (if it > had methods). But we have to be careful of the first hack (for > prototype-less classes) and check that ctor != proto before maybe branding > the ctor! The branding has nothing to do with "standard" classes, all built-in classes that would use the JS_InitClass public API (only built-ins use it) need this logic. Yes, the constructor == null case adds a test. Is that either a measurable performance problem? No. Is it a code size issue? No, especially not compared to the patch queue you put in this bug. Is it a tiny extra complexity? Yes. Could it be avoided by factoring the code into two init-class variants, one for Math and StopIteration, the other for constructor-ful classes? Yes, but maybe that is not worth the code size hit. In any event, this is not a high priority problem to "fix". > * Because of js_InitClass's constrictions Array.prototype is created as a > dense array, then on first property definition to a slow array, rather than > being slow from the start. (bug 592296) Another very low priority bug. Meanwhile, we have crash bugs and performance bugs aplenty. > * js_InitClass has special munging to have Function to have > Function.prototype as its prototype. Yes, and we could unravel this by making Object.prototype first, then Function, then Function.prototype and Object (in either order), with custom code. Is that more code? Does it run faster? I agree it would be easier to read, though. > (How does creating Function work, if we > don't have Function.prototype yet? I think but am not certain that it picks > up an *older* Function.prototype -- because we init the Function class > multiple times during this bootstrapping -- and then gets later overwritten > with the actual, used Function.prototype before the whole thing returns.) No. Please read the code and run it under a debugger before speculating. > * All the built-in stuff should be safe against deletion and enumeration > re-resolving the builtins (it's not, but I digress), so the built-ins all > save their constructor *twice* in the global object and the prototype once in > the global object. (This is so []/{} get the original array/object > prototypes.) Public JSClass flags expose this mechanism, but no external > user should ever use them." See bug 594221 -- a separate bug, nothing to do with bootstrapping. The "I hope I have demonstrated that this is a house of cards" line was snarky but non-technical. You've demonstrated some bugs on file, pointed out a couple of if statements used to share code where factoring into separate functions could avoid the if's -- at a cost in code size -- and you mixed some generic features not specific to built-ins into what started out as an argument about only built-in aka "standard" classes. We surely have an evolved, hacked over mess of a codebase. It needs cleanup. No argument there. But some of what you write above about "We" is a slam on your friends and colleagues who fixed bugs piece-wise, sometimes under release duress. It's better to talk about these suboptimal parts in separate bugs, and not lard them together with your own misreadings mixed in to built up a weak case for rewriting something that "works" in the sense of being shipped to ~500M users. We have shipped this old code for along time, and we haven't had a lot of bugs in the area of standard class init. The Error class fakery, the StopIteration proto-freezing, are about the only recent ones I can recall. Now, this overlong bug is drawing a number of valuable hackers' time away from other bugs. I maintain that you'd do better to fix real, biting bugs and optimize competitive performance problems. And I agree that re-architecture to improve maintainability is important too. But not with incredible amounts of code duplication. Lazy standard class init removal, along the lines I cited in comment 30 last paragraph, would be better-spent time too. > If this comment were on a Get Satisfaction-styled site, my mood right now > would be :-(. Or worse. I really don't want to engage in a long, drawn-out > argument here, Oh come on. You do and you did. Know thyself, take direct peer feedback without so much deflection, fix high priority bugs, and redo extant messy code to be more maintainable without duplication, and we can all avoid these arguments. /be

> No, js_InitClass can add data properties to class prototypes. Why did you think > that it could not? Sorry, I'm flat wrong here -- I was thinking of the JSPropertyOp getter/setter native "data" properties, but they aren't true data properties with string values as jsexn.cpp wants. You're on target here. Defining data properties on prototypes is rarely done. Doing it by hand in each class init function that needs such properties might be more code than using a table-driven approach, but only if the values are well-known enough to be encoded into the table. The table-driven approach seems not to be what you're objecting to where it wins for methods. But it's just another common subroutining and loop-rolling, instead of open-coding define-native-property calls in a row. That is not in principle different from consolidating more code, especially branding after prototype property decoration. So I still don't see why the patch queue copies so much code around, even as a temporary step. /be

Finally, I'm gonna tag out of the ring too. We all need a break, I've got other fish to fry. Jason is rumored to be back tomorrow. /be

(In reply to comment #37) > > * We decided StopIteration should be frozen, so we added JSCLASS_FREEZE_PROTO > > and JSCLASS_FREEZE_CTOR (but J_C_C is never used!) to freeze as quickly after > > initialization as possible. > > Hrm, I reviewed the patch and asked Andreas to remove JSCLASS_FREEZE_CTOR. > Did that followup patch bounce? Anyway, this is another jsapi.h feature, not > a bug. See bug 606573 comment 9. But I still don't see why: js> Object.isFrozen(StopIteration) true Andreas? /be

(In reply to comment #40) > (In reply to comment #37) > > > * We decided StopIteration should be frozen, so we added JSCLASS_FREEZE_PROTO > > > and JSCLASS_FREEZE_CTOR (but J_C_C is never used!) to freeze as quickly after > > > initialization as possible. > > > > Hrm, I reviewed the patch and asked Andreas to remove JSCLASS_FREEZE_CTOR. > > Did that followup patch bounce? Anyway, this is another jsapi.h feature, not > > a bug. > > See bug 606573 comment 9. But I still don't see why: > > js> Object.isFrozen(StopIteration) > true > > Andreas? He commented at bug 606573 comment 19. FWIW, my bug 606573 comment 9 said pretty much what Jeff rightly wants for one-off unshared/uncommon code: "Non-nit: running out of JSClass.flags bits, I would have done this one ad-hoc in jsiter.cpp instead of claiming those two." Andreas will, I'm sure, file a followup bug :-/. Unless Jeff rallies here and fixes this one-off case to use one-off code. /be

Comment on attachment 530492 [details] [diff] [review] Initialize Boolean specially Bugzilla doesn't have mass-minusing UI... I promised Jeff a minus on this weeks ago but didn't get around to it. In short, - making Object and Function initialization totally special sounds great; - making standard classes a little more special than they are, and making the public JS_InitClass have fewer special cases and ins and outs, sounds good; - this stack of patches has way, way too much copy & paste. The last thing is a deal-breaker. To avoid the duplication, can we just pass the proto (rather than the proto-proto) as a parameter to the InitClass function? If not the proto, then perhaps a function pointer (or some other sort of parameter) that encompasses whatever varies from Class to Class? I totally agree with comment 32. That's an ugly trend; let's reverse it.

Oh, I meant to also have a bullet that said something like, - making stuff like Math and StopIteration not go through InitClass is also something I'd be happy to review a patch for -- preferably in isolation Splitting these bullet points into separate bugs would be welcome.

Comment on attachment 530507 [details] [diff] [review] Move some variable declarations in js_InitXMLClass closer to first uses (I'm fine with taking this little patch, if you want it.)

Comment on attachment 530518 [details] [diff] [review] Update initSharingEmptyShape's comment to be more precise, and to not refer to js_InitClass (which built-in classes don't use) This comment cleanup seems good too.

Comment on attachment 530515 [details] [diff] [review] Move js_InitFunctionAndObjectClasses into jsfun.cpp If you want to move js_InitFunctionAndObjectClasses, js_InitFunctionClass, and js_InitObjectClass to the same file, in anticipation of straightening that out, go for it, r=me. I would pick either vm/GlobalObject.cpp (because I see this as part of global object initialization) *or* jsobj.cpp (because that's where js_InitClass is, and the existing code is tightly coupled to that) rather than jsfun.cpp, but any of the three is fine.

Please do start populating vm/GlobalObject.cpp -- that is a righteous place and trend to foster. /be

Comment on attachment 530519 [details] [diff] [review] Rewrite js_InitFunctionAndObjectClasses to not recur horribly and inscrutably r- for copy & paste reasons again, but I just wanted to note that GlobalObject::getObjectPrototype looks delicious. Here you keep support for lazily initializing Object and Function, which is best for now, but boy will it be a happy day when those get initialized as part of global object creation. One less crazy-makin' corner case to worry about. (I wonder if a bug or two doesn't lurk there, now that I think about it. Even newGlobal never returns a truly empty global object for the fuzzer to abuse.)

http://hg.mozilla.org/tracemonkey/rev/09326129c5c3 for declaring XML variables late http://hg.mozilla.org/tracemonkey/rev/d3daeb8ebbd6 to update the initSharingEmptyShape comment More soon.

cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/09326129c5c3 http://hg.mozilla.org/mozilla-central/rev/d3daeb8ebbd6 Note: not marking as fixed because fixed-in-tracemonkey is not present on the whiteboard.