417100 - jsregexp.c should inline more, and more aggressively

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Crowder]

Attachment: easy results from early benchmarking

Summary and patch say it all.  This relies on the patch from bug 417077

Comment 1

[Brian Crowder]

Before (in SunSpider):
    dna:                415.5ms +/- 0.6%
After:
    dna:                335.9ms +/- 2.3%

No other tests are hurt or improved enough to mention.

Comment 2

[Brendan Eich [:brendan]]

What's the code size hit?

/be

Comment 3

[Brian Crowder]

Before:
-rw-r--r--  1 crowder  crowder  45112 Feb 12 15:55 Darwin_OPT.OBJ/jsregexp.o
After:
-rw-r--r--  1 crowder  crowder  69024 Feb 12 15:55 Darwin_OPT.OBJ/jsregexp.o

Definitely not negligible, but some of these hits are worse than others.  The issue here, I think, is that we build with a general-purpose -Os across the board on Spidermonkey now, where jsregexp.c definitely benefits (and is designed to benefit, by virtue of lots of small functions, some of them invoked only once) from inlining.

Comment 4

[Igor Bukanov]

Comment on attachment 302918 [details] [diff] [review]
easy results from early benchmarking


>-static uintN
>-upcase(uintN ch)
>+JS_INLINE(static uintN
>+upcase(uintN ch))
> {
>     uintN cu;
> 
>     JS_ASSERT((uintN) (jschar) ch == ch);
>     if (ch < 128) {
>         if (ch - (uintN) 'a' <= (uintN) ('z' - 'a'))
>             ch -= (uintN) ('a' - 'A');
>         return ch;
>     }
> 
>     cu = JS_TOUPPER(ch);
>     return (cu < 128) ? ch : cu;
> }

Since the biggest contributor to the code size here is the universal TOUPPER macro I suggest to move it to a separated non-inline function. The same is applicable to the down case version.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 302918 [details] [diff] [review]
easy results from early benchmarking

Does the REGEXP_DEBUG stuff need to be inlined? Any reason it doesn't use JS_INLINE(...)? r/a+ in any event.

/be

Comment 6

[Brendan Eich [:brendan]]

Missed Igor's comment (no mid-air from attachment.cgi, a mixed blessing). Given the slow path not being tickled by benchmarks, it would be good to get those macro calls out of line.

/be

Comment 7

[Igor Bukanov]

Comment on attachment 302918 [details] [diff] [review]
easy results from early benchmarking


>
>-static REMatchState *
>+JS_INLINE(static REMatchState *
> FlatNIMatcher(REGlobalData *gData, REMatchState *x, jschar *matchChars,
>-              size_t length)
>+              size_t length))
> {
>     size_t i>     for (i = 0; i != length; i++) {
>         if (upcase(matchChars[i]) != upcase(x->cp[i]))
>             return NULL;

Here this upcase(a) != upcase(b) can be optimised since AFAIK non-ASCII upcase never gives ASCII.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Here this upcase(a) != upcase(b) can be optimised since AFAIK non-ASCII upcase
> never gives ASCII.

Georgian titlecase? Turkish dotless I? Just asking.

/be

Comment 9

[Igor Bukanov]

(In reply to comment #6)
> Missed Igor's comment (no mid-air from attachment.cgi, a mixed blessing).

and that remved r+ from  the patch :(

Comment 10

[Brendan Eich [:brendan]]

Turkish dotless I:

js> "\u0131".toLowerCase()
1
js> "\u0131"               
1
js> "\u0131".toUpperCase()
I
js> "\u0131".toUpperCase().charCodeAt(0)
73

So non-ASCII upcase gives ASCII.

/be

Comment 11

[Igor Bukanov]

(In reply to comment #9)
> (In reply to comment #6)
> > Missed Igor's comment (no mid-air from attachment.cgi, a mixed blessing).
> 
> and that remved r+ from  the patch :(
> 

and i even can not restore the state of the flag!

Comment 12

[Mike Schroepfer]

Comment on attachment 302918 [details] [diff] [review]
easy results from early benchmarking

Brendan giveth and schrep taketh away (as in removing a+ since r+ is removed)

Comment 13

[Igor Bukanov]

(In reply to comment #8)
> (In reply to comment #7)
> > Here this upcase(a) != upcase(b) can be optimised since AFAIK non-ASCII upcase
> > never gives ASCII.
> 
> Georgian titlecase? Turkish dotless I? Just asking.

But that does not matter since upcase would keep the letters in the original non-ASCII form. regexp != unicode.

Comment 14

[Brian Crowder]

I want it to be inlined just in case there's any possibility I'm paying for a function-call when I might not have to; but it's not using the always-inline stuff because it does seem to inline correctly (and portably) without it.  But in those cases, I don't mind using the inline "hint" instead of the forced-inlining.  Right now, even without the inline, they seem to be inlined anyway (by virtue of being static), but Stuart made me paranoid on IRC.

Didn't really understand comment #4 or comment #6, but please file as separate bugs.

Comment 15

[Brian Crowder]

Comment #7 looks worthy of a bug, too.

Comment 16

[Brendan Eich [:brendan]]

Do we really need followup bugs to avoid over-inlining uncommon tail code in upcase and downcase? Maybe so, but if it can be done here, so much the better.

I admit I'm not sure what to make of comment 7 in light of comment 10 (Igor, I do not follow comment 13), but comment 4 seems clear.

/be

Comment 17

[Brendan Eich [:brendan]]

Crowder, can you please take assignment of this bug? Thanks,

/be

Comment 18

[Brian Crowder]

Yeah, sorry.  I own it now.  I guess what I'm not understanding about comment 4 is why I would want to introduce another function-call, when the purpose of this patch is eliminating them?  I realize it is a code-size hit, but I think for these inner-loop routines, the code-size hit is worth the performance-gain.

Comment 19

[Brendan Eich [:brendan]]

Is it worth the perf gain to inline everything in upcase and downcase, if the tails are rarely called (never, in the case of notorious benchmarks)?

I guess it is not a big deal if the code size savings of splitting out functions, with their own prolog and epilog costs, is small or even 0. I will restamp r+ and leave it to you to decide (do the experiment, measure code size, if you think it is worth the trouble).

/be

Comment 20

[Brian Crowder]

Checking in jsregexp.c;
/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.181; previous revision: 3.180
done


I'll follow up on the research for the unicode up/down-casing in another bug; sayrer wanted this one to land asap.

Comment 21

[Bob Clary [:bc] (inactive)]

broke building the shell at least on windows:

jsregexp.c

jsregexp.c(117) : error C2054: expected '(' to follow 'inline'

jsregexp.c(118) : error C2085: 're_debug' : not in formal parameter list

jsregexp.c(118) : error C2143: syntax error : missing ';' before '{'

jsregexp.c(123) : error C2054: expected '(' to follow 'inline'

jsregexp.c(124) : error C2085: 're_debug_chars' : not in formal parameter list

jsregexp.c(124) : error C2143: syntax error : missing ';' before '{'

jsregexp.c(2088) : warning C4013: 're_debug' undefined; assuming extern returning int

jsregexp.c(2626) : warning C4013: 're_debug_chars' undefined; assuming extern returning int

Comment 22

[Brendan Eich [:brendan]]

I saw those in review bug assumed GNUC only -- sorry for not asking. Why wouldn't they use the JS_INLINE macro too?

/be

Comment 23

[Brian Crowder]

I quickly landed a fix to remove those inlines.  I'll JS_INLINE them after some more testing.