Closed Bug 419225 Opened 12 years ago Closed 7 years ago

refactor ExecuteREBytecode and SimpleMatch

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED WORKSFORME
Future

People

(Reporter: sayrer, Assigned: sayrer)

Details

Attachments

(2 files, 2 obsolete files)

This is one of those things you have to dive in and do all at once, without testing in increments. I have taken the plunge, and I have a new layout that looks like it should work. I haven't tested it yet, so it's still full of bugs.

before:
  robert-sayres-macbook-pro-15:src sayrer$ ls -l jsregexp.o 
  -rw-r--r--  1 sayrer  staff  69388 Feb 23 17:36 jsregexp.o

after:
  robert-sayres-macbook-pro-15:src sayrer$ ls -l jsregexp.o 
  -rw-r--r--  1 sayrer  staff  47348 Feb 23 17:38 jsregexp.o
Attached patch make it all one function (WIP) (obsolete) — Splinter Review
Attachment #305257 - Attachment description: make it all one function → make it all one function (WIP)
Attached patch fix simplematch cases (obsolete) — Splinter Review
Looks like I botched the repeat op.

expect:

000001: altprereq
000010:   flat1 '>' == '>' *         BT_Push: 0,0
000012:   star
000015:   dot *      BT_Push: 0,0
000016:     endchild
000012:     repeat
000015:   dot *      BT_Push: 0,0
000015:   dot *      BT_Push: 0,0
000015:   dot *      BT_Push: 0,0
000015:   dot *      BT_Push: 0,0
000015:   dot *      BT_Push: 0,0
...

getting

000001: altprereq
000010:   flat1 '>' == '>' *    BT_Push: 0,0
000012:   star
000012:   dot *         BT_Push: 0,0
000016:     endchild
000012:     repeat
000012:   repeat
000012: repeat
000012:   repeat
000012:     repeat
000012:       repeat
000012:         repeat
000012:           repeat
000012:             repeat
000012:               repeat
000012:                 repeat
000012:                   repeat
000012:                     repeat
000012:                       repeat
000012:                         repeat
...
Attachment #305257 - Attachment is obsolete: true
3062> if (!SimpleMatch(gData, x, nextop, &nextpc, JS_TRUE)) {

The problem is that SimpleMatch operates on "nextop" in the REOP_REPEAT loop, rather than "op", as in the other cases.
Attached patch better WIPSplinter Review
Attachment #305274 - Attachment is obsolete: true
this is going to be really high risk for Fx3. We save for later.
Target Milestone: --- → Future
cc:ing x00000000 here, in case he's interested.
Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?.  If this should be a blocker, please mark accordingly.
Flags: wanted1.9.1?
Priority: -- → P2
Flags: wanted1.9.1? → wanted1.9.1+
Assignee: general → sayrer
OS: Mac OS X → All
Hardware: PC → All
Version: unspecified → Trunk
Ok, so here is the whole sob story: I spent my Saturday beating up on jsregexp. The attached patch merges SimpleMatch into ExecuteREBytecode by copying the SimpleMatch code at the end of the function. All invocations of SimpleMatch are replaced with a goto to that location and the return address is stored in void* label, which is jumped to using a computed goto at the end of SimpleMatch.

GCC finds this code extremly offensive (gcc crashes), since we use a computed goto inside a must-inline function, so I had to merge ExecuteREBytecode into MatchRegExp, since not inlining ExecuteREBytecode is not an option (slowdown 1.5x).

The patch has a flag that enables and disables this optimization (LOCAL_SIMPLE_MATCH).

The results are disheartening:

old code: 240 237 239 239 239 
local SimpleMatch: 269 268 269 270 269

The new code saves a ton of space though:

__TEXT	__DATA	__OBJC	others	dec	hex
47577	560	0	278	48415	bd1f (old)
28289	560	0	278	29127	71c7 (merged SimpleMatch)

While this approach is slightly different than sayrer's and the computed goto might confuse gcc a bit, after this experiment I highly doubt sayrer's patch would yield a speedup either.

Executive summary: jsregexp sucks, and its probably not fixable with reasonable effort. Do we want to look for alternatives?

PS: Sorry for the shitty diff. Most of the change is just indentation but macosx diff doesn't seem to like -w.
This code has been removed.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.