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

RESOLVED FIXED

Status

Tamarin
Tracing Virtual Machine
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Shengnan Cong, Unassigned)

Tracking

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
Created attachment 310078 [details] [diff] [review]
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

10 years ago
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

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

Comment 4

10 years ago
Yes, in fact in avmplus.h we already have:

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

Comment 5

10 years ago
Created attachment 310286 [details] [diff] [review]
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
Attachment #310078 - Attachment is obsolete: true
(Reporter)

Comment 6

10 years ago
Created attachment 310288 [details] [diff] [review]
Here is the patch
Attachment #310286 - Attachment is obsolete: true

Comment 7

10 years ago
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.
(Reporter)

Comment 8

10 years ago
Created attachment 310527 [details] [diff] [review]
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.
Attachment #310288 - Attachment is obsolete: true
(Reporter)

Updated

10 years ago
Attachment #310527 - Flags: review?(treilly)

Updated

10 years ago
Attachment #310527 - Flags: review?(treilly) → review+

Comment 9

10 years ago
Can we generate a diff against tip with hg diff?   Thats how we usually do it.  Thanks!
(Reporter)

Comment 10

10 years ago
Created attachment 312081 [details] [diff] [review]
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.
Attachment #310527 - Attachment is obsolete: true
(Reporter)

Updated

10 years ago
Attachment #312081 - Flags: review?(treilly)

Comment 11

10 years ago
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

(Reporter)

Comment 12

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

>hg tip
changeset:   322:db21d04bc48e
tag:         tip
user:        Edwin Smith <edwsmith@adobe.com>
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

10 years ago
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

10 years ago
pushed in rev 323
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Attachment #312081 - Flags: review?(treilly) → review+
You need to log in before you can comment on or make changes to this bug.