ondra zara Reporter
	Description • 14 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1 (.NET CLR 3.5.30729)
Build Identifier: 

Under certain circumstances, arguments.callee.caller is not equal to the caller function. However, the ".toSource()" values are equal/

Reproducible: Always

Steps to Reproduce:
1. Open http://tmp.zarovi.cz/ff4.html in Firefox 4
2. Note that the first alert shows "false"
3. A possible workaround is included: after assigning arbitrary property to function, the problem disappears.
Actual Results:  
false, true

Expected Results:  
true, true

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 14 years ago

Regression range on m-c:  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=38754465ffde&tochange=3079370d6597

Looks like a likely regression from bug 471214.

	Brendan Eich [:brendan] Assignee
	Comment 2 • 14 years ago

Anyone have a reduced testcase?

/be

	Jeff Walden [:Waldo]
	Comment 3 • 14 years ago

See URL?

Looks like there needs to be something approximating a method barrier introduced inside functions, to account for access to the .caller property which should end up getting the specific instantiation of the function rather than the generic one.

	Jeff Walden [:Waldo]
	Comment 4 • 14 years ago

...and that barrier needs to munge the stack.  Or something.

	Brendan Eich [:brendan] Assignee
	Comment 5 • 14 years ago

Waldo: great, reduced test at URL, missed that -- thanks.

Yeah, this should be easy to fix. Working on it now.

/be

	Brendan Eich [:brendan] Assignee
	Comment 6 • 14 years ago

This doesn't search suspended CallStacks or anything like that. Those stacks cannot be reached by .caller, AFAIK.

ES5 strict makes caller a poison pill, FWIW, too.

/be

	Brendan Eich [:brendan] Assignee
	Comment 9 • 14 years ago

v1 was a thinko, ignore.

/be

	Brendan Eich [:brendan] Assignee
	Comment 10 • 14 years ago

Comment on attachment 456650 [details] [diff] [review]
naive fix, v2

This is going to be hard. We need to identify which calls on the stack were through the Reference (in ES terms: base object, property name) that went through this method barrier.

/be

	Brendan Eich [:brendan] Assignee
	Comment 12 • 14 years ago

Comment on attachment 456650 [details] [diff] [review]
naive fix, v2

This is the fix after all. The method barrier optimizes all o.m = function (...){...}; and o = {m: function(...){...}}; cases to avoid cloning the function for identity/mutation semantics if the only uses of o.m are o.m(...) calls.

Thus the joined function cannot be in more than exactly one property. Therefore any calls on the stack referring to it as callee (argv[-2], got and set here so I did not obscure its location with fp->calleeObject() for the get -- could add an fp->setCalleeObject though) must be from calls to o.m, not any other property, or a clone created by extracting the method, e.g., f = o.m; f(...).

Whew! I will comment and consider setCalleeObject in a bit, wanted to revive this and get Waldo's review.

/be

	Brendan Eich [:brendan] Assignee
	Comment 13 • 14 years ago

