Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1583263002:
Experimental CompressibleString UMA (Closed)
|
Created:
4 years, 11 months ago by hajimehoshi Modified:
4 years, 11 months ago CC:
chromium-reviews, eae+blinkwatch, apavlov+blink_chromium.org, kinuko+watch, blink-reviews-wtf_chromium.org, rwlbuis, caseq+blink_chromium.org, pfeldman+blink_chromium.org, blink-reviews-html_chromium.org, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews-bindings_chromium.org, gavinp+loader_chromium.org, devtools-reviews_chromium.org, blink-reviews, sof, timvolodine, lushnikov+blink_chromium.org, loading-reviews+fetch_chromium.org, Nate Chapin, tyoshino+watch_chromium.org, mlamouri+watch-blink_chromium.org, mvanouwerkerk+watch_chromium.org, sergeyv+blink_chromium.org, Mikhail, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExperimental CompressibleString UMA
This CL replaces String/AtomicString objects for JavaScript source
strings with CompressibleString. CompressibleString introduced by
this CL is a mock and just records UMA. CompressibleString has
compress/decompress methods but does not actual
compression/decompression. We're trying to experiment with this CL
to judge if we can introduce actual compression algorithm.
The new UMA Memory.CompressibleStringCount records the frequency
with which JavaScirpt source strings are compressed and decompressed
in foreground and background tabs. If decompressing in background
doesn't occur often compared to compressing in background, we would
be able to say decompressing would be effective to reduce memory.
BUG=574317
Committed: https://crrev.com/e46533afc22085c91b2ee05102c90208b029d6a9
Cr-Commit-Position: refs/heads/master@{#370693}
Patch Set 1 #Patch Set 2 : #
Total comments: 66
Patch Set 3 : haraken's review #Patch Set 4 : Add DecompressingInForegroundTab #
Total comments: 37
Patch Set 5 : (rebasing) #Patch Set 6 : haraken's review #
Total comments: 8
Patch Set 7 : isherman's review; Move CompressibleStringTable initialization #Patch Set 8 : (rebasing) #Patch Set 9 : Use only the main thread #Patch Set 10 : Adopt lazy-initializing way #Depends on Patchset: Messages
Total messages: 51 (19 generated)
Description was changed from ========== Experimental CompressibleString UMA We're now replacing CompressibleString to reduce Blink memory usage. As preparation, we need to check how often uncompressing is needed when the tab is in the background (of the browser). This CL introduces mock CompressibleString class and UMAs. BUG=574317 ========== to ========== Experimental CompressibleString UMA We're now replacing JavaScript strings with CompressibleString to reduce Blink memory usage. As preparation, we need to check how often decompressing is needed when the tab is in the background (of the browser). This CL introduces mock CompressibleString class and UMAs. BUG=574317 ==========
hajimehoshi@chromium.org changed reviewers: + haraken@chromium.org
PTAL
https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h:62: bool isEmpty() const { return isNull() || m_source.isEmpty(); } Why do you need to make this change? https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8StringResource.cpp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.cpp:41: static String fromStringResource(WebCoreStringResourceBase* resource) What is this change for? https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.cpp:51: static AtomicString fromStringResource(WebCoreStringResourceBase* resource) Ditto. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8StringResource.h (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:91: // FIX this! What do you mean? https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:96: if (!m_compressibleString.isNull()) { Add UNLIKELY. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:109: if (!m_compressibleString.isNull()) { Add UNLIKELY. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:203: // ASSERT(m_string); Remove this. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.h (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ValueCache.h:106: // Note that the last CompressibleString is not cached. This comment should be moved to the below v8ExternalString. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ValueCache.h:115: ASSERT(!string.isNull()); Add: ASSERT(m_lastStringImpl.get() != string.impl()); https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ScriptResource.cpp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ScriptResource.cpp:113: m_script = CompressibleString(StringImpl::empty()); Can't we call CompressibleString()? Why do you need StringImpl::empty()? https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/DOMWindow.cpp:30: #include "platform/MemoryPurgeController.h" Remove this. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:140: , m_timer(this, &Page::compressStrings) m_timer => m_timerForCompressStrings https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:380: m_timer.startOneShot(10, BLINK_FROM_HERE); Avoid hard-coding 10. static const double waitingTimeBeforeCompressingString = 10; https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:380: m_timer.startOneShot(10, BLINK_FROM_HERE); Add a comment what you're doing this. // Compress CompressibleStrings when 10 seconds have passed since the page went to background. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/text/CompressibleString.cpp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:9: #include "wtf/LinkedStack.h" This is not necessary. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:10: #include "wtf/Partitions.h" Ditto. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:11: #include "wtf/PassOwnPtr.h" Ditto. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:29: m_table.add(string); Can we add: ASSERT(!m_table.contains(string)); ? https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:39: m_table.remove(string); Can we add: ASSERT(m_table.contains(string)); ? https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:65: static inline CompressibleStringTable& compressibleStringTable() How about creating the table in Platform::initialize? https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:67: // Once possible we should make this non-lazy (constructed in WTFThreadData's constructor). Once possible => remove the word https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:111: return originalContentSizeInBytes(); Add a TODO to update this function once we enable the compression? https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:145: if (s_isPageBackground) Why do we need the 'if (s_isPageBackground)' check? https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:157: if (s_isPageBackground) Ditto. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/text/CompressibleString.h (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:9: #include "wtf/HashTableDeletedValueType.h" This wouldn't be needed. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:18: class PLATFORM_EXPORT CompressibleStringImpl : public RefCounted<CompressibleStringImpl> { Add final. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:29: PassRefPtr<StringImpl> impl() { return m_impl; } Add const. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:30: bool isCompressed() const { return m_isCompressed; } Add const. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:34: unsigned originalContentSizeInBytes() const; originalContentSizeInBytes => originalSizeInBytes (for consistency with currentSizeInBytes)? https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:51: class PLATFORM_EXPORT CompressibleString { Add final. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:64: bool is8Bit() const { return m_impl->is8Bit(); } m_impl ? m_impl->is8Bit() : false; https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/wtf.gyp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/wtf.gyp:61: '<(DEPTH)', What is this change for?
Thank you! https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h:62: bool isEmpty() const { return isNull() || m_source.isEmpty(); } On 2016/01/15 12:46:20, haraken wrote: > > Why do you need to make this change? Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8StringResource.cpp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.cpp:41: static String fromStringResource(WebCoreStringResourceBase* resource) On 2016/01/15 12:46:20, haraken wrote: > > What is this change for? CompressibleString::string() can return an empty string: Now I found emptyString() in WTFString.h. I'll use this. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.cpp:51: static AtomicString fromStringResource(WebCoreStringResourceBase* resource) On 2016/01/15 12:46:20, haraken wrote: > > Ditto. Unfortunately now WebCoreStringResourceBase::atomicString can't return a reference because AtomicString needs to be created for a CompressibleString. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8StringResource.h (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:91: // FIX this! On 2016/01/15 12:46:21, haraken wrote: > > What do you mean? Ah, sorry... I mean we need to decide if memoryConsumption expects 'compressed' string size or 'uncompressed' string size. In the current situation, the strings are never compressed and we don't have to care. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:96: if (!m_compressibleString.isNull()) { On 2016/01/15 12:46:20, haraken wrote: > > Add UNLIKELY. Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:109: if (!m_compressibleString.isNull()) { On 2016/01/15 12:46:20, haraken wrote: > > Add UNLIKELY. Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:203: // ASSERT(m_string); On 2016/01/15 12:46:21, haraken wrote: > > Remove this. Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.h (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ValueCache.h:106: // Note that the last CompressibleString is not cached. On 2016/01/15 12:46:21, haraken wrote: > > This comment should be moved to the below v8ExternalString. Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ValueCache.h:115: ASSERT(!string.isNull()); On 2016/01/15 12:46:21, haraken wrote: > > Add: > > ASSERT(m_lastStringImpl.get() != string.impl()); string.impl() is CompressibleStringImpl* and I think exposure of CompressibleStringImpl here is not good. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ScriptResource.cpp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ScriptResource.cpp:113: m_script = CompressibleString(StringImpl::empty()); On 2016/01/15 12:46:21, haraken wrote: > > Can't we call CompressibleString()? Why do you need StringImpl::empty()? Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/DOMWindow.cpp:30: #include "platform/MemoryPurgeController.h" On 2016/01/15 12:46:21, haraken wrote: > > Remove this. Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:140: , m_timer(this, &Page::compressStrings) On 2016/01/15 12:46:21, haraken wrote: > > m_timer => m_timerForCompressStrings Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:380: m_timer.startOneShot(10, BLINK_FROM_HERE); On 2016/01/15 12:46:21, haraken wrote: > > Avoid hard-coding 10. > > static const double waitingTimeBeforeCompressingString = 10; Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:380: m_timer.startOneShot(10, BLINK_FROM_HERE); On 2016/01/15 12:46:21, haraken wrote: > > Add a comment what you're doing this. > > // Compress CompressibleStrings when 10 seconds have passed since the page went > to background. Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/text/CompressibleString.cpp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:9: #include "wtf/LinkedStack.h" On 2016/01/15 12:46:21, haraken wrote: > > This is not necessary. Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:10: #include "wtf/Partitions.h" On 2016/01/15 12:46:21, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:11: #include "wtf/PassOwnPtr.h" On 2016/01/15 12:46:21, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:29: m_table.add(string); On 2016/01/15 12:46:21, haraken wrote: > > Can we add: > > ASSERT(!m_table.contains(string)); > > ? Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:39: m_table.remove(string); On 2016/01/15 12:46:21, haraken wrote: > > Can we add: > > ASSERT(m_table.contains(string)); > > ? Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:65: static inline CompressibleStringTable& compressibleStringTable() On 2016/01/15 12:46:21, haraken wrote: > > How about creating the table in Platform::initialize? Done (I added initializeCompressibleStringTable function to avoid to expose CompressibleStringTable class.) https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:67: // Once possible we should make this non-lazy (constructed in WTFThreadData's constructor). On 2016/01/15 12:46:21, haraken wrote: > > Once possible => remove the word > Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:111: return originalContentSizeInBytes(); On 2016/01/15 12:46:21, haraken wrote: > > Add a TODO to update this function once we enable the compression? Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:145: if (s_isPageBackground) On 2016/01/15 12:46:21, haraken wrote: > > Why do we need the 'if (s_isPageBackground)' check? See the below comment. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:157: if (s_isPageBackground) On 2016/01/15 12:46:21, haraken wrote: > > Ditto. It's because it would not make sense to count decompressing when a tab is in front. What we want to know is that how often decompression occurs when a tab is background, and I thought (decompression count in a background tab) / (compression count in a background tab) would be a good measurement. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/text/CompressibleString.h (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:9: #include "wtf/HashTableDeletedValueType.h" On 2016/01/15 12:46:21, haraken wrote: > > This wouldn't be needed. Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:18: class PLATFORM_EXPORT CompressibleStringImpl : public RefCounted<CompressibleStringImpl> { On 2016/01/15 12:46:21, haraken wrote: > > Add final. Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:29: PassRefPtr<StringImpl> impl() { return m_impl; } On 2016/01/15 12:46:21, haraken wrote: > > Add const. This was removed. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:30: bool isCompressed() const { return m_isCompressed; } On 2016/01/15 12:46:21, haraken wrote: > > Add const. Where do I have to add const? https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:34: unsigned originalContentSizeInBytes() const; On 2016/01/15 12:46:21, haraken wrote: > > originalContentSizeInBytes => originalSizeInBytes (for consistency with > currentSizeInBytes)? StringImpl's sizeInBytes contains the size of StringImpl itself while String's sizeInBytes only contains the content size. This is inconsistent if anything and I chose to clarify the new name. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:51: class PLATFORM_EXPORT CompressibleString { On 2016/01/15 12:46:21, haraken wrote: > > Add final. Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:64: bool is8Bit() const { return m_impl->is8Bit(); } On 2016/01/15 12:46:21, haraken wrote: > > m_impl ? m_impl->is8Bit() : false; Done. https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/wtf.gyp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/wtf.gyp:61: '<(DEPTH)', On 2016/01/15 12:46:21, haraken wrote: > > What is this change for? Done.
https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/text/CompressibleString.cpp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:157: if (s_isPageBackground) Makes sense. Can we also introduce: else Platform::current()->histogramCustomCounts(histogramName(), DecompressingInForegroundTab, 0, 10000, 50); to count how many strings are decompressed after the tab goes back to the foreground?
https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/text/CompressibleString.cpp (right): https://codereview.chromium.org/1583263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:157: if (s_isPageBackground) On 2016/01/18 09:58:25, haraken wrote: > > Makes sense. Can we also introduce: > > else > Platform::current()->histogramCustomCounts(histogramName(), > DecompressingInForegroundTab, 0, 10000, 50); > > to count how many strings are decompressed after the tab goes back to the > foreground? Done.
https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:189: m_lastStringImpl = stringImpl; Previously I mentioned that it would be unsafe to store a CompressibleString onto m_lastStringImpl because the backing StringImpl can become stale if the CompressibleString compresses the StringImpl. So we have introduced m_compressibleStringCache to maintain CompressibleStrings separately from normal Strings. However, I begin to think that it is just safe to store a CompressibleString onto m_lastStringImpl. Even if the CompressibleString compresses the backing StringImpl while the m_lastStringImpl is pointing to the StringImpl, the worst thing that can happen is just that the destruction of the StringImpl will be delayed until the m_lastStringImpl points to another StringImpl. So, my question is: If we're allowed to store the CompressibleString onto m_lastStringImpl (this is safe), can we remove a bunch of code duplication from this CL? For example, can we remove m_compressibleStringCache and its Traits? (I'm sorry for asking this kind of design question now -- I should have noticed this earlier...)
https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:189: m_lastStringImpl = stringImpl; On 2016/01/18 17:23:37, haraken wrote: > > Previously I mentioned that it would be unsafe to store a CompressibleString > onto m_lastStringImpl because the backing StringImpl can become stale if the > CompressibleString compresses the StringImpl. So we have introduced > m_compressibleStringCache to maintain CompressibleStrings separately from normal > Strings. > > However, I begin to think that it is just safe to store a CompressibleString > onto m_lastStringImpl. Even if the CompressibleString compresses the backing > StringImpl while the m_lastStringImpl is pointing to the StringImpl, the worst > thing that can happen is just that the destruction of the StringImpl will be > delayed until the m_lastStringImpl points to another StringImpl. In the worst case, the memory usage would be increased because both the compressed string and the uncompressed string would exist at the same time. If compressing can reduce 90% size of a JavaScript source, the memory usage will be 110% (100% + 10%), right? I think m_lastStringImpl is update so often that we can allow this temporal memory usage. > > So, my question is: > > If we're allowed to store the CompressibleString onto m_lastStringImpl (this is > safe), can we remove a bunch of code duplication from this CL? For example, can > we remove m_compressibleStringCache and its Traits? Ok, let me try the refactoring you suggested. > > (I'm sorry for asking this kind of design question now -- I should have noticed > this earlier...) No problem!
https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:189: m_lastStringImpl = stringImpl; On 2016/01/18 17:23:37, haraken wrote: > > Previously I mentioned that it would be unsafe to store a CompressibleString > onto m_lastStringImpl because the backing StringImpl can become stale if the > CompressibleString compresses the StringImpl. So we have introduced > m_compressibleStringCache to maintain CompressibleStrings separately from normal > Strings. > > However, I begin to think that it is just safe to store a CompressibleString > onto m_lastStringImpl. Even if the CompressibleString compresses the backing > StringImpl while the m_lastStringImpl is pointing to the StringImpl, the worst > thing that can happen is just that the destruction of the StringImpl will be > delayed until the m_lastStringImpl points to another StringImpl. > > So, my question is: > > If we're allowed to store the CompressibleString onto m_lastStringImpl (this is > safe), can we remove a bunch of code duplication from this CL? For example, can > we remove m_compressibleStringCache and its Traits? > > (I'm sorry for asking this kind of design question now -- I should have noticed > this earlier...) Oops, I found that I did NOT add m_lastCompressibleStringImpl and the last CompressibleString object is not cached. I have no idea how often m_stringCache is cleared, in other words, how long the temporal memory usage increase in 110% as I said, 110% for big JavaScirpt source strings) continues.
On 2016/01/19 06:21:39, hajimehoshi wrote: > https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp (right): > > https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:189: > m_lastStringImpl = stringImpl; > On 2016/01/18 17:23:37, haraken wrote: > > > > Previously I mentioned that it would be unsafe to store a CompressibleString > > onto m_lastStringImpl because the backing StringImpl can become stale if the > > CompressibleString compresses the StringImpl. So we have introduced > > m_compressibleStringCache to maintain CompressibleStrings separately from > normal > > Strings. > > > > However, I begin to think that it is just safe to store a CompressibleString > > onto m_lastStringImpl. Even if the CompressibleString compresses the backing > > StringImpl while the m_lastStringImpl is pointing to the StringImpl, the worst > > thing that can happen is just that the destruction of the StringImpl will be > > delayed until the m_lastStringImpl points to another StringImpl. > > > > So, my question is: > > > > If we're allowed to store the CompressibleString onto m_lastStringImpl (this > is > > safe), can we remove a bunch of code duplication from this CL? For example, > can > > we remove m_compressibleStringCache and its Traits? > > > > (I'm sorry for asking this kind of design question now -- I should have > noticed > > this earlier...) > > Oops, I found that I did NOT add m_lastCompressibleStringImpl and the last > CompressibleString object is not cached. I have no idea how often m_stringCache > is cleared, in other words, how long the temporal memory usage increase in 110% > as I said, 110% for big JavaScirpt source strings) continues. It would be extremely rare. Every time V8 accesses a string allocated on Blink, m_lastStringImpl is updated. This happens pretty often (e.g., just calling div.id updates m_lastStringImpl). Actually m_lastStringImpl is a cache only to optimize performance of Dromaeo, which accesses the same strings repeatedly. So you don't need to worry about the worst case.
On 2016/01/19 06:26:11, haraken wrote: > On 2016/01/19 06:21:39, hajimehoshi wrote: > > > https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp (right): > > > > > https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:189: > > m_lastStringImpl = stringImpl; > > On 2016/01/18 17:23:37, haraken wrote: > > > > > > Previously I mentioned that it would be unsafe to store a CompressibleString > > > onto m_lastStringImpl because the backing StringImpl can become stale if the > > > CompressibleString compresses the StringImpl. So we have introduced > > > m_compressibleStringCache to maintain CompressibleStrings separately from > > normal > > > Strings. > > > > > > However, I begin to think that it is just safe to store a CompressibleString > > > onto m_lastStringImpl. Even if the CompressibleString compresses the backing > > > StringImpl while the m_lastStringImpl is pointing to the StringImpl, the > worst > > > thing that can happen is just that the destruction of the StringImpl will be > > > delayed until the m_lastStringImpl points to another StringImpl. > > > > > > So, my question is: > > > > > > If we're allowed to store the CompressibleString onto m_lastStringImpl (this > > is > > > safe), can we remove a bunch of code duplication from this CL? For example, > > can > > > we remove m_compressibleStringCache and its Traits? > > > > > > (I'm sorry for asking this kind of design question now -- I should have > > noticed > > > this earlier...) > > > > Oops, I found that I did NOT add m_lastCompressibleStringImpl and the last > > CompressibleString object is not cached. I have no idea how often > m_stringCache > > is cleared, in other words, how long the temporal memory usage increase in > 110% > > as I said, 110% for big JavaScirpt source strings) continues. > > It would be extremely rare. > > Every time V8 accesses a string allocated on Blink, m_lastStringImpl is updated. > This happens pretty often (e.g., just calling div.id updates m_lastStringImpl). > Actually m_lastStringImpl is a cache only to optimize performance of Dromaeo, > which accesses the same strings repeatedly. > > So you don't need to worry about the worst case. How about m_stringCache?
On 2016/01/19 06:34:40, hajimehoshi wrote: > On 2016/01/19 06:26:11, haraken wrote: > > On 2016/01/19 06:21:39, hajimehoshi wrote: > > > > > > https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp (right): > > > > > > > > > https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:189: > > > m_lastStringImpl = stringImpl; > > > On 2016/01/18 17:23:37, haraken wrote: > > > > > > > > Previously I mentioned that it would be unsafe to store a > CompressibleString > > > > onto m_lastStringImpl because the backing StringImpl can become stale if > the > > > > CompressibleString compresses the StringImpl. So we have introduced > > > > m_compressibleStringCache to maintain CompressibleStrings separately from > > > normal > > > > Strings. > > > > > > > > However, I begin to think that it is just safe to store a > CompressibleString > > > > onto m_lastStringImpl. Even if the CompressibleString compresses the > backing > > > > StringImpl while the m_lastStringImpl is pointing to the StringImpl, the > > worst > > > > thing that can happen is just that the destruction of the StringImpl will > be > > > > delayed until the m_lastStringImpl points to another StringImpl. > > > > > > > > So, my question is: > > > > > > > > If we're allowed to store the CompressibleString onto m_lastStringImpl > (this > > > is > > > > safe), can we remove a bunch of code duplication from this CL? For > example, > > > can > > > > we remove m_compressibleStringCache and its Traits? > > > > > > > > (I'm sorry for asking this kind of design question now -- I should have > > > noticed > > > > this earlier...) > > > > > > Oops, I found that I did NOT add m_lastCompressibleStringImpl and the last > > > CompressibleString object is not cached. I have no idea how often > > m_stringCache > > > is cleared, in other words, how long the temporal memory usage increase in > > 110% > > > as I said, 110% for big JavaScirpt source strings) continues. > > > > It would be extremely rare. > > > > Every time V8 accesses a string allocated on Blink, m_lastStringImpl is > updated. > > This happens pretty often (e.g., just calling div.id updates > m_lastStringImpl). > > Actually m_lastStringImpl is a cache only to optimize performance of Dromaeo, > > which accesses the same strings repeatedly. > > > > So you don't need to worry about the worst case. > > How about m_stringCache? I'm confused. Your CL is not adding CompressibleStrings to m_lastStringImpl but is adding CompressibleStrings to m_compressibleStringCache, right? My proposal is add them to both m_lastStringImpl and m_stringCache. - Adding them to m_lastStringImpl wouldn't be an issue for the reason I mentioned above. - Adding them to m_stringCache wouldn't be an issue because there is no difference (in terms of lifetime) between m_compressibleStringCache and m_stringCache. You can just change the place where the CompressibleStrings are cached. Or are you asking why it is okay to cache the CompressibleStrings on m_compressibleStringCache? It is okay because when V8 loses the last reference to the string wrapper, V8's GC removes the cache entry from the m_compressibleStringCache. In that case, the worst case will continue until V8 loses the last reference and invokes a GC, but if we want to avoid it, we need to stop caching CompressibleStrings on m_compressibleStringCache (this is doable but will reduce performance of accessing the CompressibleStrings because V8 needs to create a new string wrapper every time V8 needs to access the CompressibleString).
On 2016/01/19 06:47:27, haraken wrote: > On 2016/01/19 06:34:40, hajimehoshi wrote: > > On 2016/01/19 06:26:11, haraken wrote: > > > On 2016/01/19 06:21:39, hajimehoshi wrote: > > > > > > > > > > https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:189: > > > > m_lastStringImpl = stringImpl; > > > > On 2016/01/18 17:23:37, haraken wrote: > > > > > > > > > > Previously I mentioned that it would be unsafe to store a > > CompressibleString > > > > > onto m_lastStringImpl because the backing StringImpl can become stale if > > the > > > > > CompressibleString compresses the StringImpl. So we have introduced > > > > > m_compressibleStringCache to maintain CompressibleStrings separately > from > > > > normal > > > > > Strings. > > > > > > > > > > However, I begin to think that it is just safe to store a > > CompressibleString > > > > > onto m_lastStringImpl. Even if the CompressibleString compresses the > > backing > > > > > StringImpl while the m_lastStringImpl is pointing to the StringImpl, the > > > worst > > > > > thing that can happen is just that the destruction of the StringImpl > will > > be > > > > > delayed until the m_lastStringImpl points to another StringImpl. > > > > > > > > > > So, my question is: > > > > > > > > > > If we're allowed to store the CompressibleString onto m_lastStringImpl > > (this > > > > is > > > > > safe), can we remove a bunch of code duplication from this CL? For > > example, > > > > can > > > > > we remove m_compressibleStringCache and its Traits? > > > > > > > > > > (I'm sorry for asking this kind of design question now -- I should have > > > > noticed > > > > > this earlier...) > > > > > > > > Oops, I found that I did NOT add m_lastCompressibleStringImpl and the last > > > > CompressibleString object is not cached. I have no idea how often > > > m_stringCache > > > > is cleared, in other words, how long the temporal memory usage increase in > > > 110% > > > > as I said, 110% for big JavaScirpt source strings) continues. > > > > > > It would be extremely rare. > > > > > > Every time V8 accesses a string allocated on Blink, m_lastStringImpl is > > updated. > > > This happens pretty often (e.g., just calling div.id updates > > m_lastStringImpl). > > > Actually m_lastStringImpl is a cache only to optimize performance of > Dromaeo, > > > which accesses the same strings repeatedly. > > > > > > So you don't need to worry about the worst case. > > > > How about m_stringCache? > > I'm confused. > > Your CL is not adding CompressibleStrings to m_lastStringImpl but is adding > CompressibleStrings to m_compressibleStringCache, right? My proposal is add them > to both m_lastStringImpl and m_stringCache. > > - Adding them to m_lastStringImpl wouldn't be an issue for the reason I > mentioned above. > > - Adding them to m_stringCache wouldn't be an issue because there is no > difference (in terms of lifetime) between m_compressibleStringCache and > m_stringCache. You can just change the place where the CompressibleStrings are > cached. > > Or are you asking why it is okay to cache the CompressibleStrings on > m_compressibleStringCache? It is okay because when V8 loses the last reference > to the string wrapper, V8's GC removes the cache entry from the > m_compressibleStringCache. In that case, the worst case will continue until V8 > loses the last reference and invokes a GC, but if we want to avoid it, we need > to stop caching CompressibleStrings on m_compressibleStringCache (this is doable > but will reduce performance of accessing the CompressibleStrings because V8 > needs to create a new string wrapper every time V8 needs to access the > CompressibleString). I'm fine with unifying m_last*. My question is: why is it OK to cache CompressibleStringImpl's StringImpl objects in m_stringCache? As long as m_compressibleStringCache holds CompressibleStringImpl objects, there is no problem because compression those objects would be effective, but if CompressibleStringImpl's StringImpl objects are cached in m_stringCache as StringImpl, StringImpl as uncompressed string would exist even when the string is compressed.
On 2016/01/19 07:00:54, hajimehoshi wrote: > On 2016/01/19 06:47:27, haraken wrote: > > On 2016/01/19 06:34:40, hajimehoshi wrote: > > > On 2016/01/19 06:26:11, haraken wrote: > > > > On 2016/01/19 06:21:39, hajimehoshi wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:189: > > > > > m_lastStringImpl = stringImpl; > > > > > On 2016/01/18 17:23:37, haraken wrote: > > > > > > > > > > > > Previously I mentioned that it would be unsafe to store a > > > CompressibleString > > > > > > onto m_lastStringImpl because the backing StringImpl can become stale > if > > > the > > > > > > CompressibleString compresses the StringImpl. So we have introduced > > > > > > m_compressibleStringCache to maintain CompressibleStrings separately > > from > > > > > normal > > > > > > Strings. > > > > > > > > > > > > However, I begin to think that it is just safe to store a > > > CompressibleString > > > > > > onto m_lastStringImpl. Even if the CompressibleString compresses the > > > backing > > > > > > StringImpl while the m_lastStringImpl is pointing to the StringImpl, > the > > > > worst > > > > > > thing that can happen is just that the destruction of the StringImpl > > will > > > be > > > > > > delayed until the m_lastStringImpl points to another StringImpl. > > > > > > > > > > > > So, my question is: > > > > > > > > > > > > If we're allowed to store the CompressibleString onto m_lastStringImpl > > > (this > > > > > is > > > > > > safe), can we remove a bunch of code duplication from this CL? For > > > example, > > > > > can > > > > > > we remove m_compressibleStringCache and its Traits? > > > > > > > > > > > > (I'm sorry for asking this kind of design question now -- I should > have > > > > > noticed > > > > > > this earlier...) > > > > > > > > > > Oops, I found that I did NOT add m_lastCompressibleStringImpl and the > last > > > > > CompressibleString object is not cached. I have no idea how often > > > > m_stringCache > > > > > is cleared, in other words, how long the temporal memory usage increase > in > > > > 110% > > > > > as I said, 110% for big JavaScirpt source strings) continues. > > > > > > > > It would be extremely rare. > > > > > > > > Every time V8 accesses a string allocated on Blink, m_lastStringImpl is > > > updated. > > > > This happens pretty often (e.g., just calling div.id updates > > > m_lastStringImpl). > > > > Actually m_lastStringImpl is a cache only to optimize performance of > > Dromaeo, > > > > which accesses the same strings repeatedly. > > > > > > > > So you don't need to worry about the worst case. > > > > > > How about m_stringCache? > > > > I'm confused. > > > > Your CL is not adding CompressibleStrings to m_lastStringImpl but is adding > > CompressibleStrings to m_compressibleStringCache, right? My proposal is add > them > > to both m_lastStringImpl and m_stringCache. > > > > - Adding them to m_lastStringImpl wouldn't be an issue for the reason I > > mentioned above. > > > > - Adding them to m_stringCache wouldn't be an issue because there is no > > difference (in terms of lifetime) between m_compressibleStringCache and > > m_stringCache. You can just change the place where the CompressibleStrings are > > cached. > > > > Or are you asking why it is okay to cache the CompressibleStrings on > > m_compressibleStringCache? It is okay because when V8 loses the last reference > > to the string wrapper, V8's GC removes the cache entry from the > > m_compressibleStringCache. In that case, the worst case will continue until V8 > > loses the last reference and invokes a GC, but if we want to avoid it, we need > > to stop caching CompressibleStrings on m_compressibleStringCache (this is > doable > > but will reduce performance of accessing the CompressibleStrings because V8 > > needs to create a new string wrapper every time V8 needs to access the > > CompressibleString). > > I'm fine with unifying m_last*. > > My question is: why is it OK to cache CompressibleStringImpl's StringImpl > objects in m_stringCache? As long as m_compressibleStringCache holds > CompressibleStringImpl objects, there is no problem because compression those > objects would be effective, but if CompressibleStringImpl's StringImpl objects > are cached in m_stringCache as StringImpl, StringImpl as uncompressed string > would exist even when the string is compressed. Ah, thanks, now I understand :) Given that we anyway need to add separate code for m_compressibleStringCache, you don't need to try to unify the m_last* part.
LGTM with nits. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp:90: m_source = CompressibleString(StringImpl::empty()); CompressibleString() ? https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:210: m_compressibleStringCache.Set(stringImpl, wrapper.Pass(), &unused); Add a comment and mention why you don't want to add CompressibleStrings to m_lastStringImpl. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:383: if (m_visibilityState == PageVisibilityStateHidden && !m_timerForCompressStrings.isActive()) { Shouldn't this be: if (m_visibilityState == PageVisibilityStateHidden) { if (!m_timer.isActive) m_timer.startOneShot(); } else if (m_timer.isActive()) { m_timer.stop(); } ? https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/text/CompressibleString.cpp (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:90: CompressibleStringImpl::CompressibleStringImpl() For performance reasons, I'd suggest inlining most of the CompressibleImpl's methods. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:96: CompressibleStringImpl::CompressibleStringImpl(PassRefPtr<StringImpl> impl) Inline. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:107: if (compressibleStringTable().contains(this)) A better condition would be: if (originalContentSizeInBytes() > CompressibleStringImplSizeThrehold) If compressibleStringTable().contains(this) doesn't hold, you'll hit an assert in the following remove. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:111: unsigned CompressibleStringImpl::originalContentSizeInBytes() const Inline. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:119: unsigned CompressibleStringImpl::currentSizeInBytes() const Inline. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:124: const String& CompressibleStringImpl::toString() It would be better to inline this. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:131: const LChar* CompressibleStringImpl::characters8() Inline. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:136: const UChar* CompressibleStringImpl::characters16() Inline. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:142: CompressingInBackgroundTab, StringWasCompressedInBackgroundTab StringWasDecompressedInBackgroundTab StringWasDecompressedInForegroundTab ? https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:156: if (s_isPageBackground) Shouldn't this be: ASSERT(s_isPageBackground); ? https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:176: CompressibleString::CompressibleString() Inline these methods. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/text/CompressibleString.h (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:27: CompressibleStringImpl(PassRefPtr<StringImpl>); Add explicit. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:72: PassRefPtr<CompressibleStringImpl> impl() const { return m_impl; } This is a getter for impl, so it should return CompressibleStringImpl*. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/WTFThreadData.h (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/WTFThreadData.h:38: namespace blink { Add a TODO and mention that the blink namespace should be removed by moving CompressibleString into wtf/.
hajimehoshi@chromium.org changed reviewers: + isherman@chromium.org
+isherman PTAL at histograms.xml
Thank you! https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp:90: m_source = CompressibleString(StringImpl::empty()); On 2016/01/19 07:43:35, haraken wrote: > > CompressibleString() ? Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:210: m_compressibleStringCache.Set(stringImpl, wrapper.Pass(), &unused); On 2016/01/19 07:43:35, haraken wrote: > > Add a comment and mention why you don't want to add CompressibleStrings to > m_lastStringImpl. You mean m_stringCache? Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:383: if (m_visibilityState == PageVisibilityStateHidden && !m_timerForCompressStrings.isActive()) { On 2016/01/19 07:43:35, haraken wrote: > > Shouldn't this be: > > if (m_visibilityState == PageVisibilityStateHidden) { > if (!m_timer.isActive) > m_timer.startOneShot(); > } else if (m_timer.isActive()) { > m_timer.stop(); > } > > ? Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/text/CompressibleString.cpp (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:90: CompressibleStringImpl::CompressibleStringImpl() On 2016/01/19 07:43:35, haraken wrote: > > For performance reasons, I'd suggest inlining most of the CompressibleImpl's > methods. Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:96: CompressibleStringImpl::CompressibleStringImpl(PassRefPtr<StringImpl> impl) On 2016/01/19 07:43:35, haraken wrote: > > Inline. Hmm, I don't want to expose compressibleStringTable() in the header file and keep this as it is. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:107: if (compressibleStringTable().contains(this)) On 2016/01/19 07:43:35, haraken wrote: > > A better condition would be: > > if (originalContentSizeInBytes() > CompressibleStringImplSizeThrehold) > > If compressibleStringTable().contains(this) doesn't hold, you'll hit an assert > in the following remove. Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:111: unsigned CompressibleStringImpl::originalContentSizeInBytes() const On 2016/01/19 07:43:35, haraken wrote: > > Inline. Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:119: unsigned CompressibleStringImpl::currentSizeInBytes() const On 2016/01/19 07:43:35, haraken wrote: > > Inline. Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:124: const String& CompressibleStringImpl::toString() On 2016/01/19 07:43:35, haraken wrote: > > It would be better to inline this. Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:131: const LChar* CompressibleStringImpl::characters8() On 2016/01/19 07:43:35, haraken wrote: > > Inline. Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:136: const UChar* CompressibleStringImpl::characters16() On 2016/01/19 07:43:35, haraken wrote: > > Inline. Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:142: CompressingInBackgroundTab, On 2016/01/19 07:43:35, haraken wrote: > > StringWasCompressedInBackgroundTab > StringWasDecompressedInBackgroundTab > StringWasDecompressedInForegroundTab > > ? Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:156: if (s_isPageBackground) On 2016/01/19 07:43:35, haraken wrote: > > Shouldn't this be: > > ASSERT(s_isPageBackground); > > ? Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.cpp:176: CompressibleString::CompressibleString() On 2016/01/19 07:43:35, haraken wrote: > > Inline these methods. Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/text/CompressibleString.h (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:27: CompressibleStringImpl(PassRefPtr<StringImpl>); On 2016/01/19 07:43:36, haraken wrote: > > Add explicit. Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/text/CompressibleString.h:72: PassRefPtr<CompressibleStringImpl> impl() const { return m_impl; } On 2016/01/19 07:43:35, haraken wrote: > > This is a getter for impl, so it should return CompressibleStringImpl*. Done. https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/WTFThreadData.h (right): https://codereview.chromium.org/1583263002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/WTFThreadData.h:38: namespace blink { On 2016/01/19 07:43:36, haraken wrote: > > Add a TODO and mention that the blink namespace should be removed by moving > CompressibleString into wtf/. Done.
https://codereview.chromium.org/1583263002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/text/CompressibleString.cpp (right): https://codereview.chromium.org/1583263002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/text/CompressibleString.cpp:122: Platform::current()->histogramCustomCounts(histogramName(), StringWasCompressedInBackgroundTab, 0, 10000, 50); nit: I'd recommend have a wrapper method for emitting to this histogram, rather than needing to repeat the constant arguments at each call site. https://codereview.chromium.org/1583263002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/text/CompressibleString.cpp:122: Platform::current()->histogramCustomCounts(histogramName(), StringWasCompressedInBackgroundTab, 0, 10000, 50); Hmm, why did you choose this histogram representation? You are currently allocating 50 buckets, logarithmically sized, to record values up to 10,000 in magnitude. You're logging three distinct values to this histogram. Please use an enumerated histogram for this purpose. Also, please take a moment to think about what led you to implement this as you did, and possibly update some documentation somewhere to be clearer. https://codereview.chromium.org/1583263002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1583263002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:19900: + The count of CompressibleString for JavaScript source stirngs in a nit: s/stirngs/strings https://codereview.chromium.org/1583263002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:19902: + and decopressing is required in the background tab. nit: s/decopressing/decompressing https://codereview.chromium.org/1583263002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:19902: + and decopressing is required in the background tab. Please document when this is recorded -- once a minute? on page backgrounding? other? One possible clearer phrasing: "Records the frequency with which JavaScript source strings are compressed and decompressed in foreground and background tabs." Please feel free to refine further.
Description was changed from ========== Experimental CompressibleString UMA We're now replacing JavaScript strings with CompressibleString to reduce Blink memory usage. As preparation, we need to check how often decompressing is needed when the tab is in the background (of the browser). This CL introduces mock CompressibleString class and UMAs. BUG=574317 ========== to ========== Experimental CompressibleString UMA This CL replaces CompressibleString with String/AtomicString objects for JavaScript source strings. CompressibleString introduced by this CL is a mock and just records UMA. CompressibleString has compress/decompress methods but does not actual compression/decompression. We're trying to experiment with this CL to judge if we can introduce actual compression algorithm. The new UMA Memory.CompressibleStringCount to records the frequency with which JavaScirpt source strings are compressed and decompressed in foreground and background tabs. BUG=574317 ==========
Description was changed from ========== Experimental CompressibleString UMA This CL replaces CompressibleString with String/AtomicString objects for JavaScript source strings. CompressibleString introduced by this CL is a mock and just records UMA. CompressibleString has compress/decompress methods but does not actual compression/decompression. We're trying to experiment with this CL to judge if we can introduce actual compression algorithm. The new UMA Memory.CompressibleStringCount to records the frequency with which JavaScirpt source strings are compressed and decompressed in foreground and background tabs. BUG=574317 ========== to ========== Experimental CompressibleString UMA This CL replaces CompressibleString with String/AtomicString objects for JavaScript source strings. CompressibleString introduced by this CL is a mock and just records UMA. CompressibleString has compress/decompress methods but does not actual compression/decompression. We're trying to experiment with this CL to judge if we can introduce actual compression algorithm. The new UMA Memory.CompressibleStringCount records the frequency with which JavaScirpt source strings are compressed and decompressed in foreground and background tabs. If decompressing in background doesn't occur often compared to compressing in background, we would be able to say decompressing would be effective to reduce memory. BUG=574317 ==========
Thank you! https://codereview.chromium.org/1583263002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/Platform.cpp (right): https://codereview.chromium.org/1583263002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/Platform.cpp:55: initializeCompressibleStringTable(); Oops, WTF::Partitions is not initialized then. Add CompressibleStringImpl::initialize and make it called at initializeWithoutV8 after WTF initialization. https://codereview.chromium.org/1583263002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/text/CompressibleString.cpp (right): https://codereview.chromium.org/1583263002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/text/CompressibleString.cpp:122: Platform::current()->histogramCustomCounts(histogramName(), StringWasCompressedInBackgroundTab, 0, 10000, 50); On 2016/01/20 01:42:38, Ilya Sherman wrote: > Hmm, why did you choose this histogram representation? You are currently > allocating 50 buckets, logarithmically sized, to record values up to 10,000 in > magnitude. You're logging three distinct values to this histogram. Please use > an enumerated histogram for this purpose. This is because I misunderstood the usage of the API... Done. > Also, please take a moment to think > about what led you to implement this as you did, and possibly update some > documentation somewhere to be clearer. Updated the description of this CL. https://codereview.chromium.org/1583263002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1583263002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:19902: + and decopressing is required in the background tab. On 2016/01/20 01:42:39, Ilya Sherman wrote: > Please document when this is recorded -- once a minute? on page backgrounding? > other? > > One possible clearer phrasing: "Records the frequency with which JavaScript > source strings are compressed and decompressed in foreground and background > tabs." Please feel free to refine further. Done.
LGTM, thanks.
Description was changed from ========== Experimental CompressibleString UMA This CL replaces CompressibleString with String/AtomicString objects for JavaScript source strings. CompressibleString introduced by this CL is a mock and just records UMA. CompressibleString has compress/decompress methods but does not actual compression/decompression. We're trying to experiment with this CL to judge if we can introduce actual compression algorithm. The new UMA Memory.CompressibleStringCount records the frequency with which JavaScirpt source strings are compressed and decompressed in foreground and background tabs. If decompressing in background doesn't occur often compared to compressing in background, we would be able to say decompressing would be effective to reduce memory. BUG=574317 ========== to ========== Experimental CompressibleString UMA This CL replacesString/AtomicString objects for JavaScript source strings with CompressibleString. CompressibleString introduced by this CL is a mock and just records UMA. CompressibleString has compress/decompress methods but does not actual compression/decompression. We're trying to experiment with this CL to judge if we can introduce actual compression algorithm. The new UMA Memory.CompressibleStringCount records the frequency with which JavaScirpt source strings are compressed and decompressed in foreground and background tabs. If decompressing in background doesn't occur often compared to compressing in background, we would be able to say decompressing would be effective to reduce memory. BUG=574317 ==========
Description was changed from ========== Experimental CompressibleString UMA This CL replacesString/AtomicString objects for JavaScript source strings with CompressibleString. CompressibleString introduced by this CL is a mock and just records UMA. CompressibleString has compress/decompress methods but does not actual compression/decompression. We're trying to experiment with this CL to judge if we can introduce actual compression algorithm. The new UMA Memory.CompressibleStringCount records the frequency with which JavaScirpt source strings are compressed and decompressed in foreground and background tabs. If decompressing in background doesn't occur often compared to compressing in background, we would be able to say decompressing would be effective to reduce memory. BUG=574317 ========== to ========== Experimental CompressibleString UMA This CL replaces String/AtomicString objects for JavaScript source strings with CompressibleString. CompressibleString introduced by this CL is a mock and just records UMA. CompressibleString has compress/decompress methods but does not actual compression/decompression. We're trying to experiment with this CL to judge if we can introduce actual compression algorithm. The new UMA Memory.CompressibleStringCount records the frequency with which JavaScirpt source strings are compressed and decompressed in foreground and background tabs. If decompressing in background doesn't occur often compared to compressing in background, we would be able to say decompressing would be effective to reduce memory. BUG=574317 ==========
Description was changed from ========== Experimental CompressibleString UMA This CL replaces String/AtomicString objects for JavaScript source strings with CompressibleString. CompressibleString introduced by this CL is a mock and just records UMA. CompressibleString has compress/decompress methods but does not actual compression/decompression. We're trying to experiment with this CL to judge if we can introduce actual compression algorithm. The new UMA Memory.CompressibleStringCount records the frequency with which JavaScirpt source strings are compressed and decompressed in foreground and background tabs. If decompressing in background doesn't occur often compared to compressing in background, we would be able to say decompressing would be effective to reduce memory. BUG=574317 ========== to ========== Experimental CompressibleString UMA This CL replaces String/AtomicString objects for JavaScript source strings with CompressibleString. CompressibleString introduced by this CL is a mock and just records UMA. CompressibleString has compress/decompress methods but does not actual compression/decompression. We're trying to experiment with this CL to judge if we can introduce actual compression algorithm. The new UMA Memory.CompressibleStringCount records the frequency with which JavaScirpt source strings are compressed and decompressed in foreground and background tabs. If decompressing in background doesn't occur often compared to compressing in background, we would be able to say decompressing would be effective to reduce memory. BUG=574317 ==========
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1583263002/#ps120001 (title: "isherman's review; Move CompressibleStringTable initialization")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1583263002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1583263002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1583263002/#ps140001 (title: "(rebasing)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1583263002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1583263002/140001
Hmm, I wasn't able to reproduce layout-test failures on my local machine...
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_...)
hajimehoshi@chromium.org changed reviewers: + tasak@google.com
+tasak (Thanks to tasak-san, we were able to find that CompressbileString was touched on a thread other than the main thread. Thank you!) haraken: I have two questions. 1. Is it OK to make a table for CompressibleString only on the main thread? We've found that inspector can refer CompressibleString object on a thread other than the main thread. In the first place, the table for CompressibleString holds candidates to be compressed. As long as inspector refers a CompressibleString object, this is never compressed. Thus, we need to consider only main thread. Is this correct? 2. If so, should we still use thread-local-table way? We followed the AtomicString way in which a table is created as thread-local data. If CompressibleString table exists only on the main thread, can we use a static variable instead of thread-local data? Assuming 1. is true, I've fixed the try-bot problem where inspector referred a CompressibleString object on a thread other than the main thread. PTAL
On 2016/01/21 10:19:34, hajimehoshi wrote: > +tasak (Thanks to tasak-san, we were able to find that CompressbileString was > touched on a thread other than the main thread. Thank you!) > > haraken: I have two questions. > > 1. Is it OK to make a table for CompressibleString only on the main thread? > We've found that inspector can refer CompressibleString object on a thread other > than the main thread. In the first place, the table for CompressibleString holds > candidates to be compressed. As long as inspector refers a CompressibleString > object, this is never compressed. Thus, we need to consider only main thread. Is > this correct? > > 2. If so, should we still use thread-local-table way? We followed the > AtomicString way in which a table is created as thread-local data. If > CompressibleString table exists only on the main thread, can we use a static > variable instead of thread-local data? > > Assuming 1. is true, I've fixed the try-bot problem where inspector referred a > CompressibleString object on a thread other than the main thread. PTAL Ah, sorry, you're right! CompressibleString is a part of our standard library, so it should support all threads. So the answer would be: - Keep using the thread-local storage. - Go back to using the lazy initialization. (I'm sorry for getting you back and forth -- I realized that AtomicStringTable is using the lazy initialization. We should do the same thing for CompressibleStringTable.)
On 2016/01/21 10:24:35, haraken wrote: > On 2016/01/21 10:19:34, hajimehoshi wrote: > > +tasak (Thanks to tasak-san, we were able to find that CompressbileString was > > touched on a thread other than the main thread. Thank you!) > > > > haraken: I have two questions. > > > > 1. Is it OK to make a table for CompressibleString only on the main thread? > > We've found that inspector can refer CompressibleString object on a thread > other > > than the main thread. In the first place, the table for CompressibleString > holds > > candidates to be compressed. As long as inspector refers a CompressibleString > > object, this is never compressed. Thus, we need to consider only main thread. > Is > > this correct? > > > > 2. If so, should we still use thread-local-table way? We followed the > > AtomicString way in which a table is created as thread-local data. If > > CompressibleString table exists only on the main thread, can we use a static > > variable instead of thread-local data? > > > > Assuming 1. is true, I've fixed the try-bot problem where inspector referred a > > CompressibleString object on a thread other than the main thread. PTAL > > Ah, sorry, you're right! > > CompressibleString is a part of our standard library, so it should support all > threads. So the answer would be: > > - Keep using the thread-local storage. > - Go back to using the lazy initialization. (I'm sorry for getting you back and > forth -- I realized that AtomicStringTable is using the lazy initialization. We > should do the same thing for CompressibleStringTable.) Sure. In the current implementation, I think compression runs only on the main thread, anyway.
On 2016/01/21 10:33:23, hajimehoshi wrote: > On 2016/01/21 10:24:35, haraken wrote: > > On 2016/01/21 10:19:34, hajimehoshi wrote: > > > +tasak (Thanks to tasak-san, we were able to find that CompressbileString > was > > > touched on a thread other than the main thread. Thank you!) > > > > > > haraken: I have two questions. > > > > > > 1. Is it OK to make a table for CompressibleString only on the main thread? > > > We've found that inspector can refer CompressibleString object on a thread > > other > > > than the main thread. In the first place, the table for CompressibleString > > holds > > > candidates to be compressed. As long as inspector refers a > CompressibleString > > > object, this is never compressed. Thus, we need to consider only main > thread. > > Is > > > this correct? > > > > > > 2. If so, should we still use thread-local-table way? We followed the > > > AtomicString way in which a table is created as thread-local data. If > > > CompressibleString table exists only on the main thread, can we use a static > > > variable instead of thread-local data? > > > > > > Assuming 1. is true, I've fixed the try-bot problem where inspector referred > a > > > CompressibleString object on a thread other than the main thread. PTAL > > > > Ah, sorry, you're right! > > > > CompressibleString is a part of our standard library, so it should support all > > threads. So the answer would be: > > > > - Keep using the thread-local storage. > > - Go back to using the lazy initialization. (I'm sorry for getting you back > and > > forth -- I realized that AtomicStringTable is using the lazy initialization. > We > > should do the same thing for CompressibleStringTable.) > > Sure. In the current implementation, I think compression runs only on the main > thread, anyway. Adopted lazy-initializing way. PTAL
On 2016/01/21 10:41:25, hajimehoshi wrote: > On 2016/01/21 10:33:23, hajimehoshi wrote: > > On 2016/01/21 10:24:35, haraken wrote: > > > On 2016/01/21 10:19:34, hajimehoshi wrote: > > > > +tasak (Thanks to tasak-san, we were able to find that CompressbileString > > was > > > > touched on a thread other than the main thread. Thank you!) > > > > > > > > haraken: I have two questions. > > > > > > > > 1. Is it OK to make a table for CompressibleString only on the main > thread? > > > > We've found that inspector can refer CompressibleString object on a thread > > > other > > > > than the main thread. In the first place, the table for CompressibleString > > > holds > > > > candidates to be compressed. As long as inspector refers a > > CompressibleString > > > > object, this is never compressed. Thus, we need to consider only main > > thread. > > > Is > > > > this correct? > > > > > > > > 2. If so, should we still use thread-local-table way? We followed the > > > > AtomicString way in which a table is created as thread-local data. If > > > > CompressibleString table exists only on the main thread, can we use a > static > > > > variable instead of thread-local data? > > > > > > > > Assuming 1. is true, I've fixed the try-bot problem where inspector > referred > > a > > > > CompressibleString object on a thread other than the main thread. PTAL > > > > > > Ah, sorry, you're right! > > > > > > CompressibleString is a part of our standard library, so it should support > all > > > threads. So the answer would be: > > > > > > - Keep using the thread-local storage. > > > - Go back to using the lazy initialization. (I'm sorry for getting you back > > and > > > forth -- I realized that AtomicStringTable is using the lazy initialization. > > We > > > should do the same thing for CompressibleStringTable.) > > > > Sure. In the current implementation, I think compression runs only on the main > > thread, anyway. > > Adopted lazy-initializing way. PTAL Thanks, LGTM
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1583263002/#ps180001 (title: "Adopt lazy-initializing way")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1583263002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1583263002/180001
Message was sent while issue was closed.
Description was changed from ========== Experimental CompressibleString UMA This CL replaces String/AtomicString objects for JavaScript source strings with CompressibleString. CompressibleString introduced by this CL is a mock and just records UMA. CompressibleString has compress/decompress methods but does not actual compression/decompression. We're trying to experiment with this CL to judge if we can introduce actual compression algorithm. The new UMA Memory.CompressibleStringCount records the frequency with which JavaScirpt source strings are compressed and decompressed in foreground and background tabs. If decompressing in background doesn't occur often compared to compressing in background, we would be able to say decompressing would be effective to reduce memory. BUG=574317 ========== to ========== Experimental CompressibleString UMA This CL replaces String/AtomicString objects for JavaScript source strings with CompressibleString. CompressibleString introduced by this CL is a mock and just records UMA. CompressibleString has compress/decompress methods but does not actual compression/decompression. We're trying to experiment with this CL to judge if we can introduce actual compression algorithm. The new UMA Memory.CompressibleStringCount records the frequency with which JavaScirpt source strings are compressed and decompressed in foreground and background tabs. If decompressing in background doesn't occur often compared to compressing in background, we would be able to say decompressing would be effective to reduce memory. BUG=574317 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Experimental CompressibleString UMA This CL replaces String/AtomicString objects for JavaScript source strings with CompressibleString. CompressibleString introduced by this CL is a mock and just records UMA. CompressibleString has compress/decompress methods but does not actual compression/decompression. We're trying to experiment with this CL to judge if we can introduce actual compression algorithm. The new UMA Memory.CompressibleStringCount records the frequency with which JavaScirpt source strings are compressed and decompressed in foreground and background tabs. If decompressing in background doesn't occur often compared to compressing in background, we would be able to say decompressing would be effective to reduce memory. BUG=574317 ========== to ========== Experimental CompressibleString UMA This CL replaces String/AtomicString objects for JavaScript source strings with CompressibleString. CompressibleString introduced by this CL is a mock and just records UMA. CompressibleString has compress/decompress methods but does not actual compression/decompression. We're trying to experiment with this CL to judge if we can introduce actual compression algorithm. The new UMA Memory.CompressibleStringCount records the frequency with which JavaScirpt source strings are compressed and decompressed in foreground and background tabs. If decompressing in background doesn't occur often compared to compressing in background, we would be able to say decompressing would be effective to reduce memory. BUG=574317 Committed: https://crrev.com/e46533afc22085c91b2ee05102c90208b029d6a9 Cr-Commit-Position: refs/heads/master@{#370693} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e46533afc22085c91b2ee05102c90208b029d6a9 Cr-Commit-Position: refs/heads/master@{#370693} |
