[Webkit-unassigned] [Bug 67081] Basic enhancements to StringBuilder

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67081





--- Comment #18 from Xianzhu Wang <wangxianzhu at chromium.org>  2011-10-09 21:31:31 PST ---
(From update of attachment 110288)
View in context: https://bugs.webkit.org/attachment.cgi?id=110288&action=review

Thanks Darin for review!

Will upload the new patch after I fixed the break on win.

>> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:171
>>      // If the buffer is at least 80% full, don't bother copying. Need to tune this heuristic!
> 
> Comment should be removed from here since it was moved into canShrink.

Done.

>> Source/JavaScriptCore/wtf/text/StringBuilder.h:118
>> +        return AtomicString(toStringPreserveCapacity());
> 
> I don’t understand the canShrink logic here. That function determines if it’s a good idea to just use the buffer rather than creating a new one since it’s already mostly full. But here, the code says “if the buffer is already full then don’t use it”, which seems like the opposite of what canShrink is supposed to mean. If this is a good algorithm, then we need a comment explaining why.
> 
> The if (!m_string.isNull) part of this function has no value; it just repeats what toStringPreserveCapacity will already do.
> 
> Your comment said that there is no easy way to optimize the case where the string is already the right size and happens to be in the table without complicating the relationship between AtomicString and StringBuilder, but that does not seem to be the case given the current form of this function.
> 
> If the AtomicString constructor had a version that took a character pointer, a length, and a StringImpl, it could have all the correct logic; StringBuilder::reifyString has nothing in it that is truly specific to StringBuilder policy.

The comment in canShrink() was misleading. I've changed it to "Only shrink the buffer if it's less than 80% full". The code is: if the buffer is over-allocated, then don't use it.

Your idea is great! Thanks.

>> Source/JavaScriptCore/wtf/text/StringBuilder.h:175
>> +    StringBuilder& operator=(const StringBuilder&);
> 
> This is a great idea. In WebKit the normal way to do this is to use WTF_NONCOPYABLE.
> 
> I would word it like this:
> 
>     // Disallow copying since it’s expensive and we don’t want code to do it by accident.

Done.

>> Source/JavaScriptCore/wtf/text/StringBuilder.h:192

> 
> If the StringBuilder is empty, presumably the cost of calling the length function is quite small, so the first check seems to be wasteful.

I added the first check to satisfy the next ASSERT(buffer) in case of equal(StringBuilder(), 0, 0).
Removed along with the ASSERT.

>> Source/JavaScriptCore/wtf/text/StringBuilder.h:195
>> +        return true;
> 
> While this condition is possible in theory, it seems it would never occur in practice. Accordingly, I suggest omitting this.

This happens when a String is appended to each of two new StringBuilders, or a StringBuilder with non-null m_string is just appended to a new StringBuilder. In this case, the m_strings in the StringBuilders share the same impl. So I prefer keep it.

>> Source/JavaScriptCore/wtf/text/StringBuilder.h:196

> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

Done.

>> Source/WebCore/editing/markup.cpp:218
>> +    return string.replace(0, "");
> 
> What’s the rationale for this change? It doesn’t seem that the copy constructor or the assignment operator are relevant here.

This is related to the change of the return type of toString() from 'String' to 'const String&', so here needed an explicit copy.
Will update the Source/WebCore/ChangeLog.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.