Allow Oilpan heap objects account for their external allocations.

Source/core/dom/Text.cpp

Index: Source/core/dom/Text.cpp
diff --git a/Source/core/dom/Text.cpp b/Source/core/dom/Text.cpp
index c983fbd9874729c34dc3b92ddc3c86b7b47abf15..9db26bb8de3df3b6c77c654958f9fa5f0c32f1d2 100644
--- a/Source/core/dom/Text.cpp
+++ b/Source/core/dom/Text.cpp
@@ -42,13 +42,48 @@
 
 namespace blink {
 
+#if ENABLE(OILPAN)
+namespace {
+// If the external string kept by a Text node exceed this threshold length,

[haraken, 2015/02/19 23:47:43]

exceeds

+// Oilpan is informed. External allocation amounts owned by heap objects are
+// taken into account when scheduling urgent Oilpan GCs.
+//
+// FIXME: having only Text nodes with strings above an ad-hoc local threshold
+// influence Oilpan's GC behavior isn't a satisfactory long-term solution.
+// But code that allocates a lot of Text nodes in tight loops, and not much more,
+// we do have to trigger Oilpan GCs to avoid PartitionAlloc OOMs. The accounting
+// does add overhead on the allocation of every Text node however, so for now, only
+// register those above the given threshold. TBC.
+const size_t stringLengthThreshold = 256;
+
+enum RegistrationMode { AsNew, AsAlive };
+
+void registerExternalAllocation(size_t length, RegistrationMode mode)
+{
+    if (length > stringLengthThreshold) {

[haraken, 2015/02/19 23:47:43]

Does it really make sense to have the threshold?

Previously we were using a similar threshold to report memory used by NodeList
to V8. However, it turned out that it can end up mis-estimating the amount of
memory used by NodeLists and it actually caused OOM (when a ton of NodeLists
each of which is smaller than the threshold are allocated). So we eliminated the
threshold from the NodeList.

[sof, 2015/02/20 09:33:48]

I added it to prevent having e.g. all smaller Text nodes that the parser
generates being accounted for - it seemed like needless overhead & wanting to
strike a balance in that regard.

So it makes sense from that POV, but it is somewhat hokey overall, hence the
accompanying comment.

[haraken, 2015/02/23 08:42:27]

Mostly convinced but let me ask one more question: Is it really faster to have
the if branch rather than unconditionally dispatching
Heap::increaseExternallyAllocatedBytes?

[sof, 2015/02/23 09:13:37]

The latter would entail an atomicAdd() which will require a lock-prefixed add
instruction on (e.g.) x86. Which will bring about a multicore-lock and a cache
flush. "Hundreds of cycles".

If we want to keep it simple&regular just take that overhead per Text node
allocation, then that's an option. Another option is to try to arrange so that
only script-induced Text node allocations are accounted for (I haven't
investigated if this is possible.)

[haraken, 2015/02/23 09:26:34]

Thanks, the current code looks reasonable to me.

I'm not sure if we want to add this kind of threshold code to each file we call
Heap::increaseExternallyAllocatedBytes, but we can worry about the problem when
we hit the problem in the future.

+        if (mode == AsNew)
+            Heap::increaseExternallyAllocatedBytes(length);
+        else
+            Heap::increaseExternallyAllocatedBytesAlive(length);
+    }
+}
+
+} // namespace
+#endif
+
 PassRefPtrWillBeRawPtr<Text> Text::create(Document& document, const String& data)
 {
+#if ENABLE(OILPAN)
+    registerExternalAllocation(data.length(), AsNew);
+#endif
     return adoptRefWillBeNoop(new Text(document, data, CreateText));
 }
 
 PassRefPtrWillBeRawPtr<Text> Text::createEditingText(Document& document, const String& data)
 {
+#if ENABLE(OILPAN)
+    registerExternalAllocation(data.length(), AsNew);
+#endif
     return adoptRefWillBeNoop(new Text(document, data, CreateEditingText));
 }
 
@@ -408,6 +443,14 @@ PassRefPtrWillBeRawPtr<Text> Text::cloneWithData(const String& data)
     return create(document(), data);
 }
 
+#if ENABLE(OILPAN)
+void Text::trace(Visitor* visitor)
+{
+    registerExternalAllocation(m_data.length(), AsAlive);
+    CharacterData::trace(visitor);
+}
+#endif
+
 #ifndef NDEBUG
 void Text::formatForDebugger(char *buffer, unsigned length) const
 {