524867 - hoist js_AllocStack out of inner String.replace loop

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: v.1

With an operation like:

  "abababababab".replace(/a/g, function(){return 'B'});

we currently call js_AllocStack/js_FreeStack before/after each js_Invoke of the lambda argument.  Like kindred functions in jsarray.cpp, we can do this once for the whole str_replace operation.  This patch does just that for a 10.9% speedup (8.1ms) on string-unpack-code.

The correctness of the patch relies on the fact that the number of arguments to each js_Invoke invocation is the same.  This follows from (1) that JSRegExp::parenCount is a constant value for a given JSRegExp and (2) the regexp used in a single str_replace invocation is fixed (cannot be changed by user code).

Comment 1

[Luke Wagner [:luke]]

Attachment: redux

This does the same thing, except using a destructor to call js_FreeStack.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 408864 [details] [diff] [review]
redux

Yummy.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 408864 [details] [diff] [review]
redux

>+    ReplaceData(JSContext *cx)
>+     : g(cx), invokevp(NULL), cb(cx)
>+    {}

Drive-by nit: : on its own line usually indents half a "tab" (c-basic-offset value) or two spaces -- not one.

This all fits on one line, so unless there's a debugability win it could go on one line.

Nice to see some RAII action, even though infallible ctors mean you have to do the null then alloc-with-check then store dance.

/be

Comment 4

[Luke Wagner [:luke]]

(In reply to comment #3)
I'm afraid I pushed a few hours ago; I'll try to remember to piggyback the nit-fix on the next thing I push.

http://hg.mozilla.org/tracemonkey/rev/3aec58a9bf6f

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9663b56ef62d