423519 - doubling TT performance on Sunspider string-validate-input by using SSE2

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, defect)

Description

[Shengnan Cong]

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; InfoPath.1; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.04506.648; .NET CLR 3.5.21022)
Build Identifier: 

The TT VM method String::testSingleByte is the hot spot of Sunspider benchmark string-validate-input. The method involves a loop that scans a given string looking for a byte whose most significant bit is set. By using the SSE2 version of this loop (actually simple to implement in C++ with MSVC intrinsic), the performance of TT on this benhmark almost doubles. The SSE2 code is guarded by a proper check to kick in only when the length of the string is larger than a threshold. The implementation has no negative impact on other cases involving short strings.



Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Shengnan Cong]

Attachment: sse2 version of method String::testSingleByte

gc->core()->sse_opt should be accessable in 
String::testSingleByte. This patch does not build with Release since gc->core() is not avaialble.

Comment 2

[Tommy Reilly]

You want to pass an AvmCore * down through the callers of testSingleByte, I would refactor the String ctor's to take a core instead and they can get the GC from that.   Also the intrinsic includes should be in ifdef for IA32 and MSVC.  

Comment 3

[Robert Sayre]

we should be able to use these intrinsics on macintel as well, right?

Comment 4

[Tommy Reilly]

Yes, in fact in avmplus.h we already have:

#if defined(_MAC) && (defined (AVMPLUS_IA32) || defined(AVMPLUS_AMD64))
#include <emmintrin.h>
#endif

Comment 5

[Shengnan Cong]

Attachment: Refactoring the code according to Tommy Reilly 's comments

- Refactor the String constructor to take a core instead of gc 
- Pass an AvmCore * down through the callers of testSingleByte
- Guard the SSE2 code and the header files in ifdef

Comment 6

[Shengnan Cong]

Attachment: Here is the patch

Comment 7

[Tommy Reilly]

Getting there...

#if defined (AVMPLUS_IA32)
 bufa = (utf8_t *)((uint32_t) buf & 0xFFFFFFF0)+16;
#else
 bufa = (utf8_t *)((uint64_t) buf & 0xFFFFFFF0)+16;
#endif

Could be:

 bufa = (utf8_t *)((uintptr_t) buf & 0xFFFFFFF0)+16;

I'm not sure about the mac intrinsic header names, have this been tested on OS X?


We lost an important String ctor inline, I guess that's because we need to call AvmCore::GetGC().  Oh well, that's fine I guess.  Actually setBuff isn't an inline so you could pass AvmCore* to it and leave the String ctor inlined.

Comment 8

[Shengnan Cong]

Attachment: refactor testSingleByte

Refactored testSingleByte() using uintptr_t instead of uint32_t/uint64_t.
Tested on OS X and there is no problem with the intrinsic header names.

About the inline of String ctor, since the String ctor is only called within StringObject.cpp, adding keyword inline to the ctor should work. 

Actually, just checked the asm in VS, the String ctor is not inlined anyway, neither as the orignal(the ctor in .h), nor using inline keyword in .cpp.

Comment 9

[Tommy Reilly]

Can we generate a diff against tip with hg diff?   Thats how we usually do it.  Thanks!

Comment 10

[Shengnan Cong]

Attachment: Diff against tip

Just generated a diff against tip using "hg diff -r tip". Let me know if it is not what you want. I am still new to hg... Thanks.

Comment 11

[Tommy Reilly]

Sure you did a pull/update before doing hg diff?

treilly:tt-string treilly$ curl https://bugzilla.mozilla.org/attachment.cgi?id=312081 | hg import -
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  5652  100  5652    0     0  26654      0 --:--:-- --:--:-- --:--:-- 1839k
applying patch from stdin
patching file core/StringObject.cpp
Hunk #1 FAILED at 37
Hunk #2 FAILED at 72
Hunk #3 FAILED at 96
Hunk #4 FAILED at 109
Hunk #5 FAILED at 121
Hunk #6 FAILED at 220
Hunk #7 FAILED at 253
7 out of 7 hunks FAILED -- saving rejects to file core/StringObject.cpp.rej
patching file core/StringObject.h
Hunk #1 FAILED at 154
Hunk #2 FAILED at 163
Hunk #3 FAILED at 348
3 out of 3 hunks FAILED -- saving rejects to file core/StringObject.h.rej
patching file core/avmplus.h
Hunk #1 FAILED at 97
1 out of 1 hunk FAILED -- saving rejects to file core/avmplus.h.rej
abort: patch failed to apply

Comment 12

[Shengnan Cong]

I did execute pull/update before the diff. Did I miss anything? Thanks!

>hg tip
changeset:   322:db21d04bc48e
tag:         tip
user:        Edwin Smith <[e-mail]>
date:        Tue Mar 18 10:14:38 2008 -0400
summary:     MinBuild fix

>hg pull
pulling from http://hg.mozilla.org/tamarin-tracing/
searching for changes
no changes found

>hg update
0 files updated, 0 files merged, 0 files removed, 0 files unresolved

>hg diff -r tip > patch.txt

Comment 13

[Tommy Reilly]

Never mind, I was getting tripped up by the windows style line endings (import diff on mac).    Got it now, I'll push it soon.

Comment 14

[Tommy Reilly]

pushed in rev 323