153223 - New RegExp engine in Rhino

Status: VERIFIED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Phil Schwartau]

A new RegExp implementation has been checked into Rhino. See, for example,
bug 125562, "Regexp performance improvement".

This has fixed 6 Rhino bugs:
  bug 106548  /^.*?$/ will not match anything
  bug 114969  [], [^] are valid RegExp conditions
  bug 123439  Backreferences /(a)? etc./ must hold |undefined| if not used
  bug 124508  regexp.lastIndex should be integer-valued double, not uint32
  bug 125562  Regexp performance improvement
  bug 126317  Crash on re.exec(str) if re.lastIndex set to certain values

And has fixed 6 test failures:
  ecma_3/RegExp/regress-105972.js
  ecma_3/RegExp/regress-100199.js
  ecma_3/RegExp/regress-123437.js
  ecma_2/RegExp/properties-002.js
  ecma_3/RegExp/regress-85721.js
  ecma_3/RegExp/15.10.6.2-2.js


I have removed all 6 of these from the "rhino-n.tests" skip list.


However, I am getting 6 new test failures:
  ecma_2/String/match-002.js
  ecma_2/String/match-003.js
  ecma_3/String/regress-83293.js
  ecma_3/Unicode/uc-002.js
  ecma_3/Unicode/uc-004.js
  js1_2/regexp/control_characters.js


I am filing this bug to track this issue. I will attach an HTML 
file of these test failures below, with hyperlinks to the testcases -

Comment 1

[Phil Schwartau]

Attachment: New RegExp test failures

Comment 2

[rogerl (gone)]

Fix checked in - variety of things:

'isWhitespace' doesn't return 'true' for non-breaking spaces.
The 'flat' input string might be the empty string.
Messed up sign extension from Byte to Char.
Messed up flow-control for capture setting in 'Match' vs. 'Test' cases.

Comment 3

[Phil Schwartau]

The latest patch fixes all 6 of the new testcase failures I reported.
However, it is causing one of the previous testcases to fail:


*-* Testcase ecma_3/RegExp/regress-85721.js failed:
Bug Number 85721
STATUS: Performance: execution of regular expression
Failure messages were:

FAILED!: Section 1 of test -
         Expected value 'Execution took less than 25 ms',
         Actual value   'Execution took 63 ms'


FAILED!: Section 2 of test -

regexp = /<sql:connection 
id="([^\r\n]*?)">\s*<sql:url>\s*([^\r\n]*?)\s*<\/sql:url>\s*<sql:driver>\s*([^\r
\n]*?)\s*<
\/sql:driver>\s*(\s*<sql:userId>\s*([^\r\n]*?)\s*<\/sql:userId>\s*)?\s*(\s*<sql:
password>\s*([^\r\n]*?)\s*<\/sql:password>\s*
)?\s*<\/sql:connection>/

string = '<sql:connection id="conn1"> <sql:url>www.m.com</sql:url> 
<sql:driver>drive.class</sql:driver>\n<sql:userId
>foo</sql:userId> <sql:password>goo</sql:password> </sql:connection>'


FAILED!: ERROR !!! regexp FAILED to match anything !!!

Expect: ["<sql:connection id="conn1"> <sql:url>www.m.com</sql:url> 
<sql:driver>drive.class</sql:driver>
<sql:userId>foo</sql:userId> <sql:password>goo</sql:password> 
</sql:connection>", "conn1", "www.m.com", "drive.class
", "<sql:userId>foo</sql:userId> ", "foo", "<sql:password>goo</sql:password> ", 
"goo"]

Actual: null



Section 1 (performance measurement) can be adusted if 25 ms is too unrealistic.
However, note that before the latest patch, this section passed.

Section 2 is checking the accuracy of the regexp match. It is a complicated
one, and I'm not sure what's going wrong...

Comment 4

[rogerl (gone)]

ggggaaaaaaaaaaaaarrrrrgghnnnnnnnnnhhhhh!

I ran the tests, I swear I ran the tests.

Comment 5

[rogerl (gone)]

New fix checked in - isSpaceChar wasn't the right answer after all.
Ran all the tests - this time without any skip lists at all, I'm getting 36 
failures (Phil - does this seem right?) though none seem RegExp related.

Comment 6

[Phil Schwartau]

Roger: that sounds about right, actually. There is a skip list 
called "rhino-n.tests" in the js/tests/ directory, and it contains
about that number. 

I will run the full testsuite myself on this and report back -

Comment 7

[Phil Schwartau]

Marking Verified Fixed.

I get no test failures in either the compiled or interpreted modes
of Rhino. I am using only the current rhino-n.tests as a skip list.

After discussion with Roger, I have increased the time limit in
ecma_3/RegExp/regress-85721.js to 100 ms for the performance tests.

Comment 8

[Igor Bukanov]

Targeting as resolved against 1.5R4