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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({fixed1.9.1})

unspecified
fixed1.9.1
Points:
---
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

v3
18.49 KB, patch
brendan
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

9 years ago
Created attachment 380108 [details] [diff] [review]
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.
Attachment #380108 - Flags: review?(brendan)
Comment on attachment 380108 [details] [diff] [review]
v1

Argh, sorry about that.

Why not use C++ inline members?

/be
(Assignee)

Comment 3

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

Comment 4

9 years ago
Created attachment 380144 [details] [diff] [review]
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.
Attachment #380108 - Attachment is obsolete: true
Attachment #380144 - Flags: review?(brendan)
Attachment #380108 - Flags: review?(brendan)
(Assignee)

Comment 5

9 years ago
Comment on attachment 380144 [details] [diff] [review]
v2

The patch has wrong changes in call_enumerate.
Attachment #380144 - Attachment is obsolete: true
Attachment #380144 - Flags: review?(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
(Assignee)

Comment 7

9 years ago
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.
Attachment #380144 - Attachment is obsolete: false
Attachment #380144 - Flags: review?(brendan)
(Assignee)

Comment 8

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

Comment 9

9 years ago
Created attachment 380156 [details] [diff] [review]
v3

The new version uses countArgsAndVars.
Attachment #380144 - Attachment is obsolete: true
Attachment #380156 - Flags: review?(brendan)
Attachment #380144 - Flags: review?(brendan)
Comment on attachment 380156 [details] [diff] [review]
v3

Great, thanks!

/be
Attachment #380156 - Flags: review?(brendan) → review+
Flags: wanted1.9.1?
(Assignee)

Comment 11

9 years ago
http://hg.mozilla.org/tracemonkey/rev/45b0e1a66ac8
Whiteboard: fixed-in-tracemonkey

Comment 12

9 years ago
http://hg.mozilla.org/mozilla-central/rev/45b0e1a66ac8
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: wanted1.9.1? → wanted1.9.1+
Resolution: --- → FIXED

Comment 13

9 years ago
shaver points out that this patch is pretty big. Do we really want this on the branch?
Flags: wanted1.9.1+ → wanted1.9.1-

Updated

9 years ago
Flags: wanted1.9.1- → wanted1.9.1?
(Assignee)

Comment 14

9 years ago
(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.
Shaver can read through the filigree, I'm sure. This is not a scary patch.

/be
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?
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
(Assignee)

Comment 18

9 years ago
(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

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/689fa78bb546
Flags: wanted1.9.1? → wanted1.9.1+

Updated

9 years ago
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.