559779 - Optimize nsAttrValue's string (de)allocation

Status: NEW

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

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase

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)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

This very quick patch helps ~10% with the testcase, but might not be good in general.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #1)
> This very quick patch
er, quickly written

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 439545 [details] [diff] [review]
wip


> nsAttrValue::Shutdown()
> {
>   delete sEnumTableArray;
>   sEnumTableArray = nsnull;
>+  if (gCachedBuffer) {
>+    gCachedBuffer->Release();
gCachedBuffer = nsnull;

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Make sure tracing works

This is a bit better testcase.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 439583 [details] [diff] [review]
wip

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?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

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.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 440739 [details] [diff] [review]
v2

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.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 440739 [details] [diff] [review]
v2

Resetting request here until there are some numbers.

Comment 16

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

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.