Bug 134376

Summary: Remove BufferForAppendingHyphen
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, bunhere, cdumez, commit-queue, darin, esprehn+autocc, glenn, gyuyoung.kim, kling, kondapallykalyan, rniwa, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Patch darin: review+

Description Benjamin Poulain 2014-06-26 19:05:32 PDT
Remove BufferForAppendingHyphen
Comment 1 Benjamin Poulain 2014-06-26 19:09:15 PDT
Created attachment 233954 [details]
Patch
Comment 2 Darin Adler 2014-06-26 19:18:52 PDT
Comment on attachment 233954 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:4
> + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2014 Apple Inc. All rights reserved.

We can use year ranges, so this should be 2004-2011, 2014, or maybe even 2004-2014 if we made changes in 2012 and 2013.

> Source/WebCore/rendering/InlineTextBox.cpp:257
> +    if (!string.isEmpty()) {

Why are you adding this check? The old code doesn’t check for this case. If you are fixing a bug, then should we add a test case for that bug?

> Source/WebCore/rendering/InlineTextBox.cpp:259
> +        string = string + hyphenString;

Why not use string.append(hyphenString) instead of string = string + hyphenString?

Irritating to have to do this memory allocation.

> Source/WebCore/rendering/InlineTextBox.h:-29
> -#include <wtf/text/StringBuilder.h>

Looks like TextIterator.cpp, and maybe other files, now needs to include StringBuilder.h.
Comment 3 Benjamin Poulain 2014-06-26 20:52:01 PDT
(In reply to comment #2)
> > Source/WebCore/rendering/InlineTextBox.cpp:259
> > +        string = string + hyphenString;
> 
> Why not use string.append(hyphenString) instead of string = string + hyphenString?

I'll explain to you IRL. It is not critical, I can cut it.
Comment 4 Benjamin Poulain 2014-06-26 22:38:50 PDT
Created attachment 233964 [details]
Patch
Comment 5 Build Bot 2014-06-27 01:11:33 PDT
Comment on attachment 233964 [details]
Patch

Attachment 233964 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5224078520614912

New failing tests:
fast/text/hyphen-min-preferred-width.html
fast/text/hyphenate-first-word-after-skipped-space.html
Comment 6 Build Bot 2014-06-27 01:11:37 PDT
Created attachment 233971 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 7 Build Bot 2014-06-27 01:36:40 PDT
Comment on attachment 233964 [details]
Patch

Attachment 233964 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4719324543582208

New failing tests:
fast/text/hyphen-min-preferred-width.html
fast/text/hyphenate-first-word-after-skipped-space.html
Comment 8 Build Bot 2014-06-27 01:36:46 PDT
Created attachment 233972 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 9 Benjamin Poulain 2014-06-27 01:44:16 PDT
Created attachment 233973 [details]
Patch
Comment 10 Darin Adler 2014-06-27 10:01:08 PDT
Comment on attachment 233973 [details]
Patch

I don’t understand this new patch that uses a String*. Why is that better than the version where we took in a String&?
Comment 11 Benjamin Poulain 2014-06-27 10:17:23 PDT
(In reply to comment #10)
> (From update of attachment 233973 [details])
> I don’t understand this new patch that uses a String*. Why is that better than the version where we took in a String&?

The problem is StringBuilder is pure overhead in the old code.

The problems I have are:
1) The initial reserveCapacity(256) spend time in malloc in all cases, we never need that buffer.
2) On the call sites of constructTextRun, we have too much overhead due to StringBuilder (2 deref + 2 destructor + more memory manipulation.
3) When we have an hyphen, we have a second "reserveCapacity()" that allocates a string of the wrong type in many cases.
4) The final append() have extra overhead of using StringBuilder.

I noticed the issue because (1) and (2) are hurting us on ARMv7.
Comment 12 Darin Adler 2014-06-27 14:25:22 PDT
Comment on attachment 233973 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        The problem is that StringBuilder's memory was also implicitely used to keep

Typo "implicitly".
Comment 13 Benjamin Poulain 2014-06-27 16:33:33 PDT
Committed r170561: <http://trac.webkit.org/changeset/170561>