Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2046353002:
Use a Vector for the buffer in StringBuilder. (Closed)
DescriptionUse a Vector for the buffer in StringBuilder.
The string overallocation strategy of StringBuilder doesn't really make sense
since we need to create a copy at the end now that StringImpl::reallocate was
removed by https://codereview.chromium.org/1390033003
We probably should add back an optimization that forcibly sets the length of the
String and uses overallocation to avoid doing a string copy in every StringBuilder,
but until we decide to do that lets switch to using a Vector for the buffer since
it can handle growing the buffer automatically for us and makes the code in
StringBuilder much simpler.
Committed: https://crrev.com/23e9098e1bfaa078916ed64a4c6b6f6bd7b00d1b
Cr-Commit-Position: refs/heads/master@{#402638}
Patch Set 1 #Patch Set 2 : m_is8Bit being false doesn't mean there's a 16bit buffer #Patch Set 3 : Keep m_string alive when creating the buffer. #Patch Set 4 : resize() must adjust buffer too. #Patch Set 5 : rebase. #
Total comments: 12
Messages
Total messages: 35 (14 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/patch-status/2046353002/1
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/patch-status/2046353002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on 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/patch-status/2046353002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on 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/patch-status/2046353002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Use a Vector for the buffer in StringBuilder. BUG= ========== to ========== Use a Vector for the buffer in StringBuilder. The string overallocation strategy of StringBuilder doesn't really make sense since we need to create a copy at the end now that StringImpl::reallocate was removed by https://codereview.chromium.org/1390033003 We probably should add back an optimization that forcibly sets the length of the String and uses overallocation to avoid doing a string copy in every StringBuilder, but until we decide to do that lets switch to using a Vector for the buffer since it can handle growing the buffer automatically for us and makes the code in StringBuilder much simpler. ==========
esprehn@chromium.org changed reviewers: + hajimehoshi@chromium.org, haraken@chromium.org
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...
Description was changed from ========== Use a Vector for the buffer in StringBuilder. The string overallocation strategy of StringBuilder doesn't really make sense since we need to create a copy at the end now that StringImpl::reallocate was removed by https://codereview.chromium.org/1390033003 We probably should add back an optimization that forcibly sets the length of the String and uses overallocation to avoid doing a string copy in every StringBuilder, but until we decide to do that lets switch to using a Vector for the buffer since it can handle growing the buffer automatically for us and makes the code in StringBuilder much simpler. ========== to ========== Use a Vector for the buffer in StringBuilder. The string overallocation strategy of StringBuilder doesn't really make sense since we need to create a copy at the end now that StringImpl::reallocate was removed by https://codereview.chromium.org/1390033003 We probably should add back an optimization that forcibly sets the length of the String and uses overallocation to avoid doing a string copy in every StringBuilder, but until we decide to do that lets switch to using a Vector for the buffer since it can handle growing the buffer automatically for us and makes the code in StringBuilder much simpler. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping
https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringBuilder.h (right): https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringBuilder.h:233: void* m_buffer; Where is |m_buffer| used now?
https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringBuilder.h (right): https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringBuilder.h:233: void* m_buffer; On 2016/06/27 at 06:13:20, hajimehoshi wrote: > Where is |m_buffer| used now? hasBuffer() uses it since it doesn't care about the type. I guess it could just check either of the other two pointers
Mostly looks good. https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringBuilder.cpp (right): https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringBuilder.cpp:86: delete m_buffer8; Can we use OwnPtr and avoid using manual delete? https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringBuilder.cpp:140: DCHECK(m_is8Bit || !hasBuffer()); Slightly better: DCHECK((m_is8Bit && m_buffer8) || !hasBuffer()); https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringBuilder.h (right): https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringBuilder.h:209: typedef Vector<UChar, 16> Buffer16; I don't mind specifying a larger inline capacity for this because most StringBuilders are used on stack. (Honestly speaking, I want to make StringBuilders STACK_ALLOCATED.) It's very rare that StringBuilders are long-lived and the overallocated capacity becomes a problem. https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringBuilder.h:220: if (m_is8Bit || !hasBuffer()) Shouldn't this be: DCHECK(!m_is8Bit); if (!hasBuffer()) ... ?
https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringBuilder.cpp (right): https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringBuilder.cpp:86: delete m_buffer8; On 2016/06/27 06:36:01, haraken wrote: > > Can we use OwnPtr and avoid using manual delete? unique_ptr I think :-)
I would have to not use a union and make it one pointer larger to use unique_ptr. I guess we can make StringBuilder 1 ptr bigger? https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringBuilder.cpp (right): https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringBuilder.cpp:86: delete m_buffer8; On 2016/06/27 at 06:38:02, hajimehoshi wrote: > On 2016/06/27 06:36:01, haraken wrote: > > > > Can we use OwnPtr and avoid using manual delete? > > unique_ptr I think :-) No, you can't store a unique_ptr in a union. So we need to do manual delete. https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringBuilder.cpp:140: DCHECK(m_is8Bit || !hasBuffer()); On 2016/06/27 at 06:36:01, haraken wrote: > Slightly better: > > DCHECK((m_is8Bit && m_buffer8) || !hasBuffer()); I'm not sure what that does, if m_buffer8 is null then we'll just check !hasBuffer() on the other side of the assert which then is !m_buffer8 so the m_buffer8 check you added doesn't do anything, it's the same as: DCHECK((m_is8Bit && m_buffer8) || !m_buffer8) https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringBuilder.h (right): https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringBuilder.h:209: typedef Vector<UChar, 16> Buffer16; On 2016/06/27 at 06:36:01, haraken wrote: > I don't mind specifying a larger inline capacity for this because most StringBuilders are used on stack. (Honestly speaking, I want to make StringBuilders STACK_ALLOCATED.) It's very rare that StringBuilders are long-lived and the overallocated capacity becomes a problem. Yeah we can do that, there's a bunch of StringBuilder m_* in the codebase right now, FetchDataLoaderAsString is an example of one where I'm a bit nervous making this too big. I'd like to set a large capacity in another patch. https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringBuilder.h:220: if (m_is8Bit || !hasBuffer()) On 2016/06/27 at 06:36:01, haraken wrote: > Shouldn't this be: > > DCHECK(!m_is8Bit); > if (!hasBuffer()) > ... > > ? No, this is called whenever we need a 16 bit buffer. We might have already converted to a 16bit buffer though. ex. append(UChar) called twice will invoke ensireBuffer16() twice, the first time we get here we have m_is8Bit true, the second time we have it false. createBuffer16() sets it.
LGTM
lgtm https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringBuilder.cpp (right): https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringBuilder.cpp:86: delete m_buffer8; On 2016/06/27 23:36:40, esprehn wrote: > On 2016/06/27 at 06:38:02, hajimehoshi wrote: > > On 2016/06/27 06:36:01, haraken wrote: > > > > > > Can we use OwnPtr and avoid using manual delete? > > > > unique_ptr I think :-) > > No, you can't store a unique_ptr in a union. So we need to do manual delete. Out of curiosity, isn't it possible to use two unique_ptrs instead of a union? I guess memory usage of this class is critical but not sure.
On 2016/06/28 at 05:01:35, hajimehoshi wrote: > lgtm > > https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/wtf/text/StringBuilder.cpp (right): > > https://codereview.chromium.org/2046353002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/wtf/text/StringBuilder.cpp:86: delete m_buffer8; > On 2016/06/27 23:36:40, esprehn wrote: > > On 2016/06/27 at 06:38:02, hajimehoshi wrote: > > > On 2016/06/27 06:36:01, haraken wrote: > > > > > > > > Can we use OwnPtr and avoid using manual delete? > > > > > > unique_ptr I think :-) > > > > No, you can't store a unique_ptr in a union. So we need to do manual delete. > > Out of curiosity, isn't it possible to use two unique_ptrs instead of a union? I guess memory usage of this class is critical but not sure. Yeah it is, looking at this I'm not sure this is really size critical. Once this patch lands I'll simplify it down to two unique_ptrs, and also look at increasing the inline capacity.
The CQ bit was checked by esprehn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Use a Vector for the buffer in StringBuilder. The string overallocation strategy of StringBuilder doesn't really make sense since we need to create a copy at the end now that StringImpl::reallocate was removed by https://codereview.chromium.org/1390033003 We probably should add back an optimization that forcibly sets the length of the String and uses overallocation to avoid doing a string copy in every StringBuilder, but until we decide to do that lets switch to using a Vector for the buffer since it can handle growing the buffer automatically for us and makes the code in StringBuilder much simpler. ========== to ========== Use a Vector for the buffer in StringBuilder. The string overallocation strategy of StringBuilder doesn't really make sense since we need to create a copy at the end now that StringImpl::reallocate was removed by https://codereview.chromium.org/1390033003 We probably should add back an optimization that forcibly sets the length of the String and uses overallocation to avoid doing a string copy in every StringBuilder, but until we decide to do that lets switch to using a Vector for the buffer since it can handle growing the buffer automatically for us and makes the code in StringBuilder much simpler. Committed: https://crrev.com/23e9098e1bfaa078916ed64a4c6b6f6bd7b00d1b Cr-Commit-Position: refs/heads/master@{#402638} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/23e9098e1bfaa078916ed64a4c6b6f6bd7b00d1b Cr-Commit-Position: refs/heads/master@{#402638} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
