495216 - call object should use argc + nvars, not argc + nvars + nupvars

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Currently to expose arguments and variables the Call object uses JS_GET_LOCAL_NAME_COUNT. But that returns argc + nvars + nupvars and the call object never exposes any upvars. In normal cases any upvar-containing closure would never have a Call object created for it. But if such object is created, for example, due to debugger activities, then JS_GET_LOCAL_NAME_COUNT would waste space for the object.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch replaces JS_UPVAR_LOCAL_NAME_START and JS_GET_LOCAL_NAME_COUNT with inline functions js_ArgVarCount and js_LocalNameCount. To fix this bug the patch uses js_ArgVarCount, not js_LocalNameCount, when dealing with the Call object.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 380108 [details] [diff] [review]
v1

Argh, sorry about that.

Why not use C++ inline members?

/be

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> (From update of attachment 380108 [details] [diff] [review])
> Argh, sorry about that.
> 
> Why not use C++ inline members?

Right, and I can stand Liveconnect-induced preprocessor checks for C++...

Comment 4

[Igor Bukanov]

Attachment: v2

The new version uses member functions, countLocalNames and countArgsVars. As the result of this the patch has to move JSFunction below FUN_INTERPRETED etc. macros so the member functions can use the macros for assertions.

Comment 5

[Igor Bukanov]

Comment on attachment 380144 [details] [diff] [review]
v2

The patch has wrong changes in call_enumerate.

Comment 6

[Brendan Eich [:brendan]]

s/countArgsVars/countArgsAndVars/

Might prefer argAndVarCount for infallible getter name (noun phrase, no verb to fail). But it reads better to have countArgsAndVars.

/be

Comment 7

[Igor Bukanov]

Comment on attachment 380144 [details] [diff] [review]
v2

The previous comments on call_enumerate was wrong and its is OK to assume in call_enumerate that the property always exists. I was thinking about duped arg names, but call_enumerate checks for them before doing the lookup.

Comment 8

[Igor Bukanov]

(In reply to comment #6)
> Might prefer argAndVarCount for infallible getter name (noun phrase, no verb to
> fail). But it reads better to have countArgsAndVars.

I have started with argAndVarCount. But the difference in the case of arg and Var made it look ugly compared with symmetry of countArgsVars (or countArgsAndVars). Plus this is not a pure getter as the member does some computations with function fields.

Comment 9

[Igor Bukanov]

Attachment: v3

The new version uses countArgsAndVars.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 380156 [details] [diff] [review]
v3

Great, thanks!

/be

Comment 11

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/45b0e1a66ac8

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/45b0e1a66ac8

Comment 13

[Robert Sayre]

shaver points out that this patch is pretty big. Do we really want this on the branch?

Comment 14

[Igor Bukanov]

(In reply to comment #13)
> shaver points out that this patch is pretty big. Do we really want this on the
> branch?

The essence of the patch is few lines of changes in jsfun.cpp. The rest is pretty mechanical rename of macro into member function that compiler verifies.

Comment 15

[Brendan Eich [:brendan]]

Shaver can read through the filigree, I'm sure. This is not a scary patch.

/be

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I saw through the filigree of the shared-array-map patch too, and while that patch was fine it exposed a latent bug that then blocked release.  If we don't need this on 1.9.1, and I don't think we do, then we should not take it right now, IMO.  We are past the time for opportunistic code cleanup and microoptimizations in FF3.5, no?

Comment 17

[Brendan Eich [:brendan]]

The risk here is that by shrinking Call object reserved slots to exclude upvars dead space, we invite a latent bug to bite that is currently harmlessly reading or writing that dead space. Ignoring wild refs, this is not possible (inspection).

The source skew hurts too, hard to weigh.

/be

Comment 18

[Igor Bukanov]

(In reply to comment #16)

> I saw through the filigree of the shared-array-map patch too, and while that
> patch was fine it exposed a latent bug that then blocked release.

It was *good* that the patch exposed that bug. It was not latent at all, it just happen that without shared-array-map the code has not crashed. But with a right setup it quite likely will.

> We are past the time for opportunistic code cleanup and microoptimizations in FF3.5, no?

This bug fixes a regression resulting in extra memory usage coming from upvar2 patch. It could be fixed using a tiny patch, but the mechanical renames gave very good way to verify that other places has not misused counting of local names.

If this patch is not going into 1.9.1 I suggest to back it out on the trunk and I will make just a minimal one as not having the renames on 1.9.1 means greater back porting cost for 1.9.1 bugs.

In general IMO various clenups and renames should not go into the trunk unless they go to 1.9.1 as well for at least few dot releases of FF 3.5 exactly for the above reason - the cost of merge conflicts when backporting fixes is rather non-trivial.

Comment 19

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/689fa78bb546