Bug 134376 – Remove BufferForAppendingHyphen

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

Remove BufferForAppendingHyphen

[Benjamin Poulain]

Created attachment 233954 [details]
Patch

[Darin Adler]

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.

[Benjamin Poulain]

(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.

[Benjamin Poulain]

Created attachment 233964 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Benjamin Poulain]

Created attachment 233973 [details]
Patch

[Darin Adler]

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&?

[Benjamin Poulain]

(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.

[Darin Adler]

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".

[Benjamin Poulain]

Committed r170561: <http://trac.webkit.org/changeset/170561>