Open
Bug 559779
Opened 14 years ago
Updated 8 months ago
Optimize nsAttrValue's string (de)allocation
Categories
(Core :: DOM: Core & HTML, defect, P5)
Tracking
()
NEW
People
(Reporter: smaug, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 3 obsolete files)
2.48 KB,
patch
|
sicking
:
feedback-
|
Details | Diff | Splinter Review |
1.14 KB,
text/html
|
Details | |
2.75 KB,
patch
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•14 years ago
|
||
This very quick patch helps ~10% with the testcase, but might not be good in general.
Reporter | ||
Comment 2•14 years ago
|
||
Attachment #439493 -
Attachment is obsolete: true
Reporter | ||
Comment 3•14 years ago
|
||
(In reply to comment #1) > This very quick patch er, quickly written
Reporter | ||
Comment 4•14 years ago
|
||
Comment on attachment 439545 [details] [diff] [review] wip > nsAttrValue::Shutdown() > { > delete sEnumTableArray; > sEnumTableArray = nsnull; >+ if (gCachedBuffer) { >+ gCachedBuffer->Release(); gCachedBuffer = nsnull;
Reporter | ||
Comment 5•14 years ago
|
||
Attachment #439545 -
Attachment is obsolete: true
Reporter | ||
Comment 6•14 years ago
|
||
This is a bit better testcase.
Attachment #439546 -
Attachment is obsolete: true
Reporter | ||
Comment 7•14 years ago
|
||
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?
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.
Attachment #439583 -
Flags: feedback?(jonas) → feedback-
Reporter | ||
Comment 9•14 years ago
|
||
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.
Reporter | ||
Comment 10•14 years ago
|
||
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] 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.
Reporter | ||
Comment 12•14 years ago
|
||
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.
Reporter | ||
Comment 14•14 years ago
|
||
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] v2 Resetting request here until there are some numbers.
Attachment #440739 -
Flags: review?(jonas)
Comment 16•6 years ago
|
||
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.
Priority: -- → P5
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•