Closed
Bug 454035
Opened 16 years ago
Closed 16 years ago
Avoid needless prototype-shape purges
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b1
People
(Reporter: brendan, Assigned: brendan)
References
Details
Attachments
(1 file)
18.85 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
V8/raytrace.js among others shows a lot of "cache miss" aborts from TraceMonkey's JSOP_SETPROP record method, because it has default-valued prototype properties that are always shadowed in the constructor -- in its Prototype.js-hooked initialize method actually. The general problem motivating shape regeneration in a scope or prototype object is A -> B -> C where there may be many A values, there are cache entries keyed by the A objects to a C property x, and then someone shadows B.x. We can't find all the A, A', A'', etc. keyed entries cheaply, so we assign a new shape to C, thus guaranteeing a miss on any of the A entries. But for newborn objects in constructors, and a great many objects in general, there can be no other object acting in the role of A, delegating to the newborn B object. So by flagging objects that are delegates (either on scope or prototype chain), we can optimize PurgeScopeChain. To flag directly in the object, I turned fslots[JSSLOT_CLASS] into a leading member, classword. There's no point in storing clasp with the private JSVAL_INT bit wasted (set) since the GC skips that slot. It really shouldn't be scanned. Igor, hoping you can give this quick review. More TraceMonkey work is built on it. I may land it in the tracemonkey repo before landing it in mozilla-central, but I won't land it anywhere before I get r+. /be
Assignee | ||
Updated•16 years ago
|
Priority: -- → P1
Assignee | ||
Comment 1•16 years ago
|
||
I swear I attached this already... /be
Attachment #337477 -
Flags: review?(igor)
Assignee | ||
Comment 2•16 years ago
|
||
(Patch is against tracemonkey tip as of yesterday, should apply to m-c too.) /be
Comment 3•16 years ago
|
||
Comment on attachment 337477 [details] [diff] [review] proposed fix >- entry = &JS_PROPERTY_CACHE(cx) >- .table[PROPERTY_CACHE_HASH_PC(pc, OBJ_SCOPE(obj)->shape)]; >+ entry = &JS_PROPERTY_CACHE(cx).table[PROPERTY_CACHE_HASH_PC(pc, OBJ_SHAPE(obj))]; I guess 80-column limit is thing of the past. Too bad that with even 1280 pixels I cannot have 2 source windows side-by-side with a readable font... > struct JSObject { > JSObjectMap *map; >+ jsuword classword; The delegate bit is required for the proto slot. So why not store it there with 2 more jsuword protoword and parentword members to replace the indexed slots? The patch touches the slot indexing in any case. Such change would ensure that the patch covers all the cases. I will finish the review tomorrow.
Assignee | ||
Comment 4•16 years ago
|
||
(In reply to comment #3) > (From update of attachment 337477 [details] [diff] [review]) > >- entry = &JS_PROPERTY_CACHE(cx) > >- .table[PROPERTY_CACHE_HASH_PC(pc, OBJ_SCOPE(obj)->shape)]; > >+ entry = &JS_PROPERTY_CACHE(cx).table[PROPERTY_CACHE_HASH_PC(pc, OBJ_SHAPE(obj))]; > > I guess 80-column limit is thing of the past. Too bad that with even 1280 > pixels I cannot have 2 source windows side-by-side with a readable font... Hmm, can we get you a bigger monitor? I'm old-school but find that (except for comments) the tw=99 is helping readability. We should talk more on IRC or mail about it if it's an issue. > > struct JSObject { > > JSObjectMap *map; > >+ jsuword classword; > > The delegate bit is required for the proto slot. And parent. > So why not store it there with > 2 more jsuword protoword and parentword members to replace the indexed slots? Because that will deoptimize the frequently-accessed proto and parent slots as well as require special GC scanning of them, instead of just letting them be scanned as jsvals. > The patch touches the slot indexing in any case. Such change would ensure that > the patch covers all the cases. The macrology is enough, I think. I'll double-check and put up a new patch if necessary. Thanks, /be
Updated•16 years ago
|
Attachment #337477 -
Flags: review?(igor) → review+
Assignee | ||
Comment 5•16 years ago
|
||
Fixed: http://hg.mozilla.org/tracemonkey/rev/1e8705a50501 http://hg.mozilla.org/mozilla-central/rev/c008abb140b8 /be
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
You need to log in
before you can comment on or make changes to this bug.
Description
•