WIP: Introduce CompressibleString

WIP: Introduce CompressibleString

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect

[hajimehoshi, 2015-10-20 05:31:04]

hajimehoshi@chromium.org changed reviewers:
+ haraken@chromium.org

[hajimehoshi, 2015-10-20 11:47:47]

haraken: This is still work in progress but I think this is almost finished.
PTAL

[haraken, 2015-10-22 16:03:31]

Sorry about the review delay...

The approach looks good to me.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp:17: :
ScriptSourceCode(CompressableString::create(source), url, startPosition)

I'm wondering why you need to create a new CompressableString here.
ScriptResource::script() returns a CompressableString*, so you just need to
catch it by ScriptSourceCode::m_source (which is RefPtr<CompressableString>).

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h:80:
RefPtrWillBeMember<CompressableString> m_source;

I don't think it makes sense to put CompressableString on Oilpan's heap. Let's
keep it in non-oilpan (i.e., Remove all WillBe types from CompressableString).

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ScriptResource.cpp (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ScriptResource.cpp:80:
RefPtrWillBeRawPtr<CompressableString> ScriptResource::script()

RefPtrWillBeRawPtr<CompressableString> => CompressableString*

(because conceptually this is not transferring ownership.)

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ScriptResource.cpp:95: return m_script;

return m_script.get();

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/CompressableString.cpp (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:23: unsigned
CompressableString::length() const

Inline these simple functions.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:38: void
CompressableString::compress()

Your plan is to make CompressableString inherit from MemoryPurgeClient and call
compress() in MemoryPurgeClient::purgeMemory(), right?

I'm a bit concerned about the overhead of making all CompressableStrings
MemoryPurgeClient. Maybe it's better to create a HashSet<CompressableString*>
which keeps track of CompressableStrings larger than 50 KB (and make the wrapper
class of the HashSet MemoryPurgeClient).

  class LargeCompressableStrings : public MemoryPurgeClient {
    HashSet<CompressableString*> m_strings;
  };

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:60: if
(isCompressed())

Add UNLIKELY.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/CompressableString.h (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.h:31: bool m_isCompressed;

Move this boolean to below m_is8Bit (to save sizeof(CompressableString)).

[esprehn, 2015-10-24 03:59:25]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-10-24 03:59:26]

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ScriptResource.h (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ScriptResource.h:61:
RefPtrWillBeRawPtr<CompressableString> script();
not a ref ptr

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ScriptResource.h:88:
RefPtrWillBeMember<CompressableString> m_script;
By making these not atomic strings you lose the de-duping behavior which is bad,
you need to add an atomization layer around your compressed string system,
otherwise this doesn't generalize to style where we need hash tables of
CompressableStrings.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/CompressableString.cpp (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:9: #include
"components/compression/compression_utils.h"
you can't include from components inside blink, this also isn't planned for the
future either. If this is a general purpose compression lib it should probably
be in base.

In general I think this needs to be in platform though, and you need content to
implement an interface for doing the compression.

WTF is like our base, and base certainly can't depend on components/

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:45: in =
std::string(reinterpret_cast<const char*>(m_impl->characters8()),
m_impl->sizeInBytes());
this is making copies which is bad.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:47: in =
std::string(reinterpret_cast<const char*>(m_impl->characters16()),
m_impl->sizeInBytes());
more copies, also don't you want string16?

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:52: memcpy(data,
out.c_str(), out.size());
Why are we storing the compressed data inside a StringImpl? It's not a valid
string anymore, you just want to store it in a buffer somewhere, not a
StringImpl. Maybe a SharedBuffer?

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/CompressableString.h (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.h:21: void compress();
Why is compress() public? Why would a caller ever want to explicitly compress
instead of having the string decide.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.h:31: bool m_isCompressed;
On 2015/10/22 at 16:03:31, haraken wrote:
> Move this boolean to below m_is8Bit (to save sizeof(CompressableString)).

You need to also combine them with a bitfield for windows.

[haraken, 2015-10-24 17:26:38]

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/CompressableString.cpp (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:9: #include
"components/compression/compression_utils.h"
On 2015/10/24 03:59:25, esprehn wrote:
> you can't include from components inside blink, this also isn't planned for
the
> future either. If this is a general purpose compression lib it should probably
> be in base.
> 
> In general I think this needs to be in platform though, and you need content
to
> implement an interface for doing the compression.
> 
> WTF is like our base, and base certainly can't depend on components/

Agreed. It is a bad idea to make wtf/ depend on components/. We can put
CompressableString in platform/.

[hajimehoshi, 2015-10-26 09:34:03]

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp:17: :
ScriptSourceCode(CompressableString::create(source), url, startPosition)
On 2015/10/22 16:03:31, haraken wrote:
> 
> I'm wondering why you need to create a new CompressableString here.
> ScriptResource::script() returns a CompressableString*, so you just need to
> catch it by ScriptSourceCode::m_source (which is RefPtr<CompressableString>).
> 

This is backward compatibility. For example, XMLDocumentParser uses this
constructor. I can remove this and fix callers instead.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h:80:
RefPtrWillBeMember<CompressableString> m_source;
On 2015/10/22 16:03:31, haraken wrote:
> 
> I don't think it makes sense to put CompressableString on Oilpan's heap. Let's
> keep it in non-oilpan (i.e., Remove all WillBe types from CompressableString).

Done.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ScriptResource.cpp (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ScriptResource.cpp:80:
RefPtrWillBeRawPtr<CompressableString> ScriptResource::script()
On 2015/10/22 16:03:31, haraken wrote:
> 
> RefPtrWillBeRawPtr<CompressableString> => CompressableString*
> 
> (because conceptually this is not transferring ownership.)

Done.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ScriptResource.cpp:95: return m_script;
On 2015/10/22 16:03:31, haraken wrote:
> 
> return m_script.get();

Done.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/CompressableString.cpp (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:9: #include
"components/compression/compression_utils.h"
On 2015/10/24 17:26:37, haraken wrote:
> On 2015/10/24 03:59:25, esprehn wrote:
> > you can't include from components inside blink, this also isn't planned for
> the
> > future either. If this is a general purpose compression lib it should
probably
> > be in base.
> > 
> > In general I think this needs to be in platform though, and you need content
> to
> > implement an interface for doing the compression.
> > 
> > WTF is like our base, and base certainly can't depend on components/
> 
> Agreed. It is a bad idea to make wtf/ depend on components/. We can put
> CompressableString in platform/.

Sure, I understand components is not available from wtf. I'll create an
interface for compression at platform.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:23: unsigned
CompressableString::length() const
On 2015/10/22 16:03:31, haraken wrote:
> 
> Inline these simple functions.

Done.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:45: in =
std::string(reinterpret_cast<const char*>(m_impl->characters8()),
m_impl->sizeInBytes());
On 2015/10/24 03:59:26, esprehn wrote:
> this is making copies which is bad.

Yeah, the implementation is temporal. To fix this, I'd need to modify the
compression component, which accepts only std::string.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:60: if
(isCompressed())
On 2015/10/22 16:03:31, haraken wrote:
> 
> Add UNLIKELY.

Done.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/CompressableString.h (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.h:21: void compress();
On 2015/10/24 03:59:26, esprehn wrote:
> Why is compress() public? Why would a caller ever want to explicitly compress
> instead of having the string decide.

Done.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.h:31: bool m_isCompressed;
On 2015/10/24 03:59:26, esprehn wrote:
> On 2015/10/22 at 16:03:31, haraken wrote:
> > Move this boolean to below m_is8Bit (to save sizeof(CompressableString)).
> 
> You need to also combine them with a bitfield for windows.

Done.

[hajimehoshi, 2015-10-27 09:43:56]

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/CompressableString.cpp (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:52: memcpy(data,
out.c_str(), out.size());
On 2015/10/24 03:59:26, esprehn wrote:
> Why are we storing the compressed data inside a StringImpl? It's not a valid
> string anymore, you just want to store it in a buffer somewhere, not a
> StringImpl. Maybe a SharedBuffer?

I agree to that StringImpl is not appropriate for this usage, but SharedBuffer
in platform is not available from wtf.

[hajimehoshi, 2015-10-27 09:52:33]

On 2015/10/27 09:43:56, hajimehoshi wrote:
>
https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/wtf/text/CompressableString.cpp (right):
> 
>
https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/wtf/text/CompressableString.cpp:52: memcpy(data,
> out.c_str(), out.size());
> On 2015/10/24 03:59:26, esprehn wrote:
> > Why are we storing the compressed data inside a StringImpl? It's not a valid
> > string anymore, you just want to store it in a buffer somewhere, not a
> > StringImpl. Maybe a SharedBuffer?
> 
> I agree to that StringImpl is not appropriate for this usage, but SharedBuffer
> in platform is not available from wtf.

Oops, I found that CompressableString depends on platform/MemoryPurgeController,
which is not allowed. I'll move CompressableString to platform.

[hajimehoshi, 2015-10-27 11:35:10]

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/CompressableString.cpp (right):

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:38: void
CompressableString::compress()
On 2015/10/22 16:03:31, haraken wrote:
> 
> Your plan is to make CompressableString inherit from MemoryPurgeClient and
call
> compress() in MemoryPurgeClient::purgeMemory(), right?
> 
> I'm a bit concerned about the overhead of making all CompressableStrings
> MemoryPurgeClient. Maybe it's better to create a HashSet<CompressableString*>
> which keeps track of CompressableStrings larger than 50 KB (and make the
wrapper
> class of the HashSet MemoryPurgeClient).
> 
>   class LargeCompressableStrings : public MemoryPurgeClient {
>     HashSet<CompressableString*> m_strings;
>   };

Done. This is still not thread safe.

https://codereview.chromium.org/1389383003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/CompressableString.cpp:45: in =
std::string(reinterpret_cast<const char*>(m_impl->characters8()),
m_impl->sizeInBytes());
On 2015/10/26 09:34:03, hajimehoshi wrote:
> On 2015/10/24 03:59:26, esprehn wrote:
> > this is making copies which is bad.
> 
> Yeah, the implementation is temporal. To fix this, I'd need to modify the
> compression component, which accepts only std::string.

Avoiding copying requires hack to components/compression and it seems a little
tough work so far. I understand that using std::string causes unnecessary
copying but we can't avoid all copying. First, we need to allocate upper-bound
size which is a little bigger than input binary before compression and shrink
this after compression. 

This CL is for experiment to measure memory usage, so I'll return to this point
after that.

[tyoshino (SeeGerritForStatus), 2015-10-29 08:35:52]

Description: Instroduce -> Introduce

[hajimehoshi, 2015-10-29 08:36:44]

Description was changed from

==========
WIP: Instroduce CompressableString

BUG=
==========

to

==========
WIP: Introduce CompressableString

BUG=
==========

[hajimehoshi, 2015-11-24 09:17:56]

haraken: I think the implementation is almost finished, though there are some
things to do. Can you take a look especially at overall design? Thanks.

[haraken, 2015-11-24 11:15:42]

With this implementation, I'm afraid that all CompressableStrings are leaking
(See the following review comments).

Let's make the following change:

- Move the CompressableString table from V8PerIsolateData to
platform/text/CompressableString.h. You can create a thread-local hash table.
See how AtomicStringTable is implemented
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
You can do a similar thing.

- A CompressableStringImpl should be added to the table when
CompressableStringImpl is created. It should be removed from the table when the
CompressableStringImpl is destructed. See how AtomicStrings are added/removed
from the AtomicStringTable.

- CompressableStringImpls need to be registered to
V8PerIsolateData::m_stringCache. ref() must be called when the V8 wrapper of the
CompressableString is returned to V8. deref() must be called when V8 collects
the wrapper. See the following comments. If it is not easy to store
CompressableStringImpls to V8PerIsolateData::m_stringCache (because the type of
the m_stringCache is StringImpl), you can introduce
V8PerIsolateData::m_compressableStringCache.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/.di...
File third_party/WebKit/.dir-locals.el (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/.di...
third_party/WebKit/.dir-locals.el:1: ((nil . ((indent-tabs-mode . nil)

Remove this file.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h:62: bool isEmpty()
const { return isNull() || m_source.isEmpty(); }

Do we need this change? I think that isEmpty should return true if isNull is
true.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h:80:
CompressableString m_source;

Just to confirm: Both ScriptSourceCode and ScriptResource have
CompressableString, but their backing storage (i.e., CompressableStringImpl) are
shared and thus not duplicated, right?

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:317:
m_compressableStrings.add(string.impl());

Who removes the destructed strings from the m_compressableStrings?

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:330: if
(!source.source().isNull() && source.source().length() >=
v8::String::kMaxLength) {

I wonder why we need this change.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:339: if
(!code.isNull() && code.length() >= v8::String::kMaxLength) {

Ditto.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8StringResource.cpp (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8StringResource.cpp:153: template
String v8StringToWebCoreString2<String>(v8::Local<v8::String>, ExternalMode);

Do we need this template?

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8StringResource.cpp:156: StringType
v8StringToWebCoreString2(v8::Local<v8::String> v8String, ExternalMode external)

Ditto.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:180: //
string->ref();

This ref is needed. If you don't have the ref, who keeps the CompressableString
alive?

I guess that in your CL all the CompressableStrings are leaking because the
strings are never evicted from the V8PerIsolateData::m_compressableStrings.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:182: //
m_stringCache.Set(stringImpl, wrapper.Pass(), &m_lastV8String);

You need to put the CompressableString into the m_stringCache. Otherwise,
deref() is never called when V8 collects the wrapper of the CompressableString
(See line 34 - 56).

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:917: // TODO: See children
frames?

No. V8PerIsolateData is per-thread, not per-frame.

I don't think that LocalFrame is a good place to dispatch compressStrings, but
let's worry about this issue later.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/text/CompressableString.cpp (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressableString.cpp:8: // TODO: This
seems illegal: Fix checkdeps?

We should add a public API to talk with component/compression.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressableString.cpp:16: void
CompressableStringImpl::purgeMemory()

What are you intending to do with the purgeMemory()?

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/text/CompressableString.h (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressableString.h:18: class
WTF_EXPORT CompressableStringImpl : public RefCounted<CompressableStringImpl> {

The design of CompressableStringImpl and CompressableString looks good.

[hajimehoshi, 2015-11-26 10:49:13]

Thank you!

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/.di...
File third_party/WebKit/.dir-locals.el (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/.di...
third_party/WebKit/.dir-locals.el:1: ((nil . ((indent-tabs-mode . nil)
On 2015/11/24 11:15:40, haraken wrote:
> 
> Remove this file.

Done.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h:62: bool isEmpty()
const { return isNull() || m_source.isEmpty(); }
On 2015/11/24 11:15:41, haraken wrote:
> 
> Do we need this change? I think that isEmpty should return true if isNull is
> true.

Done.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h:80:
CompressableString m_source;
On 2015/11/24 11:15:41, haraken wrote:
> 
> Just to confirm: Both ScriptSourceCode and ScriptResource have
> CompressableString, but their backing storage (i.e., CompressableStringImpl)
are
> shared and thus not duplicated, right?

Right. The copy constructor of CompressableString copies only the reference of
CompressableStringImpl.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:317:
m_compressableStrings.add(string.impl());
On 2015/11/24 11:15:41, haraken wrote:
> 
> Who removes the destructed strings from the m_compressableStrings?

Now CompressibleStringTable owns and destroys the instances.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:330: if
(!source.source().isNull() && source.source().length() >=
v8::String::kMaxLength) {
On 2015/11/24 11:15:41, haraken wrote:
> 
> I wonder why we need this change.

This is my mistake... Done.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:339: if
(!code.isNull() && code.length() >= v8::String::kMaxLength) {
On 2015/11/24 11:15:41, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8StringResource.cpp (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8StringResource.cpp:153: template
String v8StringToWebCoreString2<String>(v8::Local<v8::String>, ExternalMode);
On 2015/11/24 11:15:41, haraken wrote:
> 
> Do we need this template?

Sorry but this was a test code. Removed.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8StringResource.cpp:156: StringType
v8StringToWebCoreString2(v8::Local<v8::String> v8String, ExternalMode external)
On 2015/11/24 11:15:41, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:182: //
m_stringCache.Set(stringImpl, wrapper.Pass(), &m_lastV8String);
On 2015/11/24 11:15:41, haraken wrote:
> 
> You need to put the CompressableString into the m_stringCache. Otherwise,
> deref() is never called when V8 collects the wrapper of the CompressableString
> (See line 34 - 56).

Hmm, I didn't know m_stringCache was the owner of String objects.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:917: // TODO: See children
frames?
On 2015/11/24 11:15:42, haraken wrote:
> 
> No. V8PerIsolateData is per-thread, not per-frame.
> 
> I don't think that LocalFrame is a good place to dispatch compressStrings, but
> let's worry about this issue later.

I've changed to call CompressibleStringImpl::purgeMemory.

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/text/CompressableString.h (right):

https://codereview.chromium.org/1389383003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressableString.h:18: class
WTF_EXPORT CompressableStringImpl : public RefCounted<CompressableStringImpl> {
On 2015/11/24 11:15:42, haraken wrote:
> 
> The design of CompressableStringImpl and CompressableString looks good.

Thanks.

[hajimehoshi, 2015-11-26 10:49:35]

Description was changed from

==========
WIP: Introduce CompressableString

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect
==========

to

==========
WIP: Introduce CompressibleString

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect
==========

[hajimehoshi, 2015-11-26 10:56:20]

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp:84: // Encoded
by way of a m_source->isNull() being true, with the nullary constructor to be
used to
nit

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:37: #include
"platform/text/CompressibleString.h"
nit

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:403: 
nit

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ScriptResource.cpp (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ScriptResource.cpp:82:
dump->addScalar("size", "bytes", m_script.contentSizeInBytes());
I found this changes the meaning.

[haraken, 2015-11-26 11:50:03]

I'm afraid that all CompressibleStrings are still leaking in this CL... See the
comment in CompressibleString.cpp.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8StringResource.h (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:254: CORE_EXPORT
StringType v8StringToWebCoreString2(v8::Local<v8::String>, ExternalMode);

Remove this.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:191:
v8::Local<v8::String> StringCache::createStringAndInsertIntoCache(v8::Isolate*
isolate, const CompressibleString& string)

The implementation of the StringCache looks correct.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ScriptResource.cpp (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ScriptResource.cpp:96: if
(m_script.isNull() && m_data) {

Shouldn't this be !m_script.isNull()?

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/Page.cpp (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/Page.cpp:552:
CompressibleStringImpl::purgeMemory();

I'd propose to introduce a separate method for doing the string compression
(e.g., window.compressStrings) to measure the memory impact. If you use
MemoryPurgeController::purgeMemory for the measurement, it will do a bunch of
other reductions at the same time (e.g., memoryCache()->pruneAll()), which will
confuse your measurement.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/text/CompressibleString.cpp (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressibleString.cpp:74:
compressibleStringTable().compressAll();

For now this is okay, but compressing the table of this thread is not enough. We
need to compress tables of all threads.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressibleString.cpp:81: ,
m_is8Bit(m_impl->is8Bit())

You need to check !m_impl. If it's guaranteed to be non-null, you can add
ASSERT(m_impl).

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressibleString.cpp:165:
compressibleStringTable().add(m_impl.get());

Who removes the compressible string from the table?

- You need to add the CompressibleStringImpl to the table when the
CompressibleStringImpl is constructed (not when the CompressibleString is
constructed).

- You need to remove the CompressibleStringImpl to the table when the
CompressibleStringImpl is destructed (not when the CompressibleString is
destructed).

- The table should not call ref/deref. If the table adds a reference to the
CompressibleStringImpl, the CompressibleStringImpl never gets destructed until
the table gets destructed (i.e., until the Blink shuts down). In the current
implementation, all CompressibleStringImpls will leak.

Please create a small test code and confirm that:

- compression/decomrepssion is working as expected; and
- the CompressibleStrings are not leaking.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/text/CompressibleString.h (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressibleString.h:54: bool isEmpty()
const { return m_impl->isEmpty(); }

Don't we need to check !m_impl before accessing m_impl?

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressibleString.h:60: bool is8Bit()
const { return m_impl->is8Bit(); }

Ditto.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressibleString.h:64: const UChar*
characters16() const { return m_impl->characters16(); }

Ditto.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/WTFThreadData.h (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/WTFThreadData.h:50: typedef void
(*CompressibleStringTableDestructor)(blink::CompressibleStringTable*);

Hmm, it's not nice to write 'blink::' in wtf. It's a layering violation. Maybe
we need to move CompressibleString to wtf, but let's worry about the issue
later.

[hajimehoshi, 2015-11-27 11:03:58]

Thank you!

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8StringResource.h (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:254: CORE_EXPORT
StringType v8StringToWebCoreString2(v8::Local<v8::String>, ExternalMode);
On 2015/11/26 11:50:03, haraken wrote:
> 
> Remove this.

Oops, done.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp:191:
v8::Local<v8::String> StringCache::createStringAndInsertIntoCache(v8::Isolate*
isolate, const CompressibleString& string)
On 2015/11/26 11:50:03, haraken wrote:
> 
> The implementation of the StringCache looks correct.

Thanks.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ScriptResource.cpp (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ScriptResource.cpp:96: if
(m_script.isNull() && m_data) {
On 2015/11/26 11:50:03, haraken wrote:
> 
> Shouldn't this be !m_script.isNull()?

Old |!m_script| means |m_script| is null. I think my fix is correct.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/text/CompressibleString.cpp (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressibleString.cpp:74:
compressibleStringTable().compressAll();
On 2015/11/26 11:50:03, haraken wrote:
> 
> For now this is okay, but compressing the table of this thread is not enough.
We
> need to compress tables of all threads.

Commented.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressibleString.cpp:81: ,
m_is8Bit(m_impl->is8Bit())
On 2015/11/26 11:50:03, haraken wrote:
> 
> You need to check !m_impl. If it's guaranteed to be non-null, you can add
> ASSERT(m_impl).

Done.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressibleString.cpp:165:
compressibleStringTable().add(m_impl.get());
On 2015/11/26 11:50:03, haraken wrote:
> 
> Who removes the compressible string from the table?
> 
> - You need to add the CompressibleStringImpl to the table when the
> CompressibleStringImpl is constructed (not when the CompressibleString is
> constructed).
> 
> - You need to remove the CompressibleStringImpl to the table when the
> CompressibleStringImpl is destructed (not when the CompressibleString is
> destructed).
> 
> - The table should not call ref/deref. If the table adds a reference to the
> CompressibleStringImpl, the CompressibleStringImpl never gets destructed until
> the table gets destructed (i.e., until the Blink shuts down). In the current
> implementation, all CompressibleStringImpls will leak.
> 
> Please create a small test code and confirm that:
> 
> - compression/decomrepssion is working as expected; and
> - the CompressibleStrings are not leaking.

Done. (Skipped checking leaking as we talked offline.)

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/text/CompressibleString.h (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressibleString.h:54: bool isEmpty()
const { return m_impl->isEmpty(); }
On 2015/11/26 11:50:03, haraken wrote:
> 
> Don't we need to check !m_impl before accessing m_impl?

Done.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressibleString.h:60: bool is8Bit()
const { return m_impl->is8Bit(); }
On 2015/11/26 11:50:03, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/text/CompressibleString.h:64: const UChar*
characters16() const { return m_impl->characters16(); }
On 2015/11/26 11:50:03, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/WTFThreadData.h (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/WTFThreadData.h:50: typedef void
(*CompressibleStringTableDestructor)(blink::CompressibleStringTable*);
On 2015/11/26 11:50:03, haraken wrote:
> 
> Hmm, it's not nice to write 'blink::' in wtf. It's a layering violation. Maybe
> we need to move CompressibleString to wtf, but let's worry about the issue
> later.

Sure, I'll move this to wtf soon again.

[hajimehoshi, 2015-11-30 05:26:59]

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/WTFThreadData.h (right):

https://codereview.chromium.org/1389383003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/WTFThreadData.h:50: typedef void
(*CompressibleStringTableDestructor)(blink::CompressibleStringTable*);
On 2015/11/27 11:03:58, hajimehoshi wrote:
> On 2015/11/26 11:50:03, haraken wrote:
> > 
> > Hmm, it's not nice to write 'blink::' in wtf. It's a layering violation.
Maybe
> > we need to move CompressibleString to wtf, but let's worry about the issue
> > later.
> 
> Sure, I'll move this to wtf soon again.

We found that CompressibleString depends on blink::SharedBuffer, which means we
can't move CompressibleString to wtf easily...