hoist js_AllocStack out of inner String.replace loop

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Created attachment 408752 [details] [diff] [review]
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).
Attachment #408752 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

9 years ago
Summary: hoist js_AllocStack out String.replace loop → hoist js_AllocStack out of inner String.replace loop
(Assignee)

Comment 1

9 years ago
Created attachment 408864 [details] [diff] [review]
redux

This does the same thing, except using a destructor to call js_FreeStack.
Attachment #408752 - Attachment is obsolete: true
Attachment #408864 - Flags: review?(jwalden+bmo)
Attachment #408752 - Flags: review?(jwalden+bmo)

Comment 2

9 years ago
Comment on attachment 408864 [details] [diff] [review]
redux

Yummy.
Attachment #408864 - Flags: review?(jwalden+bmo) → review+
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
(Assignee)

Comment 4

9 years ago
(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
Whiteboard: fixed-in-tracemonkey

Comment 5

9 years ago
http://hg.mozilla.org/mozilla-central/rev/9663b56ef62d
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.