514981 - JSStackFrame::sharp{Array,Depth} should be locals allocated due to #n[#=] usage

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Brendan Eich [:brendan]]

The compiler sees all -- we need some script flags (bits are free) to indicate to js_Execute that the caller's sharpArray should be propagated, for eval and such.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: patch in progress

Nearly there:

js> function f(){return #1=[#1#]}
js> a = f()

js> uneval(a)
#1=[#1#]
js> 

I moved the sharpArray and sharpDepth members to the stack, and for local vars in functions named them (unnameable in JS source) "#array" and "#depth" (making the sharp variable user pay re-atomizing costs instead of pinning these bogo-names).

The following shows the remaining known bug:

js> function g(s){return #1=[eval(s)]}
js> g("#1#")
Assertion failure: script->nfixed == 0, at ../jsinterp.cpp:1545

This shows how sharp variables work across eval. An old bug I can fix now is that they'll be forgotten after the first object or array initializer in the eval'ed program closes (see JSOP_ENDINIT).

Igor, can you remind me why nfixed must be 0 for eval and debugger equivalents? Thanks,

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: patch in progress with js_Atomize OOM handling

Comment 3

[Brendan Eich [:brendan]]

Another note for review: JSOP_SHARPINIT now contains the fp->sharpDepth++; and if (--fp->sharpDepth) fp->sharpArray = NULL; code from JSOP_NEWINIT and JSOP_ENDINIT respectively. This seemed the best course to avoiding overhead in those opcodes' cases while minimizing added ops.

/be

Comment 4

[Igor Bukanov]

(In reply to comment #1)
> Igor, can you remind me why nfixed must be 0 for eval and debugger equivalents?

IIRC this just reflects the fact that eval scripts do not have gvar optimization nor JSOP_REGEXP bytecodes. I suppose that could be lifted.

Comment 5

[Brendan Eich [:brendan]]

Attachment: proposed fix

Passes js/tests and trace-tests.

/be

Comment 6

[Jeff Walden [:Waldo]]

We do have another option beyond this simplification; I trust everyone here knows what that option is.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> We do have another option beyond this simplification;

This is hardly a simplification. It's tolerable for now, barely.

> I trust everyone here
> knows what that option is.

This comment is smarmy. We're not removing sharp objects or uneval/toSource for 3.6. We can have an adult conversation without passive/aggressive b.s. in a different forum.

/be

Comment 8

[Jeff Walden [:Waldo]]

(In reply to comment #7)
> This comment is smarmy.

Wasn't intended to be anything but a restrained reminder, apologies if it was interpreted otherwise.

Comment 9

[Brendan Eich [:brendan]]

http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_frm/thread/a3cfd0ba214d8dd5#

/be

Comment 10

[Brendan Eich [:brendan]]

We remove extensions lightly at our peril. Without a principled and data-driven way to proceed, we'll be a standards-conforming, slower v8 or nitro.

Let's keep what is good, improve what needs improving, and work to lead the standard process as we have. Getters, setters, Array extras, and much of the Harmony wanted list come from Mozilla. We should not drop the ball, even as we play nice with others including MSFT (whose ex-Smalltalk guru, Allen Wirfs-Brock, did the defineProperty etc. heavy lifting).

/be

Comment 11

[Brendan Eich [:brendan]]

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

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/c19b0d06d076