Closed Bug 571789 Opened 15 years ago Closed 14 years ago

merging JSObjectOps into JSClass

Categories

(Core :: JavaScript Engine, enhancement)

Product:
Core
Component:
JavaScript Engine
Version:
Other Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 12 obsolete files)

eliminating extended class v8
318.32 KB, patch
igor
: review+
Details | Diff | Splinter Review
diff between v7 and v8
8.10 KB, patch
Details | Diff | Splinter Review
JSObjectOps represents effectively a second vtable in addition to JSClass methods that exists in JSObject. Thus the idea is to move all JSObjectOps methods into a structure inside JSClass. If this structure would be declared at the end of JSClass, it should be source-compatible with any native class implementation. In addition to simplifying the structure of JSObject it should also allow to remove the ops field from JSObjectMap as all non-native classes can have single shared map and obj->isNative() would become just obj->map == &shared_nonnative_map;
Attached patch patch (obsolete) — — Splinter Review
The initial untested implementation
Whats the advantage of this? I mean does it save us anything? Also, I think slow arrays can be just regular native objects now.
(In reply to comment #2) > Whats the advantage of this? I mean does it save us anything? This saves one word per JSScope and eliminates an extra load per isNative check. During NewObject it saves a few loads loads/stores and for any class with non-default JSObjectOps it eliminates an indirect call to get object ops. The drawback is that for many JSObjecOps methods there would be an extra null check to call the default implementation when the method is null, but those are not on the fast paths and eliminating an extra load to read the method would somewhat compensate for that. and per any JSObjecOps call. > > Also, I think slow arrays can be just regular native objects now.
Attached patch patch (obsolete) — — Splinter Review
This patch passes shell and jsapi tests. With it I observe 0.5%-1% gains with v8/sunspider on my somewhat noisy laptop. To merge JSObjectOps into JSClass I have moved a few function pointer typedefs related from jsprvtd.h to jsapi.h. I am not sure how to make it absolutely clear that those types are internal and are subject to change. Are comments sufficient? When I get results from the tryserver I will ask mrbkap to review xpcom-related changes.
Attachment #450949 - Attachment is obsolete: true
Attachment #451050 - Flags: review?(jorendorff)
I need something like this for bug 558451 not to regress perf, but I do not think we should add more private stuff to jsapi.h. Rather, per the "JSObjectOps make-over" bug 408416, use JSClass as a recipe for an internal composite that object points to directly. This requires saving a pointer to the API-user supplied JSClass, for instance-of testing. To avoid hashing, we could extend JSClass with just one "data" or "secret" field, not initialized or used by the API consumer. This would need to be set to the internal composite, if non-null, in a race-free fashion. /be
(In reply to comment #5) > To avoid hashing, we could extend JSClass with just one "data" or "secret" > field, not initialized or used by the API consumer. This would need to be set > to the internal composite, if non-null, in a race-free fashion. This still would add a cost of an extra indirect when accessing the fields. IMO having one or eleven fields at the end of JSClass does not matter if the compiler initializes all those fields into NULL if the user does not declare them. To avoid too much exposure I considered to add just something like ReservedOp resevedOp[11] where ReservedOp is void (*f)() and uses casts to access them in the engine. But that looks ugly. So what about just stuffing the extra declarations in some header file that is included by jsapi.h but that has big warnings that the stuff is private one.
(In reply to comment #6) > (In reply to comment #5) > > To avoid hashing, we could extend JSClass with just one "data" or "secret" > > field, not initialized or used by the API consumer. This would need to be set > > to the internal composite, if non-null, in a race-free fashion. > > This still would add a cost of an extra indirect when accessing the fields. No, the layout for JSObject would be {ops, props, dslots, fslots[5]} and ops would point to the composite struct containing function pointers -- so one fewer dependent load than today, same as your patch. But without more API exposure. Another problem with extending JSClass is that we can't fold in type inference results (see bug 557407). We really do not want an API-client allocated singleton for the composite structure, it is engine internal and needs to evolve freely. /be
(In reply to comment #5) > To avoid hashing, we could extend JSClass with just one "data" or "secret" > field, not initialized or used by the API consumer. This would need to be set > to the internal composite, if non-null, in a race-free fashion. Do you mean to suggest we should actually take advantage of the argument being JSClass* rather than const JSClass*? Just trying to make sure I understand what you're actually proposing...
(In reply to comment #8) > (In reply to comment #5) > > To avoid hashing, we could extend JSClass with just one "data" or "secret" > > field, not initialized or used by the API consumer. This would need to be set > > to the internal composite, if non-null, in a race-free fashion. > > Do you mean to suggest we should actually take advantage of the argument being > JSClass* rather than const JSClass*? Yes! /be
(In reply to comment #7) > No, the layout for JSObject would be {ops, props, dslots, fslots[5]} and ops > would point to the composite struct containing function pointers -- so one > fewer dependent load than today, same as your patch. But without more API > exposure. A composite structure implies a management of lifetime of such structures to track at least the lifetime of the original classes. I.e. the composite structure should be deleted when the class is removed. Reserving some space in the class itself avoids this extra complexity. > > Another problem with extending JSClass is that we can't fold in type inference > results (see bug 557407). True, but that can be dealt using your observation that JSClass * can be mutated. > We really do not want an API-client allocated > singleton for the composite structure, it is engine internal and needs to > evolve freely. I do not see why reserving some extra space in JSClass is problematic especially since any changes would be source-compatible.
(In reply to comment #10) > (In reply to comment #7) > > No, the layout for JSObject would be {ops, props, dslots, fslots[5]} and ops > > would point to the composite struct containing function pointers -- so one > > fewer dependent load than today, same as your patch. But without more API > > exposure. > > A composite structure implies a management of lifetime of such structures to > track at least the lifetime of the original classes. I.e. the composite > structure should be deleted when the class is removed. Right, XPConnect and other users of dynamically allocated JSClasses would need to call a new API entry point. The common case of static singleton JSClass need not change. > Reserving some space in the class itself avoids this extra complexity. The trade-off is that we'd need to reserve a magic amount to avoid exposing the internal sizes of the structures combined into the composite. This seems better in terms of ultimate efficiency, worse in terms of avoiding bugs. > > Another problem with extending JSClass is that we can't fold in type inference > > results (see bug 557407). > > True, but that can be dealt using your observation that JSClass * can be > mutated. Still need to reserve space for everything. > > We really do not want an API-client allocated > > singleton for the composite structure, it is engine internal and needs to > > evolve freely. > > I do not see why reserving some extra space in JSClass is problematic > especially since any changes would be source-compatible. If people use JSCLASS_NO_{OPTIONAL,RESERVED}_MEMBERS.... Anyone else have an opinion? /be
I don't think the lifetime management is problematic. Lets just gc those composite structures. I like the composite structure and using class as recipe.
(In reply to comment #11) > The trade-off is that we'd need to reserve a magic amount to avoid exposing the > internal sizes of the structures combined into the composite. This seems better > in terms of ultimate efficiency, worse in terms of avoiding bugs. ... > Still need to reserve space for everything. ... > If people use JSCLASS_NO_{OPTIONAL,RESERVED}_MEMBERS.... The bugs can only happen if the target is binary compatibility. That is not what the target here. With source-level compatibility it does not matter how many members are in the struct if they defaults to NULL. At worst the compiler would warn about missing NULL initializers, but an embedding can fix this using a macro in the source.
(In reply to comment #12) > I don't think the lifetime management is problematic. Lets just gc those > composite structures. A GC would not help as it is not known if JSClass would not be used again. A new API to tell the engine that JSClass is no longer used would be necessary.
(In reply to comment #14) > A new API to tell the engine that JSClass is no longer used would be > necessary. How far down does the rabbit hole go? How many new APIs need to be added each time we "simplify"? Given how many backwards-incompatible changes we've made in the last couple years, source compatibility seems like the very least of the worries for embedders. I don't think we should hesitate to break source compatibility to merge these two vtables, not when the alternative is so barnacle-encrusted to start (I strongly suspect many, if not most, embedders will have const JSClasses whose constness has been casted away to be passed into API entry points) and seems to be growing yet more as comments progress. Is it really important for the web that we chain ourselves in this manner? Luke mentioned an intriguing idea of making JSClass-style overlays merely a special case form of object operations, which at the least sounds intriguing.
(In reply to comment #15) > (In reply to comment #14) > > A new API to tell the engine that JSClass is no longer used would be > > necessary. > > How far down does the rabbit hole go? How many new APIs need to be added each > time we "simplify"? The GC idea from Andreas in comment 12 would avoid a new API. The concern in comment 14 is addressed by re-creating on demand internal vtables that were GC'ed -- we would need this anyway for JSClasses that were used without ever being passed to JS_InitClass. > Given how many backwards-incompatible changes we've made in the last couple > years, source compatibility seems like the very least of the worries for > embedders. The issue is not source compatibility, please re-read comment 13. > (I strongly suspect many, if not most, embedders will have const > JSClasses whose constness has been casted away to be passed into API entry > points) I've never seen any such embedding code -- not ever, and I've seen a lot. At this point we need evidence, not suspicion. > and seems to be growing yet more as comments progress. Is it really > important for the web that we chain ourselves in this manner? Can you cut back on the straw-man and rhetorical question style? It's unnecessary and annoying. > Luke mentioned an intriguing idea of making JSClass-style overlays merely a > special case form of object operations, which at the least sounds intriguing. This is unproductively vague (and "intriguing" is a flagrant Gallicism!). What did Luke mention, where? This bug is already about a JSClass "overlay" or extension, so either there's a new proposal, or the same idea. This whole bug is about whether and how to unify JSClass with JSObjectOps by folding the latter (or some evolution of it) into the former. Ignoring member offset fine-tuning, it doesn't matter which folds into what -- except precisely whether the API has to expose a bunch of dummy void (*fpN)(); members taking up exactly the right amount of space for the overlaid internal struct. The argument is thus about whether the internals should be hidden, including their size and (potentially) alignment. /be
Keeping the combined struct purely internal would also avoid the JSNoClearOp, JSNoHasInstanceOp, etc. hacks, and the extra tests they engender to default null to the native op, else call the configured op -- unless it is one of these near-null magic cookies. We could just declare a big char array at the end of JSClass, and statically assert that it was the right size (or big enough) for the internal struct. This would address the information hiding concern. Anyone enlarging the internal type would have a static assert catch failure to update the manifest array size constant. But this would not address the cost have having to default to native object op for null op, and skip for nearly-null JSNoFooOp cookies. /be
(In reply to comment #16) > Can you cut back on the straw-man and rhetorical question style? It's > unnecessary and annoying. Sorry, maybe it's just my natural style to try to cause the reader to derive the issues for himself (better to learn for oneself than to be told). What form of argument would you be more receptive to hearing, which wouldn't produce a complaint of strawmanship? > > Luke mentioned an intriguing idea of making JSClass-style overlays merely a > > special case form of object operations, which at the least sounds > > intriguing. > > This is unproductively vague (and "intriguing" is a flagrant Gallicism!). > > What did Luke mention, where? This bug is already about a JSClass "overlay" or > extension, so either there's a new proposal, or the same idea. He mentioned it in passing to me just minutes before I made the comment. As it was only a brief mention, without particular detail, I felt unable to say more than that it seemed worth evaluating further (also to incent him to elaborate, although it occurs to me he might possibly not see it without the CC). Without knowing those details or having especially evaluated its technical feasibility, I can't very well wholeheartedly recommend it. If you wish a more concise statement of the argument, it is this: Preserving source compatibility hinders bugfixing and improvement, at the absolute least by necessitating discussion of the API to set in stone. When the constraint is, well, constraining, it encourages us to play fast and loose with what we "promised" (were JSClass ten years newer it would be const; I suspect some embedders depend on this, however fixably, with a const-cast for type-correctness). Structures, behaviors, and invariants become more complicated. And at the end of the day, the particular modus operandi doesn't win functionality or performance. If when we make a source-breaking change we can provide a natural and relatively simple path forward, I think that's sufficient, and it avoids our artificially constraining ourselves and our game plan. I understand this issue in general is a very fundamental disagreement between us, but I have hope it will not remain so forever.
As a more specific concern: (In reply to comment #16) > The GC idea from Andreas in comment 12 would avoid a new API. It would indeed. Is this a good enough reason to add more contents to the GC heap? Contents whose natural lifetime is the life of the application? (Oops, I guess I'm lapsing into rhetorical questions.) I don't see that the compatibility benefit offsets the heap bloat, the incremental GC increase, or the added complexity -- feels like a step back toward fslots[JSSLOT_CLASS] to me.
Heap bloat? How many classes does our embedding have? 12? Is that the right ballpark? Lets stick to valid arguments. There is some extra complexity for the GC part, and some extra marking effort. Those seem valid arguments, but heap bloat is certainly not.
Fair enough. (But is turnabout fair play? ;-) )
The idea attributed to me wasn't even my idea; I have heard it mentioned in passing by different people. The idea is just: "Mostly, what we want is object ops, so let's add whats missing (i.e., only doable with JSClass) to object ops and support legacy JSClass use with a special 'forward to class' object ops". When I was new to SM a year ago, the JSObjectOps/JSClass duality was definitely one of the scarier points, so I find the unification very attractive. But that's probably a bit off track for this bug.
I am all for unification and simplification, but ObjectOps and Class serve orthogonal purposes. ObjectOps define object behavior, Class observes it. Not easy to merge in a meaningful way. I think we could mostly get rid of ObjectOps. Dense arrays already have a special path everywhere. The only other non-native object we have are proxies, which can be used to implement everything else we might need in the future. That's just my 2c.
(In reply to comment #23) > ObjectOps define object behavior, Class observes it. Incidentally, that's the best summary I've heard :)
(In reply to comment #24) > (In reply to comment #23) > > ObjectOps define object behavior, Class observes it. > > Incidentally, that's the best summary I've heard :) If it sounds too good to be true, it probably is. Class observes *native* object behavior (and some quasi-natives, blech). History had class first, then object ops (for liveconnect, but eventually for other stuff including xpconnect and dense arrays). Andreas is optimistic proxies cover all the non-native cases, so we could go back to a mighty if statement instead of a vtbl. I'm not sure we want to bet on proxies that way -- for one thing, it would require making dense array optimizations universal for all native objects, not via property tree paths for the indexed properties AKA elements -- but we already have many "if" bypasses for native-only fast paths. So it's plausible. Let's take things one step at a time. Keep object ops as the high-level vtbl, and class as the API exposed low-level vtbl or vtbl-recipe. Back to info-hiding vs. not. /be
Just to reinforce a point that people may not have heard (enough): object ops are broken in various ways, and the fix is to make them match the harmony:proxies handler interface more closely, if not trap-wise equivalent. This should happen in bug 408416 or something blocking it (408416 is kinda meta at this point). See bug 566143 and bug 566836 at least (these would be dependency bugs of 408416 or the new bug that blocks it). Not this bug but I like leaving breadcrumbs in bugzilla, it's a collective form of memory (albeit painful to search without the awesomebar). /be
Comment 18 asks why not break source compatibility, but doesn't propose how. The problem isn't source compat -- we can break it if need to -- again the issue is whether and how to hide the internal JSObjectOps struct but still flatten it and JSClass together. Comment 19 is unreal, especially the fslots[JSSLOT_CLASS] comparison. GC'ed ops tables inside the VM have nothing in common with that ancient history. I really don't know where to begin, so I'll end there. But please, no long-winded metaphorical justifications! The form of argument we should all seek is precise, concise, and accurate or at least relevant. /be
Attached patch expose only size of JSObjectOps (obsolete) — — Splinter Review
For references here is a version of the patch that just exposes an array of 17 of void (*)() pointers. The usage is uglier rather than with an explicit structure but it does not look bad.
(In reply to comment #17) > Keeping the combined struct purely internal would also avoid the JSNoClearOp, > JSNoHasInstanceOp, etc. hacks, and the extra tests they engender to default > null to the native op, else call the configured op -- unless it is one of these > near-null magic cookies. The cookies is only necessary for now for few cases like construct/call. With some work those can be eliminated since ideally JSClass.construct/call should be sufficient. Other cookies can also be dealt with.
Depends on: 572411
Depends on: 572494
The point wasn't that the two were mechanically similar. It was that keeping an object's class pointer in a jsval slot made as little conceptual sense as making classes GC-able seems to make now. This is my concern: that we use the conceptually simplest means to overcome each problem we encounter. We should not let source compatibility impede us in doing this. The conceptually simplest way to merge ops and class as this bug requests is to break source compatibility rather than to have users specify the two separately (and pellucidly document that the objectops methods in the class are an unstable, friend-only API), use mutation in combining the two at runtime, and ultimately GC that combined structure. If you wish for this in argument form: combine the two, don't hide the unstable object-ops bits, and document that those bits are not to be used by embedders unwilling to use friend APIs and accept ongoing breakage.
Comment on attachment 451406 [details] [diff] [review] expose only size of JSObjectOps I believe keeping all the internal operations as JSClassInternalOp will cause problems in the future as that list of operations is modified. I have found it extremely helpful in the past to get loud compiler errors when I have incompletely or inaccurately modified JSObjectOps, particularly during patch merges with changes made by others. Losing those makes that process notably more error-prone, and those errors would not show up until runtime. This would also mean committing to deliberately testing every one of those hook methods on every type of object, to ensure nothing slipped through a type hole we opened through an invalid cast. We *should* be doing that in effect. However, I would find it difficult to trust anything but a test that deliberately did so. I find it implausible that there's no esoteric object/hook combination that we don't test -- I don't think we would be exhaustive without deliberately aiming for such.
Waldo, we used to be C, and we originally had a single-size-class (cons-cell) GC allocator. So there was only obj->slots, hence obj->slots[JSSLOT_CLASS]. To bring this up now is like complaining about fashions during the reign of Louis XVI being to blame for Lilo's latest embarrassment! This bug is about conceptual cleanliness: JSClass is an API recipe (and yes, the API has no const JSClass *, so source compatibility cuts both ways: we do not keep you working if you cast away your own const on the definition), the internal ops table is hidden completely. There's no reason with C++ and modern best practices to have to combine the two. I don't understand comment 31 at all. Testing all-combinations coverage of any abstract interface is only as good as testing the implementations. Putting an internal JSObjectOps in the public header, after (comment 30) telling API users not to initialize or call those ops, does nothing to help test. Indeed it just makes an attractive nuisance that we can't test, among the embeddings. Back to information hiding. /be
(In reply to comment #32) > Putting an > internal JSObjectOps in the public header, after (comment 30) telling API users > not to initialize or call those ops, does nothing to help test. Indeed it just > makes an attractive nuisance that we can't test, among the embeddings. With a debug-only whitelist of classes that are allowed to have non-default ops it would be trivial to catch embedding errors during development.
The slow array object ops are not necessary as I commented above.
Instead of op = obj->getOps()->trace and then if (!op) js_TraceObject(); else op() how about obj->trace() ?
(In reply to comment #33) > (In reply to comment #32) > > Putting an > > internal JSObjectOps in the public header, after (comment 30) telling API users > > not to initialize or call those ops, does nothing to help test. Indeed it just > > makes an attractive nuisance that we can't test, among the embeddings. > > With a debug-only whitelist of classes that are allowed to have non-default ops > it would be trivial to catch embedding errors during development. This would catch embedding initialization of assignment of internal members, but not *use* (read-only, call-only) of those members. But really, it's possible to expose everything (jsobj.h is private but still exported) and have people abuse the internal stuff. We can always say "you were warned" and break. The information hiding goal isn't absolute (there's no way to hide fully in a shared address space -- B&D languages try but this just drives some users to try more evil hacks). Whatever we do here, we shouldn't have nearly-null cookies or more if statements where nullable ops will do. /be
(In reply to comment #32) > I don't understand comment 31 at all. The point was this: suppose I'm working on a patch that modifies the object ops structure, or whatever future thing it happens to be or be part of. I insert a couple new methods, or I rearrange a few of them. With all the casts that patch requires, I get no compile-time feedback if I happen to mis-order the function pointers. If I happen to forget one, I also get no compile-time feedback. Even assuming I have it right at some instant in time, if I end up merging with someone else's changes to that table, I get no feedback if I introduce an error while merging (say, like having an off-by-one error in where I introduce a new function pointer). In short, common mistakes don't show up until runtime, because we've subverted the type system and told it to act as though no type error is being made. These issues are not theoretical. I have run into these problems multiple times in the past with JSObjectOps changes I have made, and only compile errors have saved me from a lengthy browser-compile process and eventual runtime blowups. I didn't intend the all-combinations testing to include anything but the classes we define ourselves. Embedders missing out just seems the least bad option.
(In reply to comment #37) > (In reply to comment #32) > > I don't understand comment 31 at all. > > The point was this: suppose I'm working on a patch that modifies the object ops > structure, or whatever future thing it happens to be or be part of. I insert a > couple new methods, or I rearrange a few of them. With all the casts that > patch requires, I get no compile-time feedback if I happen to mis-order the > function pointers. Oh, you're talking about the patch Igor attached that casts void(*)() -- agreed, that is bad. Thanks for clarifying (I should have figured this out). The right way to decouple API from internals is not to cast function pointers. Just reserve space and statically assert it's big enough for the well-typed internal ops struct. /be
(In reply to comment #38) > The right way to decouple API from internals is not to cast function pointers. > Just reserve space and statically assert it's big enough for the well-typed > internal ops struct. Do you suggest do declare JSClass like in: struct JSClass { public_declarations; jsuword reserve[20]; } and then have an internal class like: struct js::ClassWithOps { public_declarations; ops; } plus a helper inline to cast js::ClassWithOps into JSClass ?
(In reply to comment #39) > (In reply to comment #38) > > The right way to decouple API from internals is not to cast function pointers. > > Just reserve space and statically assert it's big enough for the well-typed > > internal ops struct. > > Do you suggest do declare JSClass like in: > > struct JSClass { public_declarations; jsuword reserve[20]; } > > and then have an internal class like: > > struct js::ClassWithOps { public_declarations; ops; } > > plus a helper inline to cast js::ClassWithOps into JSClass ? Something like that -- might be better to use uint8 or char for the opaque array, and of course static assert that it's big enough. The name matters too. Perhaps we should call it js::ObjectOps and think of it that way. Minor point. /be
Comment on attachment 451050 [details] [diff] [review] patch I don't think the public/private split Brendan suggests is necessary. It's weird to be exposing fields and types that we consider private, but JSObjectOps was exposed for years and we were able to keep evolving it and eventually retract it. These comments are mostly nits but there's one bug in jstracer.cpp, in HasInstance. In jsapi.h: >+typedef void (*JSClassInternalOp)(); Unused. In jsarray.cpp, js_SlowArrayClass: >- slowarray_getObjectOps, NULL, NULL, NULL, >- NULL, NULL, NULL, NULL >+ NULL, NULL, NULL, NULL, >+ NULL, NULL, NULL, NULL, Here slowarray_getObjectOps should change to true or false, not NULL. In jsobj.h, JSObject::lookupProperty: >+ return !op >+ ? js_LookupProperty(cx, this, id, objp, propp) >+ : op(cx, this, id, objp, propp); All of these would look nicer to me swapped: return op ? op(cx, this, id, objp, propp) : js_LookupProperty(cx, this, id, objp, propp); but do whatever you think best. In jsops.cpp, CASE(JSOP_INSTANCEOF): >- if (!obj->map->ops->hasInstance(cx, obj, lval, &cond)) >+ if (!op ? !js_HasInstance(cx, obj, lval, &cond) : !op(cx, obj, lval, &cond)) > goto error; > regs.sp--; > STORE_OPND(-1, BOOLEAN_TO_JSVAL(cond)); >+} Please make this into a method of JSObject along with all the others. In jspubtd.h: >+ * See JSCheckAccessIdOp, below, for the JSClassInternalOp counterpart, which It's not actually below; it's in a different file. In jstracer.cpp, HasInstance: >- if (!ctor->map->ops->hasInstance(cx, ctor, val, &result)) >+ if (!op ? !js_HasInstance(cx, ctor, val, &result) : op(cx, ctor, val, &result)) > SetBuiltinError(cx); The result of op() needs a ! too. It would be even better to use ctor->hasInstance(...). In jsxml.cpp: > JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, > JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, xml_finalize, >- xml_getObjectOps, NULL, NULL, NULL, >- NULL, NULL, JS_CLASS_TRACE(xml_trace), NULL >+ false, NULL, NULL, NULL, >+ NULL, NULL, JS_CLASS_TRACE(xml_trace), NULL, Make all those NULLs line up, please. In xpcJSWeakReference.cpp, xpcJSWeakReference::Get(): >- if (obj->map->ops->thisObject && >- !(obj = obj->map->ops->thisObject(cx, obj))) >+ if (obj->getClass()->internalOps.thisObject && >+ !(obj = obj->getClass()->internalOps.thisObject(cx, obj))) Use getOps() here, perhaps?
Attachment #451050 - Flags: review?(jorendorff) → review+
(In reply to comment #41) > I don't think the public/private split Brendan suggests is > necessary. It's weird to be exposing fields and types that we consider > private, but JSObjectOps was exposed for years and we were able to > keep evolving it and eventually retract it. It's not a matter of what is necessary, since we can't stop determined embedders from using anything in the open source, or even hacking their own changes into forks of it but still taxing us with "why does it crash?" bugs. Information hiding in any C-like language, and even in many "managed runtime" languages, is advisory, not mandatory. You can't stop people from doing evil or stupid hacks to pun memory and abuse abstractions. But you can make normative statements about what you will and won't support. That's all I'm asking for here, in code not words. > All of these would look nicer to me swapped: > > return op > ? op(cx, this, id, objp, propp) > : js_LookupProperty(cx, this, id, objp, propp); > > but do whatever you think best. This is even better written > return (op ? op : js_LookupProperty)(cx, this, id, objp, propp); Why duplicate the actual parameter list? > In jsops.cpp, CASE(JSOP_INSTANCEOF): > >- if (!obj->map->ops->hasInstance(cx, obj, lval, &cond)) > >+ if (!op ? !js_HasInstance(cx, obj, lval, &cond) : !op(cx, obj, lval, &cond)) > > goto error; Minimizing duplication and inverting the test as Jason suggests here also has the nice effect of putting the ! on the outside, and avoiding two more bangs: > >+ if (!(op ? op : js_HasInstance)(cx, obj, lval, &cond)) > > goto error; Look out for other such cases, they really should not be so redundant in terms of extra operators and arg lists. /be
BTW I want this for bug 558451 to reduce the size of JSObject, so please do land soon. Take the time to digest comment 42, though :-|. /be
Blocks: 558451
(In reply to comment #43) > BTW I want this for bug 558451 to reduce the size of JSObject, so please do > land soon. Take the time to digest comment 42, though :-|. I will have a patch tomorrow with "char reserved[100]" in JSClass and separated ObjectOps. This seems a winner - no leak in the public header while avoiding ugly casts and preserving type-safety. (In reply to comment #42) > This is even better written > > > return (op ? op : js_LookupProperty)(cx, this, id, objp, propp); > > Why duplicate the actual parameter list? I need to check if at least GCC can optimize this properly with using jump into js_LookupProperty implementation.
(In reply to comment #44) > (In reply to comment #42) > > This is even better written > > > > > return (op ? op : js_LookupProperty)(cx, this, id, objp, propp); > > > > Why duplicate the actual parameter list? > > I need to check if at least GCC can optimize this properly with using jump into > js_LookupProperty implementation. That not only improves the readability but also generates smaller code with GCC. Consider the following source x.cpp: struct S; typedef bool (*F)(int a1, S *s, int a3, int a4); struct S { F f; }; extern F default_function; extern "C" { // What is in patch bool test1(int a1, S *s, int a3, int a4) { return !s->f ? default_function(a1, s, a3, a4) : s->f(a1, s, a3, a4); } // Suggestion bool test2(int a1, S *s, int a3, int a4) { return (s->f ? s->f : default_function)(a1, s, a3, a4); } } Compiling using GCC 4.4 for x86_64: gcc -c -S -Wall -Wextra -O3 -fno-exceptions -fno-rtti -fomit-frame-pointer x.cpp That gives: test1: movq (%rsi), %rax testq %rax, %rax je .L5 jmp *%rax .L5: movq default_function(%rip), %rax jmp *%rax test2: movq (%rsi), %rax testq %rax, %rax cmove default_function(%rip), %rax jmp *%rax Here given the presence of cmove the compiler generates smaller code for the suggested case. Compiling for 32 bit: gcc -c -S -Wall -Wextra -O3 -fno-exceptions -fno-rtti -fomit-frame-pointer -m32 -march=i686 x.cpp gives: test1: subl $12, %esp movl 20(%esp), %eax movl %ebx, 4(%esp) movl 16(%esp), %ecx movl %esi, 8(%esp) movl 24(%esp), %ebx movl 28(%esp), %esi movl (%eax), %edx testl %edx, %edx je .L5 movl %esi, 28(%esp) movl 8(%esp), %esi movl %ebx, 24(%esp) movl 4(%esp), %ebx movl %eax, 20(%esp) movl %ecx, 16(%esp) addl $12, %esp jmp *%edx .L5: movl default_function, %eax movl 4(%esp), %ebx movl 8(%esp), %esi addl $12, %esp jmp *%eax test2: subl $12, %esp movl 20(%esp), %edx movl (%edx), %eax testl %eax, %eax cmove default_function, %eax addl $12, %esp jmp *%eax Here even better win is observed. The bottom line is that the most readable code wins.
Depends on: 576596
The new version uses JSClass { public_fields; char reserved[buffer]; } so the only leakage to the public header is that sizeof(ObjectOps) <= sizeof(reserved). The patch also avoids type safety issues as there are no casts when declaring classes with non-null ops. Initially it looked not that bad, but with the need to support the extended classes with non-null ObjectOps made things rather ugly with all those extra classes like ClassAndOps, ClassAndOpsAndPad and ExtendedClassWithOps. So personally I would prefer the initial solution of explicitly adding JSObjectOps methods to jsapi.h with comments about their non-public status. So I asking for a feedback on this incomplete patch (there no xpconnect changes here) to decide on the approach here. The patch depends on the patches from bug 572411, bug 572494, and bug 576596 to avoid NoCall -near NULL pointers from the previous patches in this bug.
Attachment #451406 - Attachment is obsolete: true
Attachment #456083 - Flags: feedback?(brendan)
Comment on attachment 456083 [details] [diff] [review] minimal public exposure without casts FIELDS is misspelled FILEDS throughout. We don't need to keep API compat for JSExtendedClass, so I think you could still hide the ops using a buffer, and combine things, but whatever is easier. IOW, if we did get rid of JSExtendedClass, would we might then find that the patch here was not messy? We can go in any order, and hide whenever, but I'm looking for consensus on where we are going, not on immediate next stop along the way. In that light, feedback- as I agree that the complexity of keeping JSExtendedClass alongside the information-hiding of ops is worse than exposing ops, pragmatically and as an intermediate state. Question is, can we squash JSExtendedClass in this bug or should we keep it as a separate project? If separate, when? /be
Attachment #456083 - Flags: feedback?(brendan) → feedback-
(In reply to comment #47) > Question is, can we squash JSExtendedClass in this bug or should we keep it as > a separate project? If separate, when? If we can remove ExtendedClass, than lets remove it first in a separated bug. Without the extended class the patch becomes significantly more reasonable.
(In reply to comment #48) > (In reply to comment #47) > > Question is, can we squash JSExtendedClass in this bug or should we keep it as > > a separate project? If separate, when? > > If we can remove ExtendedClass, than lets remove it first in a separated bug. > Without the extended class the patch becomes significantly more reasonable. I have missed that the suggestion is not to move the extended class into JSClass but rather hide it from jsapi.h. If so then doing this here as a single patch is simpler.
Attached patch eliminating extended class (obsolete) — — Splinter Review
The new patch moves the functionality of JSExtendedClass into ObjectOps. Without the former keeping ObjectOps outside jsapi.h becomes much simpler.
Attachment #451050 - Attachment is obsolete: true
Attachment #456083 - Attachment is obsolete: true
Attachment #456165 - Flags: feedback?(brendan)
Attached patch eliminating extended class v2 (obsolete) — — Splinter Review
More complete patch with xpc parts. But it still does not compile as jsd needs a C version of js::ObjectOps. I think I will move the new structure from jsobj.cpp to jsprvtd.h and rename it back to JSObjectOps so jsd can include it.
Attachment #456165 - Attachment is obsolete: true
Attachment #456202 - Flags: feedback?(brendan)
Attachment #456165 - Flags: feedback?(brendan)
Attached patch eliminating extended class v2 (obsolete) — — Splinter Review
The prev attachment has a wrong patch.
Attachment #456202 - Attachment is obsolete: true
Attachment #456204 - Flags: feedback?(brendan)
Attachment #456202 - Flags: feedback?(brendan)
Attached patch eliminating extended class v3 (obsolete) — — Splinter Review
Here is a version that compiles.
Attachment #456204 - Attachment is obsolete: true
Attachment #456336 - Flags: feedback?(brendan)
Attachment #456204 - Flags: feedback?(brendan)
Attached patch eliminating extended class v4 (obsolete) — — Splinter Review
The patch passes tests. It does not affect SS or V8 benchmarks.
Attachment #456336 - Attachment is obsolete: true
Attachment #456873 - Flags: review?(jorendorff)
Attachment #456336 - Flags: feedback?(brendan)
Attached patch eliminating extended class v5 (obsolete) — — Splinter Review
Initial update of the patch to account with fatval changes. The patch piggyback on the split between JSClass and Class and defines ObjectOps as a member of the internal Class. But I have not yet updated xpconnected part of the patch.
Attachment #456873 - Attachment is obsolete: true
Attachment #456873 - Flags: review?(jorendorff)
Attached patch eliminating extended class v6 (obsolete) — — Splinter Review
The patch passes tests.
Attachment #458123 - Attachment is obsolete: true
Attachment #458377 - Flags: review?(jorendorff)
With the latest patch I observed about 1.3% win with 32-bit CPU on SS with no change with V8. The biggest SS win came from unpack-code with 7% speedup.
Attached patch eliminating extended class v7 (obsolete) — — Splinter Review
Attachment #458377 - Attachment is obsolete: true
Attachment #458377 - Flags: review?(jorendorff)
Attachment #459402 - Flags: review?(jorendorff)
This is a review of v6. In jsapi.h: > struct JSClass { >- const char *name; >- uint32 flags; [...] >+ const char *name; \ >+ uint32 flags; \ [...] You added some backslashes, but they aren't necessary. In jsgc.cpp: >+ if (!op) >+ js_TraceObject(trc, obj); >+ else >+ op(trc, obj); Maybe you can say (op ? op : js_TraceObject)(trc, obj); here too. In jsobjinlines.h, InitScopeForObject: >- JS_ASSERT(ops->isNative()); >+ JS_ASSERT(!(clasp->flags & CLASS_NON_NATIVE)); Can we have a method js::Class::isNative() for this sort of thing? In jstracer.cpp: >+ if(l.toObject().getClass()->ext.equality) Style nit: Add a space: `if (`. In XPCNativeWrapper.h: > inline PRBool >-IsNativeWrapperClass(JSClass *clazz) >+IsNativeWrapperClass(js::Class *clazz) > { >- return clazz == &internal::NW_NoCall_Class.base || >- clazz == &internal::NW_Call_Class.base; >+ return clazz == &internal::NW_NoCall_Class || >+ clazz == &internal::NW_Call_Class; > } Style nit: The second line should line up `clazz ==` with the first. In xpcjsruntime.cpp, XPConnectGCChunkAllocator: > virtual void *doAlloc() { >- void *chunk = 0; >+ void *chunk; > #ifdef MOZ_MEMORY >- posix_memalign(&chunk, js::GC_CHUNK_SIZE, js::GC_CHUNK_SIZE); >+ if (posix_memalign(&chunk, js::GC_CHUNK_SIZE, js::GC_CHUNK_SIZE)) >+ return nsnull; > #else > chunk = js::AllocGCChunk(); > #endif > if (chunk) > mNumGCChunksInUse++; > return chunk; > } This change doesn't seem to belong in this patch. In caps/src/nsScriptSecurityManager.cpp: > // NOTE: These class and equality hook checks better match > // what IS_WRAPPER_CLASS() does in xpconnect! > >- JSEqualityOp op = >- (jsClass->flags & JSCLASS_IS_EXTENDED) ? >- reinterpret_cast<const JSExtendedClass*>(jsClass)->equality : >- nsnull; >- if (op == sXPCWrappedNativeEqualityOps) { >+ if (jsClass->ext.equality == js::Valueify(sXPCWrappedNativeEqualityOps)) { ...and in js/src/xpconnect/src/xpcprivate.h: > // NOTE!!! > // > // If this ever changes, > // nsScriptSecurityManager::doGetObjectPrincipal() *must* be updated > // also! > // > // NOTE!!! > #define IS_WRAPPER_CLASS(clazz) \ >- (((clazz)->flags & JSCLASS_IS_EXTENDED) && \ >- reinterpret_cast<JSExtendedClass*>(clazz)->equality == XPC_WN_Equality) >+ (clazz->ext.equality == js::Valueify(XPC_WN_Equality)) This looks correct, but it has me thinking: if only there were a way to put some important code in a *single* place which multiple components could then use. It would be the ultimate software engineering technique.
(In reply to comment #45) > (In reply to comment #44) > > (In reply to comment #42) > > > This is even better written > > > > > > > return (op ? op : js_LookupProperty)(cx, this, id, objp, propp); > > > > > > Why duplicate the actual parameter list? > > > > I need to check if at least GCC can optimize this properly with using jump into > > js_LookupProperty implementation. > > That not only improves the readability but also generates smaller code with > GCC. I think the original rationale for this return op ? op(...args...) : js_LookupProperty(...args...); was that the last time anyone carefully measured, the conditional branch plus direct call was faster (in the common case where op is null) than the indirect call. In other words, we knew it generated more code. We thought the larger code was faster. But I suspect this received wisdom dates back to the days when the first Java JITs were being written. Perhaps processors didn't try to predict indirect calls at the time. I imagine even the dumbest branch target predictor, running this kind of code return (op ? op : js_LookupProperty)(...args...); would predict the target to be the same as last time, and thus hit pretty often (100% of the time in benchmarks).
s/clazz/clasp/g please -- or is that wrong here? /be
Comment on attachment 459402 [details] [diff] [review] eliminating extended class v7 OK. The changes from v6 to v7 are pretty trivial, and they look fine. FWIW this line in jsapi-tests/testExtendedEq.cpp is redundant: > JSObject *global = JS_GetGlobalObject(cx); It can just be deleted; there's already a `global` member variable in scope there.
Attachment #459402 - Flags: review?(jorendorff) → review+
(In reply to comment #61) > s/clazz/clasp/g please -- or is that wrong here? prevailing custom in xpconnect is to use clazz, not clasp: ~/m/tm/js/src/xpconnect/src> grep -i clasp * | wc 32 172 2191 ~/m/tm/js/src/xpconnect/src> grep -i clazz * | wc 104 414 6981
The new version addresses the nits besides the following: > In jsapi.h: > >+ const char *name; \ > >+ uint32 flags; \ > [...] > You added some backslashes, but they aren't necessary. I am not sure what this mean as the backslashes are added to a macro definition.
Attachment #459402 - Attachment is obsolete: true
Attachment #460675 - Flags: review+
http://hg.mozilla.org/tracemonkey/rev/7b2b90efe57d
Whiteboard: fixed-in-tracemonkey
I backed out the patch - the tree was too orange before landing.
Whiteboard: fixed-in-tracemonkey
relanded - http://hg.mozilla.org/tracemonkey/rev/77eb248fa854
Whiteboard: fixed-in-tracemonkey
> > In jsapi.h: > > >+ const char *name; \ > > >+ uint32 flags; \ > > [...] > > You added some backslashes, but they aren't necessary. > > I am not sure what this mean as the backslashes are added to a macro > definition. I was talking about the backslashes in the definition of struct JSClass. I went ahead and removed them. http://hg.mozilla.org/tracemonkey/rev/85341d4c25bd
(In reply to comment #69) > I was talking about the backslashes in the definition of struct JSClass. I went > ahead and removed them. Thanks, I completely missed that.
http://hg.mozilla.org/mozilla-central/rev/7b2b90efe57d
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: