Bug 67337 – Update RegExp and related classes to use 8 bit strings when available

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

As part of the changes for https://bugs.webkit.org/show_bug.cgi?id=66161 we need to change the JavaScriptCore regular expression code to handle 8 bit strings.  This includes the YARR parser, JIT and interpreter code.

[Michael Saboff]

Created attachment 106671 [details]
Proposed Patch

This patch contains 8bit code paths for the YARR interpreter and JIT.  The implementations of the JIT have been coded for all current architectures, but I have only tested X86_64 and ARM v7.

[WebKit Review Bot]

Attachment 106671 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/assembler/X86Assembler.h:845:  cmpw_ir is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:1175:  movzbl_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Forgot to mention performance results of this change.  Running command line benchmarks, SunSpider is 0.1% faster (noise level) and V8 is 0.2% slower.

[WebKit Review Bot]

Comment on attachment 106671 [details]
Proposed Patch

Attachment 106671 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9525004

[Gavin Barraclough]

Comment on attachment 106671 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106671&action=review

Looks good. Few issues above, my only major concern is is branch16, but would be prudent to get some non-JIT numbers to prove the extra check in the interpreter isn't an overhead.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1074
> +    Jump branch16(RelationalCondition cond, RegisterID left, TrustedImm32 right)

This is actually doing a 32-bit by 16-bit compare.  We should either make this work like the other branch16 method (only consider the low 16 bits), or we should be ripping out branch16 and just using a branch32.  A method called branch16 that compares 32 bits isn't right.

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:884
> +            m_assembler.testl_rr(left, left);

I think this needs to be a testw?

> Source/JavaScriptCore/runtime/RegExp.cpp:308
> +{

It might help understanding to add the following documentary ASSERT:

// If the state is NotCompiled or ParseError, then there is no representation.
// If there is a representation, and the state must be either JITCode or ByteCode.
ASSERT(!!m_representation == (m_state == JITCode || m_state == ByteCode));

> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:170
>  

We should probably have some kind of comment on this class indicating that it is a placeholder for something better one we have 8-bit StringImpls.

> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:209
> +            if (m_charSize == Char8)

We should check the performance impact of this check.

[Michael Saboff]

Created attachment 106826 [details]
Updated patch addressing the concerns raised with the prior patch.

I checked the performance of the interpreter character size check with JIT turned off running V8 RegExp test.  It is 3% slower.  As we change to use the replacement class we can check and tune at that time.

There still may be an export issue with windows that I will rely on the buildbot catching.

[WebKit Review Bot]

Attachment 106826 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/assembler/X86Assembler.h:845:  cmpw_ir is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:1175:  movzbl_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 106826 [details]
Updated patch addressing the concerns raised with the prior patch.

Attachment 106826 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9626306

[WebKit Review Bot]

Comment on attachment 106826 [details]
Updated patch addressing the concerns raised with the prior patch.

Attachment 106826 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9626325

[WebKit Review Bot]

Comment on attachment 106826 [details]
Updated patch addressing the concerns raised with the prior patch.

Attachment 106826 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9625568

[Geoffrey Garen]

> I checked the performance of the interpreter character size check with JIT turned off running V8 RegExp test.  It is 3% slower.  As we change to use the replacement class we can check and tune at that time.

I'm not convinced by this logic. Why should we expect things to get faster when we rename the abstract iterator class to StringConstCharacterIterator? I expect things not to get faster, since the source of the performance problem -- taking a branch on every character access -- will still be there.

[Michael Saboff]

(In reply to comment #11)
> > I checked the performance of the interpreter character size check with JIT turned off running V8 RegExp test.  It is 3% slower.  As we change to use the replacement class we can check and tune at that time.
> 
> I'm not convinced by this logic. Why should we expect things to get faster when we rename the abstract iterator class to StringConstCharacterIterator? I expect things not to get faster, since the source of the performance problem -- taking a branch on every character access -- will still be there.

I expect that we'll do more than just change names.  One of the tuning possibilities includes moving to a template implementation of the interpreter that should restore all performance back, no per character branch.

[Michael Saboff]

Created attachment 106923 [details]
Updated patch to fix build failures

[WebKit Review Bot]

Attachment 106923 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/assembler/X86Assembler.h:845:  cmpw_ir is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:1175:  movzbl_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Comment on attachment 106923 [details]
Updated patch to fix build failures

Looks great.

[Michael Saboff]

Committed r94981: <http://trac.webkit.org/changeset/94981>

[James Robinson]

This adds a build-time dependency on runtime/UString.cpp from yarr/YarrInterpreter.cpp.  Historically in the Chromium build we've avoided compiling any files in runtime/.  Is this an intentional change in the dependencies for yarr or unintentional?  Would you recommend that we start compiling in all of runtime/ even in non-JSC builds?

[James Robinson]

Filed https://bugs.webkit.org/show_bug.cgi?id=67979 about this issue.

[James Robinson]

WTF::String has a latin1() that looks identical to UString's.  Would it make sense for CharAccess be implemented in terms of WTF::String or WTF::StringImpl instead of JSC::Ustring?

[James Robinson]

(In reply to comment #19)
> WTF::String has a latin1() that looks identical to UString's.  Would it make sense for CharAccess be implemented in terms of WTF::String or WTF::StringImpl instead of JSC::Ustring?

Ack sorry, I meant to post this comment on https://bugs.webkit.org/show_bug.cgi?id=67979

[Alexey Proskuryakov]

Comment on attachment 106923 [details]
Updated patch to fix build failures

View in context: https://bugs.webkit.org/attachment.cgi?id=106923&action=review

> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:1479
> +    Interpreter(BytecodePattern* pattern, int* output, const UString input, unsigned start, unsigned length)

Shouldn't this be "const UString&"?