Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1391153004:
Make StringImpl's content immutable (Closed)
|
Created:
5 years, 2 months ago by hajimehoshi Modified:
5 years, 2 months ago Reviewers:
haraken CC:
chromium-reviews, blink-reviews, blink-reviews-wtf_chromium.org, blink-reviews-bindings_chromium.org, Mikhail Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake StringImpl's content immutable
StringImpl::truncateAssumingIsolated is the last thing to make
StringImpl mutable. This CL removes this function to improve code
health. Instead of truncateAssumingIsolate, this CL uses StringImpl::
substring at StringImpl::caseConvert and StringBuilder::reifyString
and this may cause performance regression. I'll revert this when I find
this a serious problem.
BUG=n/a
TEST=wtf_unittests --gtest_filter=WTF.*
Committed: https://crrev.com/06334c75e3417d0a4d0cb7c5565e3ac40c3940cc
Cr-Commit-Position: refs/heads/master@{#353996}
Patch Set 1 #
Total comments: 9
Patch Set 2 : haraken's review #Patch Set 3 : bug fix: StringBuffer::release returns m_data directly #
Depends on Patchset: Messages
Total messages: 34 (12 generated)
hajimehoshi@chromium.org changed reviewers: + haraken@chromium.org
PTAL
https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp (right): https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:364: String data = String(m_buffer.data(), (m_position + 1) / sizeof(BufferValueType)); return String(...) https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/StringBuffer.h (left): https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/StringBuffer.h:71: if (m_data->length() == newLength) Is it safe to change this condition to ASSERT? Sounds nice to try but it would be better to try in a separate CL. https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/StringBuffer.h (right): https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/StringBuffer.h:69: unsigned m_length; Why do we need to cache m_length in StringBuffer? m_data->length() is not enough? https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/StringBuffer.h:77: m_length = newLength; Don't we need to create a substring(0, newLength)? Otherwise we cannot shrink the memory usage when StringBuffer::shrink is called.
Thank you! https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp (right): https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:364: String data = String(m_buffer.data(), (m_position + 1) / sizeof(BufferValueType)); On 2015/10/13 07:40:29, haraken wrote: > > return String(...) Done. https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/StringBuffer.h (left): https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/StringBuffer.h:71: if (m_data->length() == newLength) Yes, it's safe: truncateAssumingIsolated had a same assertion. (newLength <= m_data->length()). https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/StringBuffer.h (right): https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/StringBuffer.h:69: unsigned m_length; On 2015/10/13 07:40:29, haraken wrote: > > Why do we need to cache m_length in StringBuffer? m_data->length() is not > enough? > This is just for avoiding StringImpl::substring. https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/StringBuffer.h:77: m_length = newLength; On 2015/10/13 07:40:29, haraken wrote: > > Don't we need to create a substring(0, newLength)? Otherwise we cannot shrink > the memory usage when StringBuffer::shrink is called. truncateAssumingIsolated doesn't reallocate Paritition alloc and this doesn't change the performance in terms of memory usage (substring would make it slightly better anyway, but it would make the speed bad).
LGTM https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/StringBuffer.h (right): https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/StringBuffer.h:77: m_length = newLength; On 2015/10/13 07:51:18, hajimehoshi wrote: > On 2015/10/13 07:40:29, haraken wrote: > > > > Don't we need to create a substring(0, newLength)? Otherwise we cannot shrink > > the memory usage when StringBuffer::shrink is called. > > truncateAssumingIsolated doesn't reallocate Paritition alloc and this doesn't > change the performance in terms of memory usage (substring would make it > slightly better anyway, but it would make the speed bad). Not related to this CL, but what's the benefit of calling the shrink() method? If it doesn't shrink the backing storage, it looks useless.
On 2015/10/13 07:54:06, haraken wrote: > LGTM > > https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/wtf/text/StringBuffer.h (right): > > https://codereview.chromium.org/1391153004/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/wtf/text/StringBuffer.h:77: m_length = newLength; > On 2015/10/13 07:51:18, hajimehoshi wrote: > > On 2015/10/13 07:40:29, haraken wrote: > > > > > > Don't we need to create a substring(0, newLength)? Otherwise we cannot > shrink > > > the memory usage when StringBuffer::shrink is called. > > > > truncateAssumingIsolated doesn't reallocate Paritition alloc and this doesn't > > change the performance in terms of memory usage (substring would make it > > slightly better anyway, but it would make the speed bad). > > Not related to this CL, but what's the benefit of calling the shrink() method? > If it doesn't shrink the backing storage, it looks useless. I don't know... Just updating the length?
The CQ bit was checked by hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391153004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391153004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_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 hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391153004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391153004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391153004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391153004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391153004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391153004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hmm, the results are flaky and I don't think this CL is to blame.
The CQ bit was checked by hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391153004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391153004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Found wtf_unittests --gtest_filter=WTF.SimplifyWhiteSpace fails on my local machine :-o
haraken: I've found a bug and fixed it. StringBuffer::release returns its inner m_data, which means m_data should be shrunk. PTAL
LGTM
The CQ bit was checked by hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391153004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391153004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/06334c75e3417d0a4d0cb7c5565e3ac40c3940cc Cr-Commit-Position: refs/heads/master@{#353996} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
