1245233 - Avoid unnecessary RegExpObject clones in CopyScript()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

js::detail::CopyScript() has this code:

> AutoObjectVector regexps(cx);
> for (unsigned i = 0; i < nregexps; i++) {
>     HeapPtrObject* vector = src->regexps()->vector;
>     for (unsigned i = 0; i < nregexps; i++) {
>         JSObject* clone = CloneScriptRegExpObject(cx, vector[i]->as<RegExpObject>());
>         if (!clone || !regexps.append(clone))
>             return false;
>     }
> }

Whoops! No need for a nested loop there. As a result, when there are N regexps
we clone N*N times instead of N times.

Fortunately, this function isn't all that hot. In a minute or two of browsing
across a few sites I got this frequency data:

> 151 counts:
> (  1)       84 (55.6%, 55.6%): nregexps = 1
> (  2)       37 (24.5%, 80.1%): nregexps = 2
> (  3)       17 (11.3%, 91.4%): nregexps = 3
> (  4)       11 ( 7.3%, 98.7%): nregexps = 4
> (  5)        1 ( 0.7%, 99.3%): nregexps = 8
> (  6)        1 ( 0.7%,100.0%): nregexps = 7

We should have done (84*1 + 37*2 + 17*3 + 11*4 + 1*7 + 1*8) = 268 clones.
We actually did (84*1 + 37*4 + 17*9 + 11*16 + 1*49 + 1*64) = 674 clones.

Not a huge deal, but it's probably possible to come up with pathological cases
where the difference is much larger.

(BTW, cpeterson: this code was added almost four years ago, in bug 749617. I
noticed the problem today via serendipitous code inspection. -Wshadow almost
certainly would have identified it immediately.)

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Avoid unnecessary RegExpObject clones in CopyScript()

Luke, you wrote this code back in 2012 and I reviewed it. Let's reverse roles
this time :)

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8714950 [details] [diff] [review]
Avoid unnecessary RegExpObject clones in CopyScript()

Review of attachment 8714950 [details] [diff] [review]:
-----------------------------------------------------------------

lol, that's very silly; good catch!

Comment 3

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65da4da2005382e31252999b25cadc3240f4b107
Bug 1245233 - Avoid unnecessary RegExpObject clones in CopyScript(). r=luke.

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/65da4da20053