Closed Bug 515000 Opened 10 years ago Closed 10 years ago

JSTempValueRooter JSTVU_SINGLE over-constrains GC-thing against static allocation

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: brendan, Assigned: brendan)

References

Details

(Keywords: regression, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

Attached patch proposed fixSplinter Review
This cleans up a few places that used void* unnecessarily, but otherwise it is minimally focused on eliminating the "string" variant of JSTempValueUnion.

/be
Attachment #399033 - Flags: review?(igor)
Attachment #399033 - Flags: review?(gal)
Gary's fuzzer-generated testcase from bug 514819 comment 26:

(function () {
    gczeal(1);
    eval("<x/>")
})()

/be
Flags: in-testsuite?
Status: NEW → ASSIGNED
Comment on attachment 399033 [details] [diff] [review]
proposed fix

Not a big fan of masquerading arbitrary pointers as object jsvals. I would sleep sounder if we stop doing that.
Attachment #399033 - Flags: review?(gal) → review+
(In reply to comment #3)
> (From update of attachment 399033 [details] [diff] [review])
> Not a big fan of masquerading arbitrary pointers as object jsvals.

Arbitrary GC-thing pointers. Now a bit less arbitrary with the exclusion of JSString* via proper jsval tagging.

> I would sleep sounder if we stop doing that.

Roger that, fodder for later bugs. Thanks for the r+, I will land.

Igor, I'm still interested in your review too.

/be
http://hg.mozilla.org/tracemonkey/rev/36b3eefa756a

/be
Keywords: regression
Whiteboard: fixed-in-tracemonkey
I think the patch should just ditch JSTVU_SINGLE and gcthing in JSTempValueRooterand replace them with JSTVU_OBJECT, JSTVU_STRINBG and JSTVU_JSVAl without "masquerading arbitrary pointers as object". 

But I really do not understand the need to fix tvr at all. What should have been fixed instead is js_GetGCThingTraceKind. That function should check for statically allocated things and return JSTRACE_STRING the same way it returns JSTRCE_DOUBLE using explicit check for double strings.

That would fix this and still existing bug with js_EnterLocalRootScope that relies on js_GetGCThingTraceKind doing the right thing.
(In reply to comment #6)
> But I really do not understand the need to fix tvr at all. What should have
> been fixed instead is js_GetGCThingTraceKind. That function should check for
> statically allocated things and return JSTRACE_STRING the same way it returns
> JSTRCE_DOUBLE using explicit check for double strings.

That puts JSString::isStatic on the potentially hot path of js_GetGCThingTraceKind called from js_CallValueTracerIfGCThing. If we don't need to call JSString::isStatic we should avoid it.

> That would fix this and still existing bug with js_EnterLocalRootScope that
> relies on js_GetGCThingTraceKind doing the right thing.

There's no bug here: js_PushLocalRoot is never called with a string that could be static punned as a jsval.

/be
(In reply to comment #5)
> http://hg.mozilla.org/tracemonkey/rev/36b3eefa756a
> 
> /be

This just blew the fuzzers with:

for(x in((x for(y in[]))(gczeal(2))))
[]

Crash [@ js_GetGCThingTraceKind] at 0x00000ff0 without -j on debug on TM tip. autoBisect indicates this bug is probably related.

The first bad revision is:
changeset:   32166:36b3eefa756a
user:        Brendan Eich <brendan@mozilla.org>
date:        Mon Sep 07 00:35:27 2009 -0700
summary:     JSTempValueRooter JSTVU_SINGLE over-constrains GC-thing against static allocation  (515000, r=gal).
Easily fixed, the problem is the "null string reference", 0x4, created by my unconditional STRING_TO_JSVAL(str) in JS_PUSH_TEMP_ROOT_STRING.

/be
Igor, we've been sharking and finding hard-to-predict branches to handle odd cases on hot paths cost us, sometimes a little but not always, and they add up. It may be that a JSString::isStatic call from js_GetGCThingTraceKind wouldn't hurt but the approach here asserts and analyzes callers to minimize the cost.

If you think we should consolidate the check, I can do that, but at this point in a followup bug.

I'm going to fix the null tagged as a string problem that Gary found immediately.

/be
Followup to handle null string tvr refs:

http://hg.mozilla.org/tracemonkey/rev/f462ab3ef84a

/be
r=me for #11
If the JSString::isStatic check becomes a performance issue, I could also combine the intString and unitString array. This should reduce the cost for the check.
#13: its more of a design issue. Don't load up hot paths with special case code. Even if its fast special case code. But we should measure some of this. I am not sure we really know we are looking at a problem here.
(In reply to comment #10)
> Igor, we've been sharking and finding hard-to-predict branches to handle odd
> cases on hot paths cost us, sometimes a little but not always, and they add up.

Yet this is exactly what the landed patch did. It loads tvr code that is executed on each string rooting with extra bit manipulations to store JSString * as jsval instead of simply adding JSTVU_STRING. On the other hand typically js_GetGCThingTraceKind is not called during a typical GC run since only a handful tvr may exists on all native stacks. Also XPConnect uses the function from the cycle collector when it performance (unfortunately due to the complexity of the rest of the code) does not matter and when the branch can be predicted since the cycle collector never passes strings to js_GetGCThingTraceKind.

> It may be that a JSString::isStatic call from js_GetGCThingTraceKind wouldn't
> hurt but the approach here asserts and analyzes callers to minimize the cost.

I do not care about the performance if js_GetGCThingTraceKind is broken. Has anybody monitored all the code paths that leads to calls js_GetGCThingTraceKind so we know for sure that statically-allocated strings are not passed there? Since the answer to the latter question is no (JS_MarkGCThing is still available and E4X still indirectly calls the js_GetGCThingTraceKind), then we have a bad bug here.

> 
> If you think we should consolidate the check, I can do that, but at this point
> in a followup bug.

See bug 515199.
I agree with Igor here. We have no strong indication that GetGCThingTraceKind is a hotspot. Until we have measured that, I would prefer the check there.
(In reply to comment #16)
> I agree with Igor here. We have no strong indication that GetGCThingTraceKind
> is a hotspot. Until we have measured that, I would prefer the check there.

Reviewer flip-flop :-P.

Ok, let's clean it up in the other bug. Thanks for filing, Igor. Do you want me to do the work?

/be
http://hg.mozilla.org/mozilla-central/rev/36b3eefa756a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #399033 - Flags: review?(igor)
You need to log in before you can comment on or make changes to this bug.