Bug 83968 – Add a version of StringImpl::find() without offset

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Benjamin Poulain]

Since the function is not inline, the offset forces a bunch of extra operations.

[Benjamin Poulain]

Created attachment 137194 [details]
Patch

[Benjamin Poulain]

This is on top of https://bugs.webkit.org/show_bug.cgi?id=83961
You can ignore the bots.

[Build Bot]

Comment on attachment 137194 [details]
Patch

Attachment 137194 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12400330

[WebKit Review Bot]

Comment on attachment 137194 [details]
Patch

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

New failing tests:
fast/xpath/substring-after.html
fast/xpath/4XPath/Core/test_core_functions.html

[WebKit Review Bot]

Created attachment 137199 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 137194 [details]
Patch

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

New failing tests:
fast/xpath/substring-after.html
fast/xpath/4XPath/Core/test_core_functions.html

[WebKit Review Bot]

Created attachment 137200 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Sam Weinig]

Comment on attachment 137194 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        This gives a 12% gains for strings of distribution of strings between 30 and 100 characters.

I don't completely understand this sentence.

> Source/WTF/ChangeLog:11
> +        By not havig the index, we can skip a couple of branches and a few instructions. This gives

Typo: havig -> having.

> Source/WTF/ChangeLog:14
> +        The case of empty string is moved bellow (matchLength == 1) since it is a less common operation.

Typo: bellow -> below.  

How did you determine that finding on the empty string is less common?

> Source/WTF/wtf/text/StringImpl.cpp:901
> +    // Check for null or empty string to match against
> +    if (UNLIKELY(!matchString))
> +        return notFound;

I don't think this checks for empty strings.

[Benjamin Poulain]

Thanks for the review. Sorry I butchered that ChangeLog.

> > Source/WTF/ChangeLog:14
> > +        The case of empty string is moved bellow (matchLength == 1) since it is a less common operation.
> 
> How did you determine that finding on the empty string is less common?

It is actually just an assumption. The case of length==1 is pretty common from JavaScript.

[Benjamin Poulain]

Created attachment 138695 [details]
Patch for landing

[Darin Adler]

Comment on attachment 138695 [details]
Patch for landing

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

> Source/JavaScriptCore/runtime/StringPrototype.cpp:781
> +        if (pos)
> +            result = s.find(u2, pos);
> +        else
> +            result = s.find(u2);

I don’t get how this could be an improvement. Can’t the normal find just optimize just as well for the case where pos is 0 as an if statement can?

[Benjamin Poulain]

Created attachment 138704 [details]
Patch

[Benjamin Poulain]

> > Source/JavaScriptCore/runtime/StringPrototype.cpp:781
> > +        if (pos)
> > +            result = s.find(u2, pos);
> > +        else
> > +            result = s.find(u2);
> 
> I don’t get how this could be an improvement. Can’t the normal find just optimize just as well for the case where pos is 0 as an if statement can?

Yep, I was divided on that one...

It was showing improvement on JavaScript because the source strings are big enough on average. I did not want to risk slowing down something else with the extra branch and function call.

Since you also think that does not look great, I removed that from the latest version. I can change this later if needed.

[Benjamin Poulain]

Comment on attachment 138704 [details]
Patch

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

> Source/JavaScriptCore/runtime/StringPrototype.cpp:764
>      if (a1.isUndefined())
> -        pos = 0;
> -    else if (a1.isUInt32())
> -        pos = min<uint32_t>(a1.asUInt32(), len);
> +        result = s.find(u2);

This is the best case for this patch, and it is also very common, so I am fine sticking with this.

[Benjamin Poulain]

Finally solved the symbol issue. Sam, would you mind looking at the changes and confirm it is still ok to land:
-removed the if(pos==0) following Darin comment
-added the new symbol for WebKit2 Windows

[WebKit Review Bot]

Comment on attachment 138704 [details]
Patch

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

New failing tests:
fast/xpath/substring-after.html
fast/xpath/4XPath/Core/test_core_functions.html

[WebKit Review Bot]

Created attachment 138787 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Benjamin Poulain]

Created attachment 138885 [details]
Patch

[Benjamin Poulain]

Committed r115290: <http://trac.webkit.org/changeset/115290>

[Csaba Osztrogonác]

Reopen, because http://trac.webkit.org/changeset/115290 broke the Qt Mac build.
Bot unfortunately I can't paste build log, because build.webkit.org is died at the moment.

[Csaba Osztrogonác]

build.webkit.org works again, so here is the build log:
../../../../Source/WebCore/bridge/qt/qt_runtime.cpp: In function ‘bool JSC::Bindings::isJSUint8ClampedArray(JSC::JSValue)’:
../../../../Source/WebCore/bridge/qt/qt_runtime.cpp:142: error: ‘JSUint8ClampedArray’ is not a class or namespace

[Csaba Osztrogonác]

Sorry for blaming this patch, it is absolutely innocent. :)