643885 - Optimize nsContentUtils::RemoveScriptBlocker to minimize the number of memmove's

Status: RESOLVED FIXED

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

Description

[(no longer active)]

nsContentUtils::RemoveScriptBlocker removes the script blockers run on by one, each of which incurs a memmove.  This is basically unnecessary.  If we have an nsCOMArray API for removing multiple consecutive elements, we can optimize it by just doing this work at the end of the loop, I guess.

This was noted when profiling the testcase in bug 237842.

Comment 1

[Boris Zbarsky [:bzbarsky]]

nsCOMArray doesn't have such an API, but nsTArray does.  We should switch, imo.

Comment 2

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

Yeah. I'd like to move towards making nsCOMArray just be a typedef of nsTArray<nsCOMPtr<T>>, but that'll likely take longer than we want to wait for.

Comment 3

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

Also, IIRC nsCOMArray releases its internal buffer when doing Clear().
So it might make sense to for it to keep some buffer alive all the time - 
say when the buffer is less than 64 items or something.

Comment 4

[(no longer active)]

(In reply to comment #1)
> nsCOMArray doesn't have such an API, but nsTArray does.  We should switch, imo.

nsVoidArray (the underlying store) supports it (nsVoidArray::RemoveElementsAt), which is why I said that we could add an equivalent nsCOMArray API.

Comment 5

[(no longer active)]

(In reply to comment #3)
> Also, IIRC nsCOMArray releases its internal buffer when doing Clear().
> So it might make sense to for it to keep some buffer alive all the time - 
> say when the buffer is less than 64 items or something.

That sounds like a good idea to me.  Can you please file a bug for that?

Comment 6

[(no longer active)]

Attachment: Part 1: Add the new RemoveObjectsAt API to nsCOMArray

Comment 7

[(no longer active)]

Attachment: Part 2: Optimize nsContentUtils::RemoveScriptBlocker to minimize the number of memmove's

Comment 8

[(no longer active)]

Hmm, this second patch manages to leak all of the objects that someone might think of!  I don't understand the leak yet.

Comment 9

[(no longer active)]

Attachment: Part 1: Add the new RemoveObjectsAt API to nsCOMArray

My implementation of RemoveObjectsAt had a bug which showed up when trying to delete the last item in the array (i.e., RemoveObjectsAt(count-1,1)).  This version of the patch fixes the bug and adds a test for it.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 521122 [details] [diff] [review]
Part 2: Optimize nsContentUtils::RemoveScriptBlocker to minimize the number of memmove's

r=me

Comment 11

[(no longer active)]

bsmedberg: ping?

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/57494f03fba3
http://hg.mozilla.org/mozilla-central/rev/ff2e6be782da

(dev-doc-needed for the new nsCOMArray API)

Comment 13

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/XPCOM_array_guide#Deleting_objects

Also added a mention to Firefox 6 for developers.