Commit 2069f0a0 authored by Benoît Lizé's avatar Benoît Lizé Committed by Commit Bot

blink/dom: Make CharacterData use ParkableStrings for large strings.

Some DOM nodes are large (for instance, inline scripts), and their data is kept
in memory at all time. In the case of inline scripts, the data is owned by both
the DOM node and the script object. This prevents any inline script source code
from getting compressed.

Add a way to store such nodes as ParkableStrings. On facebook.com/obama, this
allows to compress ~1.9MB more data, and yields sizable memory savings (~1.5MB,
see linked bug).

The cost is to add a bool and a ParkableString member to each CharacterData
node, that is ~2 * sizeof(void*). There is no runtime cost (aside from a single
branch) if compression is not enabled, of if the data is small.

Bug: 929110
Change-Id: Icd70b108f5ebbd714f373ba362cb708cbb9aa52a
Reviewed-on: https://chromium-review.googlesource.com/c/1459548
Commit-Queue: Benoit L <lizeb@chromium.org>
Reviewed-by: default avatarKentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#630719}
parent 16f523ba
......@@ -33,11 +33,23 @@
#include "third_party/blink/renderer/core/events/mutation_event.h"
#include "third_party/blink/renderer/core/probe/core_probes.h"
#include "third_party/blink/renderer/platform/bindings/exception_state.h"
#include "third_party/blink/renderer/platform/bindings/parkable_string_manager.h"
namespace blink {
void CharacterData::Atomize() {
data_ = AtomicString(data_);
void CharacterData::MakeParkableOrAtomize() {
if (is_parkable_)
return;
// ParkableStrings have some overhead, don't pay it if we're not going to
// park a string at all.
if (ParkableStringManager::ShouldPark(*data_.Impl())) {
parkable_data_ = ParkableString(data_.ReleaseImpl());
data_ = String();
is_parkable_ = true;
} else {
data_ = AtomicString(data_);
}
}
void CharacterData::setData(const String& data) {
......@@ -59,20 +71,20 @@ String CharacterData::substringData(unsigned offset,
return String();
}
return data_.Substring(offset, count);
return data().Substring(offset, count);
}
void CharacterData::ParserAppendData(const String& data) {
String new_str = data_ + data;
String new_str = this->data() + data;
SetDataAndUpdate(new_str, data_.length(), 0, data.length(),
SetDataAndUpdate(new_str, this->data().length(), 0, data.length(),
kUpdateFromParser);
}
void CharacterData::appendData(const String& data) {
String new_str = data_ + data;
String new_str = this->data() + data;
SetDataAndUpdate(new_str, data_.length(), 0, data.length(),
SetDataAndUpdate(new_str, this->data().length(), 0, data.length(),
kUpdateFromNonParser);
// FIXME: Should we call textInserted here?
......@@ -90,7 +102,7 @@ void CharacterData::insertData(unsigned offset,
return;
}
String new_str = data_;
String new_str = this->data();
new_str.insert(data, offset);
SetDataAndUpdate(new_str, offset, 0, data.length(), kUpdateFromNonParser);
......@@ -131,7 +143,7 @@ void CharacterData::deleteData(unsigned offset,
exception_state))
return;
String new_str = data_;
String new_str = data();
new_str.Remove(offset, real_count);
SetDataAndUpdate(new_str, offset, real_count, 0, kUpdateFromNonParser);
......@@ -148,7 +160,7 @@ void CharacterData::replaceData(unsigned offset,
exception_state))
return;
String new_str = data_;
String new_str = this->data();
new_str.Remove(offset, real_count);
new_str.insert(data, offset);
......@@ -161,11 +173,11 @@ void CharacterData::replaceData(unsigned offset,
}
String CharacterData::nodeValue() const {
return data_;
return data();
}
bool CharacterData::ContainsOnlyWhitespaceOrEmpty() const {
return data_.ContainsOnlyWhitespaceOrEmpty();
return data().ContainsOnlyWhitespaceOrEmpty();
}
void CharacterData::setNodeValue(const String& node_value) {
......@@ -177,7 +189,11 @@ void CharacterData::SetDataAndUpdate(const String& new_data,
unsigned old_length,
unsigned new_length,
UpdateSource source) {
String old_data = data_;
String old_data = this->data();
if (is_parkable_) {
is_parkable_ = false;
parkable_data_ = ParkableString();
}
data_ = new_data;
DCHECK(!GetLayoutObject() || IsTextNode());
......@@ -217,7 +233,7 @@ void CharacterData::DidModifyData(const String& old_data, UpdateSource source) {
Document::kDOMCharacterDataModifiedListener)) {
DispatchScopedEvent(*MutationEvent::Create(
event_type_names::kDOMCharacterDataModified, Event::Bubbles::kYes,
nullptr, old_data, data_));
nullptr, old_data, data()));
}
DispatchSubtreeModifiedEvent();
}
......
......@@ -26,6 +26,7 @@
#include "third_party/blink/renderer/core/core_export.h"
#include "third_party/blink/renderer/core/dom/node.h"
#include "third_party/blink/renderer/platform/bindings/parkable_string.h"
#include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
namespace blink {
......@@ -36,10 +37,16 @@ class CORE_EXPORT CharacterData : public Node {
DEFINE_WRAPPERTYPEINFO();
public:
void Atomize();
const String& data() const { return data_; }
// Makes the data either Parkable or Atomic. This enables de-duplication in
// both cases, the first one allowing compression as well.
void MakeParkableOrAtomize();
String data() const {
return is_parkable_ ? parkable_data_.ToString() : data_;
}
void setData(const String&);
unsigned length() const { return data_.length(); }
unsigned length() const {
return is_parkable_ ? parkable_data_.length() : data_.length();
}
String substringData(unsigned offset, unsigned count, ExceptionState&);
void appendData(const String&);
void replaceData(unsigned offset,
......@@ -52,7 +59,7 @@ class CORE_EXPORT CharacterData : public Node {
bool ContainsOnlyWhitespaceOrEmpty() const;
StringImpl* DataImpl() { return data_.Impl(); }
StringImpl* DataImpl() { return data().Impl(); }
void ParserAppendData(const String&);
......@@ -60,13 +67,19 @@ class CORE_EXPORT CharacterData : public Node {
CharacterData(TreeScope& tree_scope,
const String& text,
ConstructionType type)
: Node(&tree_scope, type), data_(!text.IsNull() ? text : g_empty_string) {
: Node(&tree_scope, type),
is_parkable_(false),
data_(!text.IsNull() ? text : g_empty_string) {
DCHECK(type == kCreateOther || type == kCreateText ||
type == kCreateEditingText);
}
void SetDataWithoutUpdate(const String& data) {
DCHECK(!data.IsNull());
if (is_parkable_) {
is_parkable_ = false;
parkable_data_ = ParkableString();
}
data_ = data;
}
enum UpdateSource {
......@@ -75,6 +88,8 @@ class CORE_EXPORT CharacterData : public Node {
};
void DidModifyData(const String& old_value, UpdateSource);
bool is_parkable_;
ParkableString parkable_data_;
String data_;
private:
......
......@@ -3750,7 +3750,7 @@ String Element::TextFromChildren() {
return g_empty_string;
if (first_text_node && !found_multiple_text_nodes) {
first_text_node->Atomize();
first_text_node->MakeParkableOrAtomize();
return first_text_node->data();
}
......
......@@ -309,7 +309,7 @@ class MediaControlsImplTest : public PageTestBase,
virtual bool EnableDownloadInProductHelp() { return false; }
const String& GetDisplayedTime(MediaControlTimeDisplayElement* display) {
const String GetDisplayedTime(MediaControlTimeDisplayElement* display) {
return ToText(display->firstChild())->data();
}
......
......@@ -187,7 +187,8 @@ bool ParkableStringManager::ShouldPark(const StringImpl& string) {
// Don't attempt to park strings smaller than this size.
static constexpr unsigned int kSizeThreshold = 10000;
// TODO(lizeb): Consider parking non-main thread strings.
return string.length() > kSizeThreshold && IsMainThread();
return string.length() > kSizeThreshold && IsMainThread() &&
GetCompressionMode() != CompressionMode::kDisabled;
}
scoped_refptr<ParkableStringImpl> ParkableStringManager::Add(
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment