Open Bug 559779 Opened 12 years ago Updated 4 years ago

Optimize nsAttrValue's string (de)allocation


(Core :: DOM: Core & HTML, defect, P5)





(Reporter: smaug, Unassigned)


(Blocks 1 open bug)



(3 files, 3 obsolete files)

Attached file testcase (obsolete) —
nsAttrValue seem to allocate and release quite a bit in this artificial testcase.
We could perhaps try to reuse nsStringBuffers or something like that.

	1.2%	100.0%	libgklayout.dylib	nsGenericHTMLElement::SetAttribute(nsAString_internal const&, nsAString_internal const&)
	1.7%	82.3%	libgklayout.dylib	 nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, int)
	5.9%	75.3%	libgklayout.dylib	  nsGenericElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, int)
	6.9%	34.4%	libgklayout.dylib	   nsGenericElement::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAString_internal const&, nsAttrValue&, unsigned char, int, int, nsAString_internal const*)
	1.3%	17.1%	libgklayout.dylib	    nsAttrAndChildArray::SetAndTakeAttr(nsIAtom*, nsAttrValue&)
	0.4%	15.2%	libgklayout.dylib	     nsAttrValue::Reset()
	0.6%	14.3%	libxpcom_core.dylib	      nsStringBuffer::Release()
	0.3%	0.3%	libnspr4.dylib	      PR_AtomicDecrement
	0.1%	0.1%	libgklayout.dylib	      dyld_stub_nsStringBuffer::Release()
	0.0%	0.0%	libmozalloc.dylib	      moz_free
	0.0%	0.0%	libSystem.B.dylib	      free
	0.5%	0.5%	libgklayout.dylib	     nsAttrValue::SwapValueWith(nsAttrValue&)
	0.1%	0.1%	libxpcom_core.dylib	     nsStringBuffer::Release()
	1.2%	2.3%	libgklayout.dylib	    nsHTMLDivElement::IsAttributeMapped(nsIAtom const*) const
	0.9%	1.8%	libgklayout.dylib	    nsGenericHTMLElement::AfterSetAttr(int, nsIAtom*, nsAString_internal const*, int)
	1.5%	1.5%	libgklayout.dylib	    nsContentUtils::RemoveScriptBlocker()
	1.2%	1.4%	libgklayout.dylib	    nsNodeUtils::AttributeChanged(nsIContent*, int, nsIAtom*, int)
	1.4%	1.4%	libgklayout.dylib	    nsContentUtils::AddScriptBlocker()
	0.9%	0.9%	libgklayout.dylib	    nsIContent::IntrinsicState() const
	0.6%	0.6%	libgklayout.dylib	    nsContentUtils::IsEventAttributeName(nsIAtom*, int)
	0.2%	0.2%	libgklayout.dylib	    dyld_stub_nsCOMPtr_base::~nsCOMPtr_base()
	0.1%	0.1%	libgklayout.dylib	    nsGenericElement::FindAttributeDependence(nsIAtom const*, nsGenericElement::MappedAttributeEntry const* const*, unsigned int)
	0.1%	0.1%	libxpcom_core.dylib	    nsCOMPtr_base::~nsCOMPtr_base()
	0.1%	0.1%	libgklayout.dylib	    nsAttrValue::Reset()
	0.1%	0.1%	libgklayout.dylib	    nsAttrValue::SwapValueWith(nsAttrValue&)
	0.0%	0.0%	libgklayout.dylib	    nsStubMutationObserver::AttributeChanged(nsIDocument*, nsIContent*, int, nsIAtom*, int)
	1.0%	15.6%	libgklayout.dylib	   nsAttrValue::SetTo(nsAString_internal const&)
	1.1%	12.8%	libgklayout.dylib	    nsAttrValue::GetStringBuffer(nsAString_internal const&) const
	0.4%	6.2%	libxpcom_core.dylib	     nsStringBuffer::Alloc(unsigned long)
	1.0%	1.9%	libxpcom_core.dylib	     CopyUnicodeTo(nsAString_internal const&, unsigned int, unsigned short*, unsigned int)
	0.9%	0.9%	libSystem.B.dylib	     malloc
	0.8%	0.8%	libSystem.B.dylib	     __memcpy
	0.7%	0.7%	libmozalloc.dylib	     moz_malloc
	0.5%	0.5%	libgklayout.dylib	     dyld_stub_nsStringBuffer::Alloc(unsigned long)
	0.5%	0.5%	libxpcom_core.dylib	     nsStringBuffer::FromString(nsAString_internal const&)
	0.1%	0.1%	libgklayout.dylib	     dyld_stub_nsStringBuffer::FromString(nsAString_internal const&)
	0.1%	0.1%	libgklayout.dylib	     dyld_stub_CopyUnicodeTo(nsAString_internal const&, unsigned int, unsigned short*, unsigned int)
	1.0%	1.0%	libxpcom_core.dylib	    nsStringBuffer::FromString(nsAString_internal const&)
	0.7%	0.7%	libxpcom_core.dylib	    nsStringBuffer::Alloc(unsigned long)
	0.1%	0.1%	libxpcom_core.dylib	    CopyUnicodeTo(nsAString_internal const&, unsigned int, unsigned short*, unsigned int)
