Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2315853002:
Massively simplify WTF's StringConcatenate (Closed)
DescriptionMassively simplify WTF's StringConcatenate
This does a number of simplifications to remove most of the StringConcatenate
code:
- Delegate const char* to const LChar* so we don't have code duplication.
- Use StringImpl::copyChars instead of manual loops. This is less code and
will also use memcpy when possible so it's much faster.
- Remove the Vector versions of StringTypeAdapter, they aren't used, and doing
string + vector is very weird, I don't think we need to support this.
- Add missing const to everything, but also store the String values and adapters
by value instead of refs. This avoids any issues with temporary strings.
- Merge StringOperators into StringConcatenate.
- Use CheckedNumeric instead of the manual sumWithOverflow. This code was bad
since it made string1 + string2 return a nullptr when there was overflow which
was likely to crash since code assumes that string concat will never result in
null. We now crash gracefully inside ValueOrDie() instead when this happens.
- Make all related classes STACK_ALLOCATED() instead of DISALLOW_NEW(), the
classes should never be members in heap objects.
- Keep the StringTypeAdapters as a member in StringAppend, then keep the adapters
as members. Then I merged makeString into StringAppend reusing the is8Bit and
writeTo logic removing lots of code.
NOTE: This patch removes the ability to do string + non-const-UChar* or
non-const-LChar*. In practice this is never done, and the simplification
seems worth it.
Patch Set 1 #Patch Set 2 : no really. #Patch Set 3 : missing inlines. #
Total comments: 4
Messages
Total messages: 24 (16 generated)
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Clean up StringConcatenate.h BUG= ========== to ========== Massively simplify WTF's StringConcatenate This does a number of simplifications to remove most of the StringConcatenate code: - Use StringImpl::copyChars instead of manual loops. This is less code and will also use memcpy when possible so it's much faster. - Remove the Vector versions of StringTypeAdapter, they aren't used, and doing string + vector is very weird, I don't think we need to support this. - Add missing const to everything, but also store the String values and adapters by value instead of refs. This avoids any issues with temporary strings. - Merge StringOperators into StringConcatenate. ==========
Description was changed from ========== Massively simplify WTF's StringConcatenate This does a number of simplifications to remove most of the StringConcatenate code: - Use StringImpl::copyChars instead of manual loops. This is less code and will also use memcpy when possible so it's much faster. - Remove the Vector versions of StringTypeAdapter, they aren't used, and doing string + vector is very weird, I don't think we need to support this. - Add missing const to everything, but also store the String values and adapters by value instead of refs. This avoids any issues with temporary strings. - Merge StringOperators into StringConcatenate. ========== to ========== Massively simplify WTF's StringConcatenate This does a number of simplifications to remove most of the StringConcatenate code: - Use StringImpl::copyChars instead of manual loops. This is less code and will also use memcpy when possible so it's much faster. - Remove the Vector versions of StringTypeAdapter, they aren't used, and doing string + vector is very weird, I don't think we need to support this. - Add missing const to everything, but also store the String values and adapters by value instead of refs. This avoids any issues with temporary strings. - Merge StringOperators into StringConcatenate. - Delegate const char* to const LChar* so we don't have code duplication. - Use CheckedNumeric instead of the manual sumWithOverflow. This code was bad since it made string1 + string2 return a nullptr when there was overflow which was likely to crash since code assumes that string concat will never result in null. We now crash gracefully inside ValueOrDie() instead when this happens. ==========
Description was changed from ========== Massively simplify WTF's StringConcatenate This does a number of simplifications to remove most of the StringConcatenate code: - Use StringImpl::copyChars instead of manual loops. This is less code and will also use memcpy when possible so it's much faster. - Remove the Vector versions of StringTypeAdapter, they aren't used, and doing string + vector is very weird, I don't think we need to support this. - Add missing const to everything, but also store the String values and adapters by value instead of refs. This avoids any issues with temporary strings. - Merge StringOperators into StringConcatenate. - Delegate const char* to const LChar* so we don't have code duplication. - Use CheckedNumeric instead of the manual sumWithOverflow. This code was bad since it made string1 + string2 return a nullptr when there was overflow which was likely to crash since code assumes that string concat will never result in null. We now crash gracefully inside ValueOrDie() instead when this happens. ========== to ========== Massively simplify WTF's StringConcatenate This does a number of simplifications to remove most of the StringConcatenate code: - Delegate const char* to const LChar* so we don't have code duplication. - Use StringImpl::copyChars instead of manual loops. This is less code and will also use memcpy when possible so it's much faster. - Remove the Vector versions of StringTypeAdapter, they aren't used, and doing string + vector is very weird, I don't think we need to support this. - Add missing const to everything, but also store the String values and adapters by value instead of refs. This avoids any issues with temporary strings. - Merge StringOperators into StringConcatenate. - Use CheckedNumeric instead of the manual sumWithOverflow. This code was bad since it made string1 + string2 return a nullptr when there was overflow which was likely to crash since code assumes that string concat will never result in null. We now crash gracefully inside ValueOrDie() instead when this happens. - Make all related classes STACK_ALLOCATED() instead of DISALLOW_NEW(), the classes should never be members in heap objects. ==========
Description was changed from ========== Massively simplify WTF's StringConcatenate This does a number of simplifications to remove most of the StringConcatenate code: - Delegate const char* to const LChar* so we don't have code duplication. - Use StringImpl::copyChars instead of manual loops. This is less code and will also use memcpy when possible so it's much faster. - Remove the Vector versions of StringTypeAdapter, they aren't used, and doing string + vector is very weird, I don't think we need to support this. - Add missing const to everything, but also store the String values and adapters by value instead of refs. This avoids any issues with temporary strings. - Merge StringOperators into StringConcatenate. - Use CheckedNumeric instead of the manual sumWithOverflow. This code was bad since it made string1 + string2 return a nullptr when there was overflow which was likely to crash since code assumes that string concat will never result in null. We now crash gracefully inside ValueOrDie() instead when this happens. - Make all related classes STACK_ALLOCATED() instead of DISALLOW_NEW(), the classes should never be members in heap objects. ========== to ========== Massively simplify WTF's StringConcatenate This does a number of simplifications to remove most of the StringConcatenate code: - Delegate const char* to const LChar* so we don't have code duplication. - Use StringImpl::copyChars instead of manual loops. This is less code and will also use memcpy when possible so it's much faster. - Remove the Vector versions of StringTypeAdapter, they aren't used, and doing string + vector is very weird, I don't think we need to support this. - Add missing const to everything, but also store the String values and adapters by value instead of refs. This avoids any issues with temporary strings. - Merge StringOperators into StringConcatenate. - Use CheckedNumeric instead of the manual sumWithOverflow. This code was bad since it made string1 + string2 return a nullptr when there was overflow which was likely to crash since code assumes that string concat will never result in null. We now crash gracefully inside ValueOrDie() instead when this happens. - Make all related classes STACK_ALLOCATED() instead of DISALLOW_NEW(), the classes should never be members in heap objects. - Keep the StringTypeAdapters as a member in StringAppend, then keep the adapters as members. Then I merged makeString into StringAppend reusing the is8Bit and writeTo logic removing lots of code. ==========
esprehn@chromium.org changed reviewers: + haraken@chromium.org, jyasskin@chromium.org, yutak@chromium.org
Description was changed from ========== Massively simplify WTF's StringConcatenate This does a number of simplifications to remove most of the StringConcatenate code: - Delegate const char* to const LChar* so we don't have code duplication. - Use StringImpl::copyChars instead of manual loops. This is less code and will also use memcpy when possible so it's much faster. - Remove the Vector versions of StringTypeAdapter, they aren't used, and doing string + vector is very weird, I don't think we need to support this. - Add missing const to everything, but also store the String values and adapters by value instead of refs. This avoids any issues with temporary strings. - Merge StringOperators into StringConcatenate. - Use CheckedNumeric instead of the manual sumWithOverflow. This code was bad since it made string1 + string2 return a nullptr when there was overflow which was likely to crash since code assumes that string concat will never result in null. We now crash gracefully inside ValueOrDie() instead when this happens. - Make all related classes STACK_ALLOCATED() instead of DISALLOW_NEW(), the classes should never be members in heap objects. - Keep the StringTypeAdapters as a member in StringAppend, then keep the adapters as members. Then I merged makeString into StringAppend reusing the is8Bit and writeTo logic removing lots of code. ========== to ========== Massively simplify WTF's StringConcatenate This does a number of simplifications to remove most of the StringConcatenate code: - Delegate const char* to const LChar* so we don't have code duplication. - Use StringImpl::copyChars instead of manual loops. This is less code and will also use memcpy when possible so it's much faster. - Remove the Vector versions of StringTypeAdapter, they aren't used, and doing string + vector is very weird, I don't think we need to support this. - Add missing const to everything, but also store the String values and adapters by value instead of refs. This avoids any issues with temporary strings. - Merge StringOperators into StringConcatenate. - Use CheckedNumeric instead of the manual sumWithOverflow. This code was bad since it made string1 + string2 return a nullptr when there was overflow which was likely to crash since code assumes that string concat will never result in null. We now crash gracefully inside ValueOrDie() instead when this happens. - Make all related classes STACK_ALLOCATED() instead of DISALLOW_NEW(), the classes should never be members in heap objects. - Keep the StringTypeAdapters as a member in StringAppend, then keep the adapters as members. Then I merged makeString into StringAppend reusing the is8Bit and writeTo logic removing lots of code. NOTE: This patch loses the ability to do string + non-const-UChar* or non-const-LChar*. In practice this is never done, and the simplification seems worth it. ==========
Description was changed from ========== Massively simplify WTF's StringConcatenate This does a number of simplifications to remove most of the StringConcatenate code: - Delegate const char* to const LChar* so we don't have code duplication. - Use StringImpl::copyChars instead of manual loops. This is less code and will also use memcpy when possible so it's much faster. - Remove the Vector versions of StringTypeAdapter, they aren't used, and doing string + vector is very weird, I don't think we need to support this. - Add missing const to everything, but also store the String values and adapters by value instead of refs. This avoids any issues with temporary strings. - Merge StringOperators into StringConcatenate. - Use CheckedNumeric instead of the manual sumWithOverflow. This code was bad since it made string1 + string2 return a nullptr when there was overflow which was likely to crash since code assumes that string concat will never result in null. We now crash gracefully inside ValueOrDie() instead when this happens. - Make all related classes STACK_ALLOCATED() instead of DISALLOW_NEW(), the classes should never be members in heap objects. - Keep the StringTypeAdapters as a member in StringAppend, then keep the adapters as members. Then I merged makeString into StringAppend reusing the is8Bit and writeTo logic removing lots of code. NOTE: This patch loses the ability to do string + non-const-UChar* or non-const-LChar*. In practice this is never done, and the simplification seems worth it. ========== to ========== Massively simplify WTF's StringConcatenate This does a number of simplifications to remove most of the StringConcatenate code: - Delegate const char* to const LChar* so we don't have code duplication. - Use StringImpl::copyChars instead of manual loops. This is less code and will also use memcpy when possible so it's much faster. - Remove the Vector versions of StringTypeAdapter, they aren't used, and doing string + vector is very weird, I don't think we need to support this. - Add missing const to everything, but also store the String values and adapters by value instead of refs. This avoids any issues with temporary strings. - Merge StringOperators into StringConcatenate. - Use CheckedNumeric instead of the manual sumWithOverflow. This code was bad since it made string1 + string2 return a nullptr when there was overflow which was likely to crash since code assumes that string concat will never result in null. We now crash gracefully inside ValueOrDie() instead when this happens. - Make all related classes STACK_ALLOCATED() instead of DISALLOW_NEW(), the classes should never be members in heap objects. - Keep the StringTypeAdapters as a member in StringAppend, then keep the adapters as members. Then I merged makeString into StringAppend reusing the is8Bit and writeTo logic removing lots of code. NOTE: This patch loses the ability to do string + non-const-UChar* or non-const-LChar*. In practice this is never done, and the simplification seems worth it. ==========
Description was changed from ========== Massively simplify WTF's StringConcatenate This does a number of simplifications to remove most of the StringConcatenate code: - Delegate const char* to const LChar* so we don't have code duplication. - Use StringImpl::copyChars instead of manual loops. This is less code and will also use memcpy when possible so it's much faster. - Remove the Vector versions of StringTypeAdapter, they aren't used, and doing string + vector is very weird, I don't think we need to support this. - Add missing const to everything, but also store the String values and adapters by value instead of refs. This avoids any issues with temporary strings. - Merge StringOperators into StringConcatenate. - Use CheckedNumeric instead of the manual sumWithOverflow. This code was bad since it made string1 + string2 return a nullptr when there was overflow which was likely to crash since code assumes that string concat will never result in null. We now crash gracefully inside ValueOrDie() instead when this happens. - Make all related classes STACK_ALLOCATED() instead of DISALLOW_NEW(), the classes should never be members in heap objects. - Keep the StringTypeAdapters as a member in StringAppend, then keep the adapters as members. Then I merged makeString into StringAppend reusing the is8Bit and writeTo logic removing lots of code. NOTE: This patch loses the ability to do string + non-const-UChar* or non-const-LChar*. In practice this is never done, and the simplification seems worth it. ========== to ========== Massively simplify WTF's StringConcatenate This does a number of simplifications to remove most of the StringConcatenate code: - Delegate const char* to const LChar* so we don't have code duplication. - Use StringImpl::copyChars instead of manual loops. This is less code and will also use memcpy when possible so it's much faster. - Remove the Vector versions of StringTypeAdapter, they aren't used, and doing string + vector is very weird, I don't think we need to support this. - Add missing const to everything, but also store the String values and adapters by value instead of refs. This avoids any issues with temporary strings. - Merge StringOperators into StringConcatenate. - Use CheckedNumeric instead of the manual sumWithOverflow. This code was bad since it made string1 + string2 return a nullptr when there was overflow which was likely to crash since code assumes that string concat will never result in null. We now crash gracefully inside ValueOrDie() instead when this happens. - Make all related classes STACK_ALLOCATED() instead of DISALLOW_NEW(), the classes should never be members in heap objects. - Keep the StringTypeAdapters as a member in StringAppend, then keep the adapters as members. Then I merged makeString into StringAppend reusing the is8Bit and writeTo logic removing lots of code. NOTE: This patch removes the ability to do string + non-const-UChar* or non-const-LChar*. In practice this is never done, and the simplification seems worth it. ==========
Looks mostly okay. https://codereview.chromium.org/2315853002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringConcatenate.h (right): https://codereview.chromium.org/2315853002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringConcatenate.h:145: const String m_buffer; I don't think store-as-value is an improvement, unless the original code was doing something bad. The new code costs an unnecessary ref churn, which should be avoided in the common code like this. Your change also invalidates an existing test with no good reason. What do you mean by "any issues with temporary strings" in your change description?
https://codereview.chromium.org/2315853002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringConcatenate.h (right): https://codereview.chromium.org/2315853002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringConcatenate.h:145: const String m_buffer; On 2016/09/07 at 10:25:39, Yuta Kitamura wrote: > I don't think store-as-value is an improvement, unless the original > code was doing something bad. The new code costs an unnecessary ref > churn, which should be avoided in the common code like this. Note that the StringAppend class has always stored the strings by value with: StringType1 m_string1; StringType2 m_string2; which was ref churn. I made the adapters store by value, and now store the adapters inside StringAppend directly so that's the same number of refs. The extra refs come from making the combining StringTypeAdapter<StringAppend<StringType1, StringType2>> store the StringAppend by value which has to copy the tree. That was a const ref before, but I keep hitting use after frees from it. Note that previously we were creating the StringTypeAdapters repeatedly inside the process, each instance over a const char* would then invoke strlen again in the out of line constructor, which was many branches to count the length of the string. We were also manually copying the bytes inside StringTypeAdapter<String>::writeTo instead of using memcpy which this patches does by way of copyChars. That also saves lots of branches. So overall I think this patch is probably reducing the number of branches. > > Your change also invalidates an existing test with no good reason. What test? Do you mean the StringOperators one? That test has been disabled for several years apparently, I just renamed and moved it. > > What do you mean by "any issues with temporary strings" in your > change description? That might be wrong, it might just be the StringAppend instances. I've been trying to get this patch or some variant working for several weeks, and I keep hitting various use after frees because the temporaries inside operator+ chains have been problematic. ex. https://codereview.chromium.org/2245773002 There I almost got it right, but MSVC disagrees. I can try again, it's been rather frustrating though. :)
https://codereview.chromium.org/2315853002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringConcatenate.h (right): https://codereview.chromium.org/2315853002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringConcatenate.h:145: const String m_buffer; On 2016/09/07 16:21:26, esprehn wrote: > On 2016/09/07 at 10:25:39, Yuta Kitamura wrote: > > I don't think store-as-value is an improvement, unless the original > > code was doing something bad. The new code costs an unnecessary ref > > churn, which should be avoided in the common code like this. > > Note that the StringAppend class has always stored the strings by value with: > StringType1 m_string1; > StringType2 m_string2; > > which was ref churn. I made the adapters store by value, and now store the > adapters inside StringAppend directly so that's the same number of refs. The > extra refs come from making the combining > StringTypeAdapter<StringAppend<StringType1, StringType2>> store the StringAppend > by value which has to copy the tree. That was a const ref before, but I keep > hitting use after frees from it. > > Note that previously we were creating the StringTypeAdapters repeatedly inside > the process, each instance over a const char* would then invoke strlen again in > the out of line constructor, which was many branches to count the length of the > string. We were also manually copying the bytes inside > StringTypeAdapter<String>::writeTo instead of using memcpy which this patches > does by way of copyChars. That also saves lots of branches. > > So overall I think this patch is probably reducing the number of branches. > > > > > Your change also invalidates an existing test with no good reason. > > What test? Do you mean the StringOperators one? That test has been disabled for > several years apparently, I just renamed and moved it. > > > > > What do you mean by "any issues with temporary strings" in your > > change description? > > That might be wrong, it might just be the StringAppend instances. I've been > trying to get this patch or some variant working for several weeks, and I keep > hitting various use after frees because the temporaries inside operator+ chains > have been problematic. ex. > https://codereview.chromium.org/2245773002 > > There I almost got it right, but MSVC disagrees. I can try again, it's been > rather frustrating though. :) LLVM has a type called "Twine" that stores references to its arguments in order to facilitate concatenation: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/Twine.h?revis... It probably is slightly faster than storing the tree by value, but it's also more fiddly and increases the risk of use-after-frees. The refcount churn is much less significant for WTF::Strings than it would be for a thread-safe string.
I talked to jeffery offline about this, and he thinks this is probably an improvement overall since I save two calls to strlen by making the calls inline so the compiler can do it, and also switch to memcpy for all of the string copying. Want to take another look? :)
LGTM on my side.
LGTM in landing this, but I still wonder why const ref used to work and it doesn't with this change. It may be good to split this patch into smaller ones so the issue can be spotted more easily. https://codereview.chromium.org/2315853002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringConcatenate.h (right): https://codereview.chromium.org/2315853002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringConcatenate.h:145: const String m_buffer; Ah sorry, I wasn't aware the tests were disabled already.
On 2016/09/08 at 05:09:31, yutak wrote: > LGTM in landing this, but I still wonder why const ref used > to work and it doesn't with this change. It may be good > to split this patch into smaller ones so the issue can be > spotted more easily. Yeah that's totally fair, I'll split it up. :) > > https://codereview.chromium.org/2315853002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/wtf/text/StringConcatenate.h (right): > > https://codereview.chromium.org/2315853002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/wtf/text/StringConcatenate.h:145: const String m_buffer; > Ah sorry, I wasn't aware the tests were disabled already. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
