Closed Bug 417100 Opened 14 years ago Closed 14 years ago

jsregexp.c should inline more, and more aggressively

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: crowderbt, Assigned: crowderbt)

References

Details

(Keywords: perf)

Attachments

(1 file)

Summary and patch say it all.  This relies on the patch from bug 417077
Attachment #302918 - Flags: review?(brendan)
Depends on: 417077
Before (in SunSpider):
    dna:                415.5ms +/- 0.6%
After:
    dna:                335.9ms +/- 2.3%

No other tests are hurt or improved enough to mention.
Keywords: perf
What's the code size hit?

/be
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 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 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
Attachment #302918 - Flags: review?(brendan)
Attachment #302918 - Flags: review+
Attachment #302918 - Flags: approval1.9+
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 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.
Attachment #302918 - Flags: review+ → review?(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
(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 :(
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
(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 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)
Attachment #302918 - Flags: approval1.9+
(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.
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 #7 looks worthy of a bug, too.
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
Crowder, can you please take assignment of this bug? Thanks,

/be
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.
Assignee: general → crowder
Status: NEW → ASSIGNED
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
Attachment #302918 - Flags: review?(brendan) → review+
Attachment #302918 - Flags: approval1.9?
Attachment #302918 - Flags: approval1.9? → approval1.9+
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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

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

/be
I quickly landed a fix to remove those inlines.  I'll JS_INLINE them after some more testing.
Depends on: 417477
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.