419225 - refactor ExecuteREBytecode and SimpleMatch

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P2)

Description

[Robert Sayre]

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

Comment 1

[Robert Sayre]

Attachment: make it all one function (WIP)

Comment 2

[Robert Sayre]

Attachment: fix simplematch cases

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
...

Comment 3

[Robert Sayre]

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.

Comment 4

[Robert Sayre]

Attachment: better WIP

Comment 5

[Robert Sayre]

this is going to be really high risk for Fx3. We save for later.

Comment 6

[Brian Crowder]

cc:ing x00000000 here, in case he's interested.

Comment 7

[Damon Sicore (:damons)]

Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?.  If this should be a blocker, please mark accordingly.

Comment 8

[Andreas Gal :gal]

Attachment: Merge SimpleMatch into ExecuteREBytecode using computed gosub.

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 **** diff. Most of the change is just indentation but macosx diff doesn't seem to like -w.

Comment 9

[Luke Wagner [:luke]]

This code has been removed.