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

RESOLVED FIXED in mozilla1.9.2

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

Trunk
mozilla1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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
(Assignee)

Comment 1

8 years ago
Created attachment 399418 [details] [diff] [review]
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
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #399418 - Flags: review?(igor)
(Assignee)

Comment 2

8 years ago
Created attachment 399419 [details] [diff] [review]
patch in progress with js_Atomize OOM handling
Attachment #399418 - Attachment is obsolete: true
Attachment #399419 - Flags: review?(igor)
Attachment #399418 - Flags: review?(igor)
(Assignee)

Comment 3

8 years ago
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

8 years ago
(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.
(Assignee)

Comment 5

8 years ago
Created attachment 399513 [details] [diff] [review]
proposed fix

Passes js/tests and trace-tests.

/be
Attachment #399419 - Attachment is obsolete: true
Attachment #399513 - Flags: review?(igor)
Attachment #399419 - Flags: review?(igor)
We do have another option beyond this simplification; I trust everyone here knows what that option is.
(Assignee)

Comment 7

8 years ago
(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
(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.
(Assignee)

Comment 9

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

/be
(Assignee)

Comment 10

8 years ago
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

Updated

8 years ago
Attachment #399513 - Flags: review?(igor) → review+
(Assignee)

Comment 11

8 years ago
http://hg.mozilla.org/tracemonkey/rev/c19b0d06d076

/be
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

8 years ago
Depends on: 515815
Depends on: 515892
Depends on: 515957
Depends on: 516262
http://hg.mozilla.org/mozilla-central/rev/c19b0d06d076
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 517076
Depends on: 517077

Updated

8 years ago
Flags: blocking1.9.2?

Updated

8 years ago
Flags: blocking1.9.2? → blocking1.9.2-
Blocks: 536275

Updated

8 years ago
Blocks: 557378
Depends on: 561011
Depends on: 561383
You need to log in before you can comment on or make changes to this bug.