Bug 96125 – StringImpl::find(StringImpl*) doesn't handle cases where search and match strings are different bitness

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

[Michael Saboff]

StringImpl::FInd(StringImpl*) handles the case where both strings are 8 bit.  If one string is 16 bit, the other string is unconverted to 16 bit to perform the search.

[Michael Saboff]

Created attachment 162813 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/te..." exit_code: 1
Source/WTF/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


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

[Michael Saboff]

Created attachment 162818 [details]
Patch with Tab character removed from ChangeLog

[Benjamin Poulain]

Comment on attachment 162818 [details]
Patch with Tab character removed from ChangeLog

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

r- for the the other find().

Why not update find(StringImpl* matchString, unsigned index) similarly?

> Source/WTF/wtf/text/StringImpl.cpp:971
> -    while (searchHash != matchHash || memcmp(searchCharacters + i, matchCharacters, matchLength * sizeof(CharType))) {
> +    while (searchHash != matchHash || !equal(searchCharacters + i, matchCharacters, matchLength)) {

This may causes regressions, although that is unlikely for the length we use with find().

[Michael Saboff]

Created attachment 162869 [details]
Patch with changes for both exported find()'s and reverseFind()

(In reply to comment #4)
> (From update of attachment 162818 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162818&action=review
> 
> r- for the the other find().
> 
> Why not update find(StringImpl* matchString, unsigned index) similarly?

Okay - done.  I also added reverseFind().

[Michael Saboff]

(In reply to comment #4)
 
> > Source/WTF/wtf/text/StringImpl.cpp:971
> > -    while (searchHash != matchHash || memcmp(searchCharacters + i, matchCharacters, matchLength * sizeof(CharType))) {
> > +    while (searchHash != matchHash || !equal(searchCharacters + i, matchCharacters, matchLength)) {
> 
> This may causes regressions, although that is unlikely for the length we use with find().

Performance results.  Doesn't seem to be an issue.


Generating benchmark report at /Volumes/Data/src/webkit/BaseLine_96125_SunSpiderV8Real_msaboff-pro_20120907_1415_report.txt
And raw data at /Volumes/Data/src/webkit/BaseLine_96125_SunSpiderV8Real_msaboff-pro_20120907_1415.json

Benchmark report for SunSpider and V8Real on msaboff-pro (MacPro5,1).

VMs tested:
"BaseLine" at /Volumes/Data/src/webkit.baseline/WebKitBuild/Release/DumpRenderTree (r127888)
"96125" at /Volumes/Data/src/webkit.work/WebKitBuild/Release/DumpRenderTree (r127888)

Collected 12 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc() between
sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific
preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence
intervals in milliseconds.

                                         BaseLine                   96125                                       
SunSpider:
   3d-cube                            8.7977+-0.5067            8.6507+-0.5261          might be 1.0170x faster
   3d-morph                           8.1564+-0.0928     ?      8.1984+-0.1211        ?
   3d-raytrace                       12.4149+-0.3862     ?     12.4155+-0.3946        ?
   access-binary-trees                2.4674+-0.4458            2.3497+-0.4525          might be 1.0501x faster
   access-fannkuch                    6.8857+-0.0270     ?      6.9154+-0.0627        ?
   access-nbody                       4.2681+-0.1196            4.2181+-0.0692          might be 1.0119x faster
   access-nsieve                      3.3983+-0.0395     ?      3.4485+-0.0620        ? might be 1.0148x slower
   bitops-3bit-bits-in-byte           1.3664+-0.0103            1.3625+-0.0079        
   bitops-bits-in-byte                5.7576+-0.0217     ?      5.7651+-0.0136        ?
   bitops-bitwise-and                 2.1705+-0.1227            2.0922+-0.1335          might be 1.0374x faster
   bitops-nsieve-bits                 3.5294+-0.0185            3.5262+-0.0237        
   controlflow-recursive              2.5184+-0.0217            2.5136+-0.0307        
   crypto-aes                         9.8818+-0.5973            9.7241+-0.5389          might be 1.0162x faster
   crypto-md5                         3.8916+-0.1044     ?      3.9928+-0.1031        ? might be 1.0260x slower
   crypto-sha1                        3.1377+-0.0400            3.1106+-0.0284        
   date-format-tofte                 14.2957+-0.6866     ?     14.9089+-1.0965        ? might be 1.0429x slower
   date-format-xparb                 11.5976+-0.1341     ?     11.7902+-0.2157        ? might be 1.0166x slower
   math-cordic                        4.2203+-0.0755     ?      4.2681+-0.0663        ? might be 1.0113x slower
   math-partial-sums                 10.1748+-0.0727           10.1726+-0.0382        
   math-spectral-norm                 3.0190+-0.0195     ?      3.0290+-0.0179        ?
   regexp-dna                        10.4678+-0.3456     ?     10.4715+-0.3451        ?
   string-base64                      6.1099+-0.5521     ?      6.1596+-0.5285        ?
   string-fasta                       8.2396+-0.2519     ?      8.5118+-0.4220        ? might be 1.0330x slower
   string-tagcloud                   13.7735+-0.3097           13.7045+-0.3052        
   string-unpack-code                23.8898+-0.5114           23.7975+-0.6630        
   string-validate-input              8.7721+-0.5857     ?      9.1247+-0.4853        ? might be 1.0402x slower

   <arithmetic> *                     7.4308+-0.1244     ?      7.4701+-0.1598        ? might be 1.0053x slower
   <geometric>                        5.9255+-0.0953     ?      5.9367+-0.1142        ? might be 1.0019x slower
   <harmonic>                         4.6523+-0.0844            4.6334+-0.0915          might be 1.0041x faster

                                         BaseLine                   96125                                       
V8Real:
   encrypt                           0.40235+-0.00059    ?     0.40247+-0.00067       ?
   decrypt                           6.94680+-0.01378    ?     6.96763+-0.03970       ?
   deltablue                x2       0.56615+-0.00542    ?     0.57171+-0.00694       ?
   earley                            1.23551+-0.00667    ?     1.24194+-0.01050       ?
   boyer                            13.70504+-0.05580         13.66802+-0.07235       
   raytrace                 x2       4.54731+-0.05376    ?     4.60798+-0.05023       ? might be 1.0133x slower
   regexp                   x2      26.80334+-0.12066    ?    26.91426+-0.17851       ?
   richards                 x2       0.27124+-0.00194          0.27095+-0.00141       
   splay                    x2       0.75214+-0.00602          0.74667+-0.00341       
   navier-stokes            x2      12.75767+-0.00745    ^    12.73450+-0.01316       ^ definitely 1.0018x faster

   <arithmetic>                      7.10534+-0.01566    ?     7.12326+-0.02639       ? might be 1.0025x slower
   <geometric> *                     2.43480+-0.00627    ?     2.44082+-0.00531       ? might be 1.0025x slower
   <harmonic>                        0.90144+-0.00387    ?     0.90232+-0.00223       ? might be 1.0010x slower

                                         BaseLine                   96125                                       
All benchmarks:
   <arithmetic>                       7.3068+-0.0794     ?      7.3380+-0.1016        ? might be 1.0043x slower
   <geometric>                        4.2223+-0.0425     ?      4.2311+-0.0501        ? might be 1.0021x slower
   <harmonic>                         1.7994+-0.0095            1.7989+-0.0087          might be 1.0003x faster

                                         BaseLine                   96125                                       
Geomean of preferred means:
   <scaled-result>                   13.4499+-0.1172     ?     13.5012+-0.1431        ? might be 1.0038x slower

[Benjamin Poulain]

> Performance results.  Doesn't seem to be an issue.
>                   
> Geomean of preferred means:
>    <scaled-result>                   13.4499+-0.1172     ?     13.5012+-0.1431        ? might be 1.0038x slower

Aren't many the string-test showing as slower?
Just an issue for that particular run?

[Michael Saboff]

(In reply to comment #7)
> > Performance results.  Doesn't seem to be an issue.
> >                   
> > Geomean of preferred means:
> >    <scaled-result>                   13.4499+-0.1172     ?     13.5012+-0.1431        ? might be 1.0038x slower
> 
> Aren't many the string-test showing as slower?
> Just an issue for that particular run?

The differences are all within the confidence interval, that's why I don't think there are any slowdowns.

[Benjamin Poulain]

Comment on attachment 162869 [details]
Patch with changes for both exported find()'s and reverseFind()

> The differences are all within the confidence interval, that's why I don't think there are any slowdowns.

Fair enough.

[WebKit Review Bot]

Comment on attachment 162869 [details]
Patch with changes for both exported find()'s and reverseFind()

Clearing flags on attachment: 162869

Committed r127928: <http://trac.webkit.org/changeset/127928>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.