Attached patch wip (obsolete) — Splinter Review
This very quick patch helps ~10% with the testcase, but might not be good in general.
Attached file testcase (obsolete) —
Attachment #439493 - Attachment is obsolete: true
(In reply to comment #1)
> This very quick patch
er, quickly written
Comment on attachment 439545 [details] [diff] [review]

> nsAttrValue::Shutdown()
> {
>   delete sEnumTableArray;
>   sEnumTableArray = nsnull;
>+  if (gCachedBuffer) {
>+    gCachedBuffer->Release();
gCachedBuffer = nsnull;
Attached patch wipSplinter Review
Attachment #439545 - Attachment is obsolete: true
This is a bit better testcase.
Attachment #439546 - Attachment is obsolete: true
Comment on attachment 439583 [details] [diff] [review]

This speeds up the updated testcase over 20%.
Reusing stringbuffer is the thing which helps most in this
particular case, but even just using realloc and
not releasing so much helps in other cases.

Jonas, what do you think about this?
Attachment #439583 - Flags: feedback?(jonas)
The problem I have with this is that it feels a lot like optimizing for test suites.

This only helps if you are setting the same attribute to a value with the same length over and over. Or if you are toggling between setting it to the empty string and the same value over and over.

Something as simple as:

for (...) {
  a.setAttribute("foo", "bar");
  a.setAttribute("foo", "baar");

won't be helped at all. If we want to create something that helps with real world cases I think we need a smarter and bigger cache. Or at least running tests on some real world website showing that it helps there.
Allocation/deallocation shows up very high up when setting or clearing
string attributes. malloc/free are slow.
So we clearly need to do something there, if we want setAttr to be reasonable fast.

But sure, perhaps the patch is optimizing too much for a simple testcase.
I'll think about something more generic cache.
Attached patch v2Splinter Review
This caches small string buffers. The idea is to keep this all very simple,
but still allow significant speed ups in cases where small attributes values
are changed a lot.
If more complex caching would be used, handling that cache would start to
cost more.

Dromaeo doesn't seem to test "string" attributes.
Gmail doesn't use them either too much, but Gmaps use them a bit.
And so does FF UI.
Attachment #440739 - Flags: review?(jonas)
Comment on attachment 440739 [details] [diff] [review]

I still would have liked to see some actual data being collected. For example to show that 25 is a good limit, or to see if storing more than one buffer per length makes any difference.

Did you see any improvements in gmaps? I think facebook might be also be a good candidate.
This all is a lot more for perf tests than for actual web sites.
(Same thing could be said about many Dromaeo/V8/Sunspider bugs).

But I'll try to provide some data here.
But anyway, we need to optimize
StringBuffer usage in nsAttrValue somehow. Currently it is just terribly heavy.
I totally agree that we need to optimize, and I like the general approach in this patch. I'd just like to base the parameters on something more firm than just guessing.

For what it's worth, I don't think we need to do perf measurements, just pull some numbers on how often strings of various sizes are allocated or some such.
Just from memory, gmail didn't seem to use string attributes too much.
Gmaps does use. And the length was often around 20 chars, IIRC, but
there were also some attributes which were about 60-64 chars.

But I'll re-test.
Comment on attachment 440739 [details] [diff] [review]

Resetting request here until there are some numbers.
Attachment #440739 - Flags: review?(jonas)

Move all DOM bugs that haven't been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.