jsregexp.c should inline more, and more aggressively

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: crowderbt, Assigned: crowderbt)

Tracking

({perf})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
Created attachment 302918 [details] [diff] [review]
easy results from early benchmarking

Summary and patch say it all.  This relies on the patch from bug 417077
Attachment #302918 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Depends on: 417077
(Assignee)

Comment 1

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

No other tests are hurt or improved enough to mention.
(Assignee)

Updated

11 years ago
Keywords: perf
What's the code size hit?

/be
(Assignee)

Comment 3

11 years ago
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

11 years ago
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 7

11 years ago
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

Comment 9

11 years ago
(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

Comment 11

11 years ago
(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

11 years ago
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+

Comment 13

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

Comment 14

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

Comment 15

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

Comment 18

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

Updated

11 years ago
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

Updated

11 years ago
Attachment #302918 - Flags: review?(brendan) → review+

Updated

11 years ago
Attachment #302918 - Flags: approval1.9?

Updated

11 years ago
Attachment #302918 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 20

11 years ago
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
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 21

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

Comment 23

11 years ago
I quickly landed a fix to remove those inlines.  I'll JS_INLINE them after some more testing.
Depends on: 417477

Updated

11 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.