(In reply to comment #12)
> Thus the joined function cannot be in more than exactly one property. Therefore
> any calls on the stack referring to it as callee (argv[-2], got and set here so
> I did not obscure its location with fp->calleeObject() for the get -- could add
> an fp->setCalleeObject though) must be from calls to o.m, not any other
> property, or a clone created by extracting the method, e.g., f = o.m; f(...).

Forgot to add (or clarify -- pre-caffeine here, sorry!) that if an activation of the function was via f = o.m; f(...) then there'd already be a clone due to the method barrier being crossed at f = o.m, and no need or way to go through the barrier patched here again.

/be

	Jeff Walden [:Waldo]
	Comment 15 • 14 years ago

Comment on attachment 456697 [details] [diff] [review]
complete fix, with info-hiding and testsuite addition

Introducing a stackwalk into every method barrier can't be good.  But maybe it's not that bad.  Color me skeptical; I'd want perf numbers demonstrating it's not bad before granting review, but...

I alluded to the below behavior (seen with your patch applied) in comment 3, it seems overly opaquely:

[jwalden@find-waldo-now tests]$ ../dbg/js -j
js> var o = { f: function() { return o.g(); }, g: function() { return arguments.callee.caller; } };
js> var c = o.f();
js> c
(function () {return o.g();})
js> o.f
(function () {return o.g();})
js> c === o.f
false

Your patch appears to fix the barrier occurring via generic property lookup, but it doesn't fix it occurring through fun.caller, which the above example triggers.

	Brendan Eich [:brendan] Assignee
	Comment 16 • 14 years ago

Waldo, thanks for the great review and testcase -- comment 10 was right after all. Patch in a sec.

/be

	Brendan Eich [:brendan] Assignee
	Comment 17 • 14 years ago

Method extraction is rare, stacks are shallow. Stats in a bit, have to head home.

/be

	Brendan Eich [:brendan] Assignee
	Comment 18 • 14 years ago

No method read barrier crossings (method extraction, get-not-call) in SunSpider.

v8 has 175 hits (testing the old copy of v8 benchmarks under js/src -- if this copy is as old and musty as it seems, I'll redo tomorrow on the latest v8 suite).

/be

	Brendan Eich [:brendan] Assignee
	Comment 19 • 14 years ago

Update: closing on perf wins to balance the stack walk. It indeed hurts v8, which extracts methods in several ways, one hard to optimize away:

Object.extend = function(destination, source) {
  for (var property in source) {
    destination[property] = source[property];
  }
  return destination;
}

Without a way to chain joined function object references together for patching when unjoining later due to mutation or extraction, the read of a method via source[property] must clone (unjoin).

/be

	Brendan Eich [:brendan] Assignee
	Comment 20 • 14 years ago

This was a wild ride, including the mq rebase on top of the fatvals landing. More when I've slept, it passes trace-tests and jsreftests.

/be

	Brendan Eich [:brendan] Assignee
	Comment 21 • 14 years ago

The perf win is due to recognizing more cases where a null closure lambda is used in a way that can't be observed to differ if the implementation joins function objects (as ES3 tried to allow in observable situations too; ES5 got rid of this mistake), namely as a parameter to the original String.prototype.replace native, or similarly for Array.prototype.sort's comparator funarg, or for the "module pattern" lambda-immediately-applied case.

/be

	Brendan Eich [:brendan] Assignee
	Comment 22 • 14 years ago

Same as last patch but with better parallel structure between interp and tracer, and a warning comment about keeping it that way. Zzzz...

/be

	Brendan Eich [:brendan] Assignee
	Comment 24 • 14 years ago

/tmp/function.stats file for SunSpider, run interp-only to get complete states (we don't generate trace-JITted code to update rt->functionMeter):

function meter (t/string-validate-input.js):
allfun              232
heavy               9
nofreeupvar         56
onlyfreevar         127
display             0
flat                5
setupvar            0
badfunarg           35
joinedsetmethod     28
joinedinitmethod    0
joinedreplace       170
joinedsort          1
joinedmodulepat     4
mreadbarrier        0
mwritebarrier       0
mwslotbarrier       5
unjoined            12

method read barrier count map:

unjoined function count map:
         2 t/string-unpack-code.js:66
         2 t/string-unpack-code.js:51
         2 t/string-unpack-code.js:30
         2 t/string-unpack-code.js:16
         1 t/string-tagcloud.js:40
         1 t/string-tagcloud.js:229
         1 t/date-format-xparb.js:45
         1 t/date-format-xparb.js:45

I manually sort -nr'ed the bottom indented lines of hashmap dump. This is a win over the stats without the patch fully enabled, particularly for string.replace calls in crypto-aes.js:

12,14c12,14
< joinedreplace       0
< joinedsort          0
< joinedmodulepat     0
---
> joinedreplace       170
> joinedsort          1
> joinedmodulepat     4
18c18
< unjoined            187
---
> unjoined            12
23,25d22
<         85 t/crypto-aes.js:283
<         85 t/crypto-aes.js:279
<          4 t/access-nbody.js:157
31d27
<          1 t/string-tagcloud.js:235

Still, SS timing is so noisy even on my MBP with SSD and all apps shut down that I see only a marginal win. More like 1.5% on v8, details next.

/be

	Jeff Walden [:Waldo]
	Comment 25 • 14 years ago

Comment on attachment 458220 [details] [diff] [review]
no stack walk in method read barrier, perf win on v8 (1-2%), + comment fixes

>diff --git a/js/src/jsarray.h b/js/src/jsarray.h

>+/* JSFastNative entry point for Array.prototype.sort. */
>+extern JSBool
>+js_array_sort(JSContext *cx, uintN argc, js::Value *vp);

Is the comment necessary?  Seems obvious enough to me, accords with normal tradition for other extern'd built-in function native-code definitions, seems unnecessary.

Minor tweak: this should be js::array_sort.


>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp

>+                    } else if (sprop->hasSlot()) {
>+                        Value v = thisp->getSlot(sprop->slot);
>+                        JSObject *clone;
>+
>+                        if (IsFunctionObject(v, &clone) && GET_FUNCTION_PRIVATE(cx, clone) == fun) {
>+                            JS_ASSERT(clone != funobj);
>+                            *vp = v;
>+                            setCalleeObject(*clone);
>+                        }
>+                    }

This code looks mostly okay, but this bit raises my hackles.  In particular: is it possible to change the value stored in the slot to *another* clone of the primordial function here?  Initially this should be fine, because it'll look up the original clone.  But if I get ahold of a completely different clone, and I overwrite this value somehow, I will get that different clone back -- not the one I really should get (because that different clone, and the real one I should get, will both have the same private JSFunction*).  I tried for awhile to make a testcase to cause this to happen, but I wasn't successful.  Maybe it's not possible, but it is not immediately obvious to me that this is the case.  Perhaps you (or jorendorff, to whom I've forwarded a r? to get a second mind on this) can explain why my worries are unfounded?


>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp

>+                    if (op2 == JSOP_CALL) {
>+                        /*
>+                         * Array.prototype.sort and String.prototype.replace are
>+                         * optimized as if they are special form. We know that they
>+                         * won't leak the joined function object in obj, therefore
>+                         * we don't need to clone that compiler- created function
>+                         * object for identity/mutation reasons.
>+                         */

Sigh, a characteristically opaque optimization that nevertheless is claimed to improve performance (haven't tested, quite believable that it does).

I'm not sure whether I believe the optimization is worth the complexity.  I'm sort of leaning toward resigned acceptance, given that it does show overall gains, and it's not incredibly inscrutable when you think about it.  But it does require that double-(triple?)-take, and it makes our property access strategy even more special-cased.  If jorendorff can live with it, I guess that'll settle it from my point of view.

I assume profiling or somesuch pointed out these two optimization opportunities -- have you considered extending this to the array extras as well?  filter/forEach/every/some/map/reduce/reduceRight could all benefit from this optimization as well, although to be sure they're -- currently -- less used on the web.


>+                        int iargc = GET_ARGC(pc2);
>+
>+                        /*
>+                         * Note that we have not pushed obj as the final argument
>+                         * yet, so regs.sp[-iargc - 1], not regs.sp[-iargc - 2], is
>+                         * the callee for this JSOP_CALL.
>+                         */
>+                        JSFunction *calleeFun =
>+                            GET_FUNCTION_PRIVATE(cx, &regs.sp[-iargc - 1].toObject());

I would prefer [+1 - iargc - 2] to make the necessarily-different adjustment more understandable (perhaps without the +, I'm uncertain how much it improves clarity).


>diff --git a/js/src/jsobj.h b/js/src/jsobj.h

>+inline bool JSObject::isObject() const { return getClass() == &js_ObjectClass; }

I could go for isPlainObject, perhaps, here.  The current name just makes me burst out laughing -- but I can't think of a sufficiently better one to strongly request it.


>diff --git a/js/src/jsobjinlines.h b/js/src/jsobjinlines.h

> inline bool
>+JSObject::canHaveMethodBarrier() const
>+{
>+    return isObject() || isFunction() || isPrimitive() || isDate();

isRegExp() too, or no?


r=me as much as I can, although I am not completely convinced on the clone-confusion point -- hopefully others can correct or elaborate upon the point.

	Brendan Eich [:brendan] Assignee
	Comment 26 • 14 years ago

Good point on js::array_sort (same for str_replace), minimizes the diff. I got rid of that useless one line comment (I think I was gonna say why array_sort is exported, but that's for jsarray.h to comment on).

Re: [+1 - iargc - 2], I tried that and [(-iargc - 2) + 1] but neither was an improvement, since the reader has to do the CSE, or else grok the -argc - 2 based on (JSOP_CALL's case):

    vp = regs.sp - (argc + 2);

This suggests 1 - (argc + 2) vs. 0 - (argc + 2), which I've used elsewhere. But again readers who do CSE in their heads will have to waste time doing that and checking number-line relations. Still, I'd rather repeat (argc + 2) if it helps. What do you think?

Great catch on the "wrong clone" issue (you can't trust clones ;-). I will work on that and put up a better patch. Thanks,

/be

	Jeff Walden [:Waldo]
	Comment 27 • 14 years ago

(In reply to comment #26)
> This suggests 1 - (argc + 2) vs. 0 - (argc + 2), which I've used elsewhere.

1 - (argc + 2) seems better, yes -- I like that the most of the mooted possibilities.

	Brendan Eich [:brendan] Assignee
	Comment 28 • 14 years ago

I didn't optimize null closure lambdas passed to the Array extras; they aren't used in benchmarketing yet and this bug's patch is big enough. I'll file another bug on addressing them either specifically or (ideally) generally.

See the revised testcase for case analysis and coverage. Thanks again for the great review. If you could take one more look, I bet you'd be convinced without needing to summon jorendorff.

/be

	Jeff Walden [:Waldo]
	Comment 29 • 14 years ago

Comment on attachment 458922 [details] [diff] [review]
revised per review comments

Yeah, I think I'm good with this now.  Maybe in several years we might be able to remove fun.caller and a lot of this convolution...

	Brendan Eich [:brendan] Assignee
	Comment 30 • 14 years ago

(In reply to comment #29)
> Comment on attachment 458922 [details] [diff] [review]
> revised per review comments
> 
> Yeah, I think I'm good with this now.  Maybe in several years we might be able
> to remove fun.caller and a lot of this convolution...

That would be great -- ES5 strict will help to the extent that it's used.

/be

	Brendan Eich [:brendan] Assignee
	Comment 31 • 14 years ago

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

/be

	Brendan Eich [:brendan] Assignee
	Comment 32 • 14 years ago

Out again:

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

Leak-o-rama, I suspect due to JSSLOT_FUN_METHOD_OBJ entraining the object owning the joined function object.

/be

	Brendan Eich [:brendan] Assignee
	Comment 33 • 14 years ago

Of course the patch breaks debug leak tests: the JS_FUNCTION_METERING stuff marks functions entered into methodReadBarrierCountMap and unjoinedFunctionCountMap. The envar testing does not prevent this, it controls only whether we dump these maps. Waldo, I will tryserver a fix and if it is good, put it up for review. You around this weekend?

/be

	Jeff Walden [:Waldo]
	Comment 34 • 14 years ago

Later in the day tomorrow, yes -- after 17:00 or so.

	Brendan Eich [:brendan] Assignee
	Comment 36 • 14 years ago

The if (!fun) early return in JSStackFrame::getValidCalleeObject was badly translated from the FUN_CALLER: case in fun_getProperty.

I did not rename JSStackFrame::getValidCalleeObject to getValidCalleeValue, which has one too many "Val*" words -- instead I commented both jsinterp.h and jsdbgapi.h.

If you have a better name, lay it on me. Thanks,

/be

	Brendan Eich [:brendan] Assignee
	Comment 37 • 14 years ago

Pre-caffeine me thought there was an argv test in calleeValue, as there is in callee. These JSStackFrame methods need at least naming help (use of maybe for non-asserting null-checking methods, e.g. callee -> maybeCallee, could help) but I will leave that for another bug.

/be

	Brendan Eich [:brendan] Assignee
	Comment 38 • 14 years ago

Tryserver victory (brendan@mozilla.com Sun Jul 25 14:12:10 2010 PDT), modulo failed builds due to insufficient disk space and some flappy-looking orangeness.

/be

	Jeff Walden [:Waldo]
	Comment 39 • 14 years ago

...plus removing the JS_FUNCTION_METERING #define and just using DEBUG directly.

	Brendan Eich [:brendan] Assignee
	Comment 40 • 14 years ago

http://hg.mozilla.org/tracemonkey/rev/80382d88b92c

/be

	Brendan Eich [:brendan] Assignee
	Comment 41 • 14 years ago

Looks like this landed in m-c:

http://hg.mozilla.org/mozilla-central/log?rev=577648

/be

	ivan
	Comment 42 • 14 years ago

I think this is still buggy. See:

http://ludios.net/browser_bugs/ff4_arguments_callee_bug.html

I see `false' in the nightly I just tried, Mozilla/5.0 (Windows NT 5.1; rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre

It's `true` everywhere else. I hope I'm not misinterpreting things.

	Brendan Eich [:brendan] Assignee
	Comment 43 • 14 years ago

(In reply to comment #42)
> I think this is still buggy. See:
> 
> http://ludios.net/browser_bugs/ff4_arguments_callee_bug.html
> 
> I see `false' in the nightly I just tried, Mozilla/5.0 (Windows NT 5.1;
> rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre
> 
> It's `true` everywhere else. I hope I'm not misinterpreting things.

You are not -- thanks. Please file a new bug, one patch per bug. Your test shows the prototype chain in action, where the method barrier is on the proto of the |this| object. Should be easy to fix, I'll take the bug.

I don't believe bug 586453 (also filed today) is related, as I can't reduce it to a shell testcase and it seems not to involve any joined function objects (from "null closure" lambda expressions assigned to properties in assignment statements or object initialisers).

/be

	ivan
	Comment 44 • 14 years ago

Thanks, filed bug 586482