580480 - allow flat-matching in String ops which pass an optional arguments like "g"

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Currently, String.prototype.{match, replace, search} uses simple string matching (thereby skipping the regexp engine) when the pattern contains no regexp meta-chars and no additional flags (like "g") are passed.  With bug 579173, we also avoid flattening sufficiently-heavy ropes.  To have this optimization apply more widely, we could relax the "no additional flags" restriction to support, e.g., "g".

In particular, this comes up with the old (0.9) SS test where (1) regexp-dna passes "g" to replace (2) we win 10ms with the rope trick.

This is not necessarily a priority (unless we care about 10ms on SS 0.9), just a future TODO.

Comment 1

[Till Schneidereit [:till]]

I'd be interested in working on this bug as it seems easy enough to be a good way to get to know the SpiderMonkey code base.

I have done some preliminary investigation of RegExpGuard.tryFlatMatch and AFAIKT, there are several issues that have to be dealt with:

1. get the re-flags from *vp (so this has to be added to tryFlathMatch's signature)

2. compare the flags to cases that can be dealt with inside of tryFlatMatch

3. actually deal with the cases found in 2.



1. seems pretty straight-forward and my test builds with that included seem to be working properly

For 2., I guess ParseRegExpFlags should (and seems to) work

For 3., there are several different cases:

- I think that any combination of the flags 'm' and 'y' (and an empty flags string) can be ignored: replace doesn't interpret meta chars at all and the result of match and search shouldn't be changed by these flags.

- 'g' should again not make any difference for match and search. For replace, though, it seems like FlatMatch and BuildFlatReplacement would have to be extended to support dealing with multiple matches.

- 'i' should continue to make tryFlatMatch bail out.


Does this analysis seem reasonable? And if so: would someone be willing to mentor me through this task? I have to warn, though: I'm not _that_ experienced with C/C++, so I'd totally understand if that, combined with this bug's low priority makes mentoring me not that interesting.

Comment 2

[Luke Wagner [:luke]]

One thing I think I didn't realize when filing this bug in 2010 was that the optional regexp args are a mozilla-specific extension.  A quick test shows that, e.g., Chrome does not seem to support these optional args.  I am not aware of any attempts to add these flags to the standard.  Thus, it does not seem like we should spend any effort to optimize them (in fact, one day we may remove them!).

cdleary: am I right about the above?

Comment 3

[Till Schneidereit [:till]]

Ah, right. That makes sense.

Depending on how much any of these flags are used, maybe it still makes sense to deal with the easy cases? Meaning the empty flags string and the 'm' and 'y' flags and 'g' for match and search (but not replace)? That should be pretty quick and without an undue increase of code complexity.

Comment 4

[Luke Wagner [:luke]]

It would be interesting to measure: add a counter to JSRuntime that you inc every time such a flag is used, dump out the counter in JS_DestroyRuntime, then browse around and see what you find.

Comment 5

[Till Schneidereit [:till]]

I added the instrumentation you suggested and then used http://gregor-wagner.com/tmp/mem to open the top 150 sites. The result: there were 3 usages of the flags argument, all of them to replace and all of them with the global flag.

Based on that, I have to agree with you: This isn't worth any optimization and increase in code complexity at all. Thus, I suggest WONTFIXing this bug.


As an aside: Since I was already looking into this topic, I took the time to check for usages of optional flags in the Firefox/ Gecko source itself. All in all, there seem to be only 11 usages, all for String.prototype.replace, so removing support for them seems feasible.

Comment 6

[Luke Wagner [:luke]]

(In reply to Till Schneidereit from comment #5)
> Based on that, I have to agree with you: This isn't worth any optimization
> and increase in code complexity at all. Thus, I suggest WONTFIXing this bug.

Agreed.  There are many things about strings in JS to be optimized, though.  I hope you consider looking around more :)

> As an aside: Since I was already looking into this topic, I took the time to
> check for usages of optional flags in the Firefox/ Gecko source itself. All
> in all, there seem to be only 11 usages, all for String.prototype.replace,
> so removing support for them seems feasible.

Yes, please file a bug to remove them (and cc me).