Closed Bug 454035 Opened 16 years ago Closed 16 years ago

Avoid needless prototype-shape purges

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: brendan, Assigned: brendan)

References

Details

Attachments

(1 file)

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
Priority: -- → P1
Attached patch proposed fixSplinter Review
I swear I attached this already...

/be
Attachment #337477 - Flags: review?(igor)
(Patch is against tracemonkey tip as of yesterday, should apply to m-c too.)

/be
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.
(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
Attachment #337477 - Flags: review?(igor) → review+
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
Flags: in-testsuite-
Flags: in-litmus-
Depends on: 480579
Depends on: 481516
Depends on: 479198
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: