Since the function is not inline, the offset forces a bunch of extra operations.
Created attachment 137194 [details] Patch
This is on top of https://bugs.webkit.org/show_bug.cgi?id=83961 You can ignore the bots.
Comment on attachment 137194 [details] Patch Attachment 137194 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12400330
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
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
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
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
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.
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.
Created attachment 138695 [details] Patch for landing
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?
Created attachment 138704 [details] Patch
> > 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.
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.
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
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
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
Created attachment 138885 [details] Patch
Committed r115290: <http://trac.webkit.org/changeset/115290>
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.
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
Sorry for blaming this patch, it is absolutely innocent. :)