Avoid unnecessary RegExpObject clones in CopyScript()

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8714950 [details] [diff] [review]
Avoid unnecessary RegExpObject clones in CopyScript()

Luke, you wrote this code back in 2012 and I reviewed it. Let's reverse roles
this time :)
Attachment #8714950 - Flags: review?(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!
Attachment #8714950 - Flags: review?(luke) → review+
(Assignee)

Comment 3

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

Comment 4

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/65da4da20053
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.