SM: eliminate JSStackFrame.(nvars|vars)

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({perf})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

10 years ago
Currently JSStackFrame.nvars duplicates the data available either through JSStackFrame.fun->u.i.nvars or through JSStackFrame.script->ngvars + JS_SCRIPT_REGEXPS(JSStackFrame.)->length. Since the field is not accessed at the performance-critical paths, I suggest to eliminate it replacing its with the value computed from the script and fun fields.
(Assignee)

Comment 1

10 years ago
Created attachment 326580 [details] [diff] [review]
v1

The patch implements the idea from the comment 0. It compiles but I have not tested it beyond that.
(Assignee)

Comment 2

10 years ago
Created attachment 326634 [details] [diff] [review]
v2

This passes the tests
Attachment #326580 - Attachment is obsolete: true
Attachment #326634 - Flags: review?(brendan)
(Assignee)

Comment 3

10 years ago
It turned out that it is possible to eliminate JSStackFrame.vars in addition to JSStackFrame.nvars. If callers of js_Interpret would allocate the stack slots together with var space, then JSStackFrame.vars becomes an alias to spbase - nvars. Thus, if in addition the order of variables in the var array would be reversed, the code can access a variable with the index "i" using fp->spbase[-i] instead of fp->vars[i]. 
Summary: SM: eliminating JSStackFrame.nvars → SM: eliminating JSStackFrame.(nvars|vars)
(Assignee)

Updated

10 years ago
Attachment #326634 - Attachment is obsolete: true
Attachment #326634 - Flags: review?(brendan)
Summary: SM: eliminating JSStackFrame.(nvars|vars) → SM: eliminate JSStackFrame.(nvars|vars)
Sounds good -- we can fuse vars and operand stack allocation for sure.

/be
(Assignee)

Comment 5

10 years ago
Created attachment 327084 [details] [diff] [review]
v3

The patch removes both vars and nvars from JSStackFrame. It passes shell tests but I need to do more testing before asking for review. In js_Execute the patch insists that if the function receives the upper frame for the script to execute, then the script should have zero global vars and shared regexp.

This is equivalent to stating that the script should not contain JSOP_(SET|GET|CALL)GVAR and JSOP_REGEXP bytecodes. This should not change anything as currently any execution of script with those bytecodes would corrupt the upper frame vars and/or stack.
Igor, this patch is a helpful starting point for further work in tracemonkey -- are you able to test more and request review soon? If not, I would be happy to take it off your hands ;-).

/be
(Assignee)

Comment 7

10 years ago
Created attachment 328666 [details] [diff] [review]
v4

The new patch syncs with trunk changes and fixes the bug in js_PutCallObject where previously I forgot to change var enumerator to go in the reverese order.

The patch passes shell/mochi tests.
Attachment #327084 - Attachment is obsolete: true
Attachment #328666 - Flags: review?(brendan)
(Assignee)

Comment 8

10 years ago
(In reply to comment #6)
> Igor, this patch is a helpful starting point for further work in tracemonkey --
> are you able to test more and request review soon? If not, I would be happy to
> take it off your hands ;-).

I updated the patch. But feel free to grab not yet filed bugs about combining JSOP_VAR and JSOP_LOCAL and combining JSOP_(ARG|VAR|LOCAL). The former should be straightforward with this bug fixed. The latter is more involved as it would be necessary either always copy args to the new location when calling the interpreted function or have parallel stacks for JSInterpretedFrame instances and the script stack itself. Plus this will restrict the total number of args and vars to 64K unless a special unoptimized bytecode would be added to access slot index beyond 64K. 
Unifying vars and locals can be done, but args are hard: the caller pushes as many as it likes, the callee was compiled based on its arity, so it cannot index linearly from argv[0..(argc-1)] and then the next slot is vars[0] -- there could be extra args on the caller's operand stack.

This, plus the possible discontinuity, led long ago to the JOF_QARG vs. JOF_QVAR operator coding.

/be
(Assignee)

Comment 10

10 years ago
(In reply to comment #9)
> Unifying vars and locals can be done, but args are hard: the caller pushes as
> many as it likes, the callee was compiled based on its arity, so it cannot
> index linearly from argv[0..(argc-1)] and then the next slot is vars[0] --
> there could be extra args on the caller's operand stack.

This is only the issue when the caller pushes more arguments than the function defines. This is rare. To support it the arguments can be either copied or rotated on the stack to avoid the hole between defined arguments and vars.

On the other hand simply moving args past the space to js_InlinedFrame can be very cheap given that it will remove the need for js_InlinedFrame.args and js_InlinedFrame.spbase. The former can always be calculated from the address of the frame itself. 
We only have to worry about excess args for scripted functions if there's a reference to arguments or a call to eval, right?  But even then, we can put the excess args after vars, and have ARGSUB manage the skipping of vars for those case?

if (n >= nargs) slot += nvars; // comme ca?

Doesn't work for natives without an ABI change or some copying, dunno if that's a problem.
(Assignee)

Comment 12

10 years ago
(In reply to comment #11)
> We only have to worry about excess args for scripted functions if there's a
> reference to arguments or a call to eval, right?

arguments[i] or eval or closure referring to args is not a problem: js_GetArgsProperty from jsfun.cpp can apply all the necessary logic to calculate argument reference even if arguments are split between 2 frames. One or two extra ifs there would not hurt performance. 


  But even then, we can put the
> excess args after vars, and have ARGSUB manage the skipping of vars for those
> case?
> 
> if (n >= nargs) slot += nvars; // comme ca?
> 
> Doesn't work for natives without an ABI change or some copying, dunno if that's
> a problem.
> 

(Assignee)

Updated

10 years ago
Blocks: 444444
(Assignee)

Comment 13

10 years ago
(In reply to comment #10)
> On the other hand simply moving args past the space to js_InlinedFrame can be
> very cheap given that it will remove the need for js_InlinedFrame.args and
> js_InlinedFrame.spbase. The former can always be calculated from the address of
> the frame itself. 

That move is not necessary if the interpreter would use the following layout for all the slots:

[agument_slots] [space for JS_StackFrame/jS_InlineFrame] [var slots] [local and stack slots]

Then instead of the current triples of arg/var/local one would need single slot instruction which uses (jsval *)fp[slotIndex] to get the reference to the particular slot. Here slotIndex is signed int16 number which is negative for arguments and positive for var and locals and accounts for stack slots reserved for JS_StackFrame/jS_InlineFrame.

The only complication with this schema is the case of callers that pushes more arguments than the function knows. To handle this the code can simply shift cyclically all the arguments so known arguments would come after the extra ones. This would slightly complicates js_GetArgsProperty as it would need to account for this shift, but this is a cheap price to pay for bytecode simplicity.
(Assignee)

Updated

10 years ago
Blocks: 421864
This all sounds great. How long do you think it'll take to do? Is this ahead of the x0 too many doubles bug in your priority q? Let me know if I can help.

/be
(Assignee)

Comment 15

10 years ago
(In reply to comment #14)
> This all sounds great. How long do you think it'll take to do? Is this ahead of
> the x0 too many doubles bug in your priority q? Let me know if I can help.

My current plan for doubles includes using spbase-allocated space for d-registers and all these eliminate frame.something bugs comes from various attempts to grab the slots without bloating the frame. With this bug fixed the layout may look like:

dregs-vars-stack_slots with d-register pointer calculated as fp->spbase[-fun->u.i.nvars - d_index].

But with the schema about universal JSOP_SLOT a better layout would be:

args_slots-JSStackFrameSlots-dregs-var_slots-stack_slots

The reason for this is that with the hard limit of 128 d-regs this does not limit practically the amount of possible variables/stack slots while allowing very fast access to both dreg and args/vars/locals using (double*)(fp + fixed_offset) and (jsval*)fp[(int16)slot]. Here the slot value as written into the bytecode should account for the size of both JSStackFrame and the number of d-regs.

So if JSOP_SLOT is desirable, it would be better to finish d-register patch after implementing the parts of JSOP_SLOT in depends on. So the question is should we go for JSOP_SLOT? 

Its main drawback is that it would limit the max number of arguments to 32K and the total of dregs-vars-locals also to 32K down from the current limit of 64K for each of args/vars/locals. But this is not a fatal since the limit can be extended with a couple of unoptimized JSOP_(GET|SET)HUGESLOT bytecodes later.

It is hard to see how exactly much time is necessary to implement all of this, but the first necessary step is to implement just layout allocation and index rebasing without fusing bytecodes. This would make JSOP_GET(ARG|VAR|LOCAL) identical in the interpreter but would not require changing the decompiler. 

Moreover, the scope of the initial work can be farther limited if the current layout for arguments would not be changed. That is, the idea is to implement a picture like:

JSStackFrameSlots-var_slots-stack_slots

with access to vars/locals given by (jsval*)fp[var_or_local_slot]. This would make just SOP_(GET|SET)(VAR|LOCAL) identical in the interpreter, but would avoid all the issues with arguments shifting to support oversized calls. This is effectively what is the bug 444444 about. With that (nicely numbered :) ) bug fixed d-regs work can be finished without being affected by farther work on JSOP_SLOT.

Given the experience with this bug i think I can have a tested patch for bug 444444 before Monday.
(Assignee)

Comment 16

10 years ago
Created attachment 329172 [details] [diff] [review]
v5 (unified var and local slots)

This patch unifies the index space for variables and locals. I replaces JSStackFrame.(vars[i]|spbase[i]) with JSStackFrame.slots[i] where i is uint16 index referring to both vars and locals. This way in the interpreter various JSOP_*LOCAL* bytecode cases shares the same code with the corresponding JSOP_*VAR* cases. It should be straightforward to eliminate all JSOP_*LOCAL* bytecodes (or, as a matter of taste, JSOP_*VAR* ones), but I left this to a follow up to minimize the patch.

Since the index space is shared, it is necessary to adjust indexes for locals to account for the total number of variables. For functions it is possible to do that during code generation. For a script with fast globals or with a regexp literals the adjustment required a separated pass over the emitted bytecode due to incremental code writing.

The patch passes shell and mochi tests. I will base the following work for the bug 421864 on it. The idea there is to reserve some extra slots before vars for double registers avoiding bloating JSStackFrame with extra fields.
Attachment #328666 - Attachment is obsolete: true
Attachment #328666 - Flags: review?(brendan)
(Assignee)

Comment 17

10 years ago
Created attachment 329354 [details] [diff] [review]
v5b

The new version fixes unused variables warnings and adds comments to js_GetVariableBytecodeLength about the switch bytecode structures.
Attachment #329172 - Attachment is obsolete: true
Attachment #329354 - Flags: review?(brendan)
(Assignee)

Comment 18

10 years ago
Created attachment 329362 [details] [diff] [review]
v6

The new version of the patch checks for globals + regexp 64K overflow in js_CompileScript.
Attachment #329362 - Flags: review?(brendan)
(Assignee)

Updated

10 years ago
Attachment #329362 - Attachment is patch: true
Attachment #329362 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

10 years ago
Attachment #329354 - Attachment is obsolete: true
Attachment #329354 - Flags: review?(brendan)
Comment on attachment 329362 [details] [diff] [review]
v6

>-        SD_SET_TARGET(sd, NULL);
>+        SD_SET_TARGET(sd, (JSObject *) 0);

This doesn't look right or necessary. Others like it but with STOBJ_SET_SLOT seem more like C++ 0-casting, but why can't we stick with good old NULL?

>+      case TOK_ARRAYPUSH: {
>+          jsint slot;

jsint slot is overindented by 2.

>+        /*
>+         * The array object's stack index is in cg->arrayCompDepth. See below
>          * under the array initialiser code generator for array comprehension
>          * special casing.
>          */
>         if (!js_EmitTree(cx, cg, pn->pn_kid))
>             return JS_FALSE;
>-        EMIT_UINT16_IMM_OP(PN_OP(pn), cg->arrayCompSlot);
>-        break;
>+        slot = AdjustBlockSlot(cx, cg, cg->arrayCompDepth);

Is the Slot => Depth terminology change worth it? The AdjustBlockSlot parameter is named slot, not depth.

>+#define TREE_CONTEXT_FINISH(cx, tc)                                           \
>+    JS_SCOPE_DEPTH_METERING(                                                  \
>+        (tc)->maxScopeDepth == (uintN) -1 ||                                  \

Who stores -1?

>-                     * Fast globals use fp->vars to map the global name's
>+                     * Fast globals use frame variables to map the global name's

Nit: touching 80th column above.

>+inline jsval *
>+SpBase(JSStackFrame *fp)
>+{
>+    return fp->slots + fp->script->nfixed;
>+}
>+
>+inline uintN
>+GlobalVarCount(JSStackFrame *fp)
>+{
>+    uintN n;
>+    
>+    JS_ASSERT(!fp->fun);
>+    n = fp->script->nfixed;
>+    if (fp->script->regexpsOffset != 0)
>+        n -= JS_SCRIPT_REGEXPS(fp->script)->length;
>+    return n;
>+}

Shouldn't these be static inline to avoid expansion as extern?

>+            if (JOF_TYPE(cs->format) == JOF_LOCAL ||
>+                (JOF_TYPE(cs->format) == JOF_SLOTATOM)) {
>+                /*
>+                 * JSOP_GETARGPROP and JSOP_GETVARPROP are also of
>+                 * JOF_SLOTATOM type, but they present only in a function.

s/they present/they are present/

>+        STOBJ_SET_PARENT(FUN_OBJECT(fun), 0);
>+        STOBJ_SET_PROTO(FUN_OBJECT(fun), 0);

More such NULL => 0 (without casts) gratuitous changes?

>+inline uintN
>+StackDepth(JSScript *script)
>+{
>+    return script->nslots- script->nfixed;
>+}

Another static inline.

>-#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 27)
>+#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 28)

What required this? js_XDRScript changes might mean JSXDR_MAGIC_SCRIPT_8 aka JSXDR_MAGIC_SCRIPT_CURRENT, but not a JSXDR_BYTECODE_VERSION rev.

Looks pretty good, want this patch and more (obsoleting of ops!).

/be
(Assignee)

Comment 20

10 years ago
(In reply to comment #19)
> (From update of attachment 329362 [details] [diff] [review])
> >-        SD_SET_TARGET(sd, NULL);
> >+        SD_SET_TARGET(sd, (JSObject *) 0);
> 
> This doesn't look right or necessary. Others like it but with STOBJ_SET_SLOT
> seem more like C++ 0-casting, but why can't we stick with good old NULL?

This is to silence GCC warnings about NULL casting to unsigned long on 64-bit Linux.

> >-        EMIT_UINT16_IMM_OP(PN_OP(pn), cg->arrayCompSlot);
> >-        break;
> >+        slot = AdjustBlockSlot(cx, cg, cg->arrayCompDepth);
> 
> Is the Slot => Depth terminology change worth it? The AdjustBlockSlot parameter
> is named slot, not depth.

The rename was helpful to ensure that I have not missed code adjusting comprehension slots. Would uisng
            slot = cg->arrayCompDepth;
            slot = AdjustBlockSlot(cx, cg, slot);

clear up things?


> 
> >+#define TREE_CONTEXT_FINISH(cx, tc)                                           \
> >+    JS_SCOPE_DEPTH_METERING(                                                  \
> >+        (tc)->maxScopeDepth == (uintN) -1 ||                                  \
> 
> Who stores -1?

The scope metering should tell the global accounting code about maxScopeDepth only once. Since the tree context for functions is created twice, once for parsing and once for the code generation, the code sets maxScopeDepth to -1 in the TOK_FUNCTION case of js_EmitTree to avoid double-reporting and not to worry about using the function parse node to transfer that counter to the second tree context.

> 
> What required this? js_XDRScript changes might mean JSXDR_MAGIC_SCRIPT_8 aka
> JSXDR_MAGIC_SCRIPT_CURRENT, but not a JSXDR_BYTECODE_VERSION rev.

The changes made any current bytecode referring to let-locals invalid.
(Assignee)

Comment 21

10 years ago
Created attachment 329829 [details] [diff] [review]
v7

The new version of the patch fixes the issues from the comment 19:

diff -r 17c9eb092225 js/src/jsemit.cpp
--- a/js/src/jsemit.cpp	Wed Jul 16 15:38:50 2008 +0200
+++ b/js/src/jsemit.cpp	Wed Jul 16 16:10:45 2008 +0200
@@ -6094,1 +6094,1 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
-          jsint slot;
+        jsint slot;
@@ -6103,1 +6103,2 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
-        slot = AdjustBlockSlot(cx, cg, cg->arrayCompDepth);
+        slot = cg->arrayCompDepth;
+        slot = AdjustBlockSlot(cx, cg, slot);
diff -r 17c9eb092225 js/src/jsinterp.cpp
--- a/js/src/jsinterp.cpp	Wed Jul 16 15:38:50 2008 +0200
+++ b/js/src/jsinterp.cpp	Wed Jul 16 16:10:45 2008 +0200
@@ -5607,4 +5607,5 @@ js_Interpret(JSContext *cx)
-                     * Fast globals use frame variables to map the global name's
-                     * atom index to the permanent fp->varobj slot number,
-                     * tagged as a jsval.  The atom index for the global's
-                     * name literal is identical to its variable index.
+                     * Fast globals use frame variables to map the global
+                     * name's atom index to the permanent fp->varobj slot
+                     * number, tagged as a jsval. The atom index for the
+                     * global's name literal is identical to its variable
+                     * index.
diff -r 17c9eb092225 js/src/jsinterp.h
--- a/js/src/jsinterp.h	Wed Jul 16 15:38:50 2008 +0200
+++ b/js/src/jsinterp.h	Wed Jul 16 16:10:45 2008 +0200
@@ -96,1 +96,1 @@ inline jsval *
-inline jsval *
+static inline jsval *
@@ -102,1 +102,1 @@ inline uintN
-inline uintN
+static inline uintN
diff -r 17c9eb092225 js/src/jsparse.cpp
--- a/js/src/jsparse.cpp	Wed Jul 16 15:38:50 2008 +0200
+++ b/js/src/jsparse.cpp	Wed Jul 16 16:10:45 2008 +0200
@@ -656,2 +656,2 @@ js_CompileScript(JSContext *cx, JSObject
-                 * JSOP_GETARGPROP and JSOP_GETVARPROP are also of
-                 * JOF_SLOTATOM type, but they present only in a function.
+                 * JSOP_GETARGPROP and JSOP_GETVARPROP also have JOF_SLOTATOM
+                 * type, but they may be emitted only for a function.
@@ -1116,2 +1116,2 @@ NewCompilerFunction(JSContext *cx, JSTre
-        STOBJ_SET_PARENT(FUN_OBJECT(fun), 0);
-        STOBJ_SET_PROTO(FUN_OBJECT(fun), 0);
+        STOBJ_SET_PARENT(FUN_OBJECT(fun), (JSObject *) 0);
+        STOBJ_SET_PROTO(FUN_OBJECT(fun), (JSObject *) 0);
diff -r 17c9eb092225 js/src/jsscript.h
--- a/js/src/jsscript.h	Wed Jul 16 15:38:50 2008 +0200
+++ b/js/src/jsscript.h	Wed Jul 16 16:10:45 2008 +0200
@@ -115,1 +115,1 @@ inline uintN
-inline uintN
+static inline uintN
Attachment #329362 - Attachment is obsolete: true
Attachment #329829 - Flags: review?(brendan)
Attachment #329362 - Flags: review?(brendan)
Comment on attachment 329829 [details] [diff] [review]
v7

>-        SD_SET_TARGET(sd, NULL);
>+        SD_SET_TARGET(sd, (JSObject *) 0);

I'm still not sure why this is necessary. In any event, (JSObject*) is the wrong cast. From jsemit.h:

#define SD_SET_TARGET(sd,jt)    ((sd)->target = JT_SET_TAG(jt))

and

#define JT_SET_TAG(jt)          ((JSJumpTarget *)((jsword)(jt) | JT_TAG_BIT))

What is NULL in stddef.h or stdio.h on your 64-bit platform?

>+ * Adjust the slot for a block local to account for the number of variables
>+ * that share the same index space with locals. Due to the incremental code
>+ * generation for script we do the adjustment via code patching in

s/for script/for top-level script,/ (note comma)

>+ * js_CompileScript, see comments there.

s/,/;/

>+        STOBJ_SET_PARENT(FUN_OBJECT(fun), (JSObject *) 0);
>+        STOBJ_SET_PROTO(FUN_OBJECT(fun), (JSObject *) 0);

Right cast here, but again -- what is wrong with NULL that it can't be cast to jsword or jsval on the 64-bit target in question? Ideally, NULL is 0 for C++ at least (if not for C too).

>+static inline jsval *
>+SpBase(JSStackFrame *fp)

>+static inline uintN
>+StackDepth(JSScript *script)

I'm warming up to StackBase instead of SpBase -- Sp is odd-looking and cryptic, doesn't really say "operand stack base". True, StackBase could be taken as the oldest frame's operand stack base, but that's useless -- and in the context of StackDepth, StackBase makes sense.

/be
(Assignee)

Comment 23

10 years ago
(In reply to comment #22)
> (From update of attachment 329829 [details] [diff] [review])
> >-        SD_SET_TARGET(sd, NULL);
> >+        SD_SET_TARGET(sd, (JSObject *) 0);
> 
> I'm still not sure why this is necessary.

The exact warning is the following:

 warning: converting to non-pointer type ‘long int’ from NULL

and comes from using GCC's -Wconversion. From http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html :

-Wconversion
    Warn for implicit conversions that may alter a value....
    For C++, also warn for conversions between NULL and non-pointer types;...

The reason for the warning is that NULL is equivalent to 0 meaning that on 64-bit CPU sizeof(NULL) != sizeof(void *) etc. GCC can detect this since it by default expands NULL into __null which has absolutely the same meaning as 0 but allows warnings about possibly unintended usage.

In SM cases that usage is (int64) NULL which IMO is indeed suspicious since the cast requires expansion of the value from 32 to 64 bits while using something that is typically is associate with a pointer. I.e. that is on the same scale as hypothetical 

__near void* p;
...
(int64) p

where __near void* means a 32-bit pointer.
(Assignee)

Comment 24

10 years ago
Created attachment 330407 [details] [diff] [review]
v8

The new version fixes nits and renames SpBase(fp) into StackBase(fp)
Attachment #329829 - Attachment is obsolete: true
Attachment #330407 - Flags: review?(brendan)
Attachment #329829 - Flags: review?(brendan)
Comment on attachment 330407 [details] [diff] [review]
v8

Looks good. Casting 0 was considered portable style in the '80s, but generally NULL is enough. An alternative is to put (void*)(jt) in SD_SET_TARGET. Would that silence the warning?

/be
Attachment #330407 - Flags: review?(brendan) → review+
(Assignee)

Comment 26

10 years ago
(In reply to comment #25)
> (From update of attachment 330407 [details] [diff] [review])
> Looks good. Casting 0 was considered portable style in the '80s, 

It was a must when programming for DOS. I remember writing something like  (void far*)0 fifteen years ago. 

> An alternative is to put (void*)(jt) in SD_SET_TARGET. Would
> that silence the warning?

Yes, it would. But also writing just 0, not NULL, would silence GCC. Or turning the macro into inline function accepting a pointer as its argument. 
Let's go with 0 where casts seem unnecessary and 0 is unambiguous. Inlines may be a good thing, but we shouldn't C++-ize for its own sake yet. Want an orderly and probably minimal plan for conversion from C to C++, outside of new code.

/be
(Assignee)

Updated

10 years ago
Blocks: 446229
(Assignee)

Updated

10 years ago
No longer blocks: 446229
(Assignee)

Comment 28

10 years ago
(In reply to comment #27)
> Let's go with 0 where casts seem unnecessary and 0 is unambiguous.

To avoi unreleate to this bug changes I filed the bug 446229. In fact I should have one this long time ago.

> Inlines may
> be a good thing, but we shouldn't C++-ize for its own sake yet.

inline != C++. static inline has the semantics in C++ and C an is supported for a few years in all relevant C compilers.
(Assignee)

Comment 29

10 years ago
(In reply to comment #28)
> (In reply to comment #27)
> > Let's go with 0 where casts seem unnecessary and 0 is unambiguous.
> 
> To avoi unreleate to this bug changes I filed the bug 446229. In fact I should
> have one this long time ago.

Sorry for bad text here - "d" letter on the notebook has started to require an extra pressure. I meant:

To avoid unrelated to this bug changes I filed the bug 446229. In fact I should
have done this long time ago.
(Assignee)

Comment 30

10 years ago
Created attachment 330487 [details] [diff] [review]
v9

The new version is the previous patch minus warning fixes that went into 446229.
Attachment #330407 - Attachment is obsolete: true
Attachment #330487 - Flags: review+
(Assignee)

Comment 31

10 years ago
landed - http://hg.mozilla.org/mozilla-central/index.cgi/rev/471f34aa61df
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 32

10 years ago
Apparently the patch made sunSpider to run little bit faster, http://graphs.mozilla.org/graph.html#spst=range&spstart=0&spend=1216591080&bpst=cursor&bpstart=0&bpend=1216591080&m1tid=911647&m1bl=0&m1avg=0 . 

I wonder what contributed to that most, a smaller JSStackFrame or dense js_Interpret() with shared implementation of local and var bytecodes?

Updated

10 years ago
Depends on: 447702
(Assignee)

Updated

10 years ago
Blocks: 447762

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
Depends on: 453024
(Assignee)

Updated

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