Last Comment Bug 643885 - Optimize nsContentUtils::RemoveScriptBlocker to minimize the number of memmove's
: Optimize nsContentUtils::RemoveScriptBlocker to minimize the number of memmove's
Status: RESOLVED FIXED
: dev-doc-complete, perf
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on:
Blocks: 237842
  Show dependency treegraph
 
Reported: 2011-03-22 13:29 PDT by :Ehsan Akhgari
Modified: 2011-04-22 11:17 PDT (History)
10 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add the new RemoveObjectsAt API to nsCOMArray (6.29 KB, patch)
2011-03-22 22:50 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 2: Optimize nsContentUtils::RemoveScriptBlocker to minimize the number of memmove's (1.61 KB, patch)
2011-03-23 00:05 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review
Part 1: Add the new RemoveObjectsAt API to nsCOMArray (6.56 KB, patch)
2011-03-23 12:08 PDT, :Ehsan Akhgari
benjamin: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2011-03-22 13:29:33 PDT
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 [:bz] 2011-03-22 13:37:31 PDT
nsCOMArray doesn't have such an API, but nsTArray does.  We should switch, imo.
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-03-22 13:44:26 PDT
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] (vacation Aug 25-28) 2011-03-22 14:04:14 PDT
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 :Ehsan Akhgari 2011-03-22 14:57:31 PDT
(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 :Ehsan Akhgari 2011-03-22 14:58:18 PDT
(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 :Ehsan Akhgari 2011-03-22 22:50:53 PDT
Created attachment 521116 [details] [diff] [review]
Part 1: Add the new RemoveObjectsAt API to nsCOMArray
Comment 7 :Ehsan Akhgari 2011-03-23 00:05:59 PDT
Created attachment 521122 [details] [diff] [review]
Part 2: Optimize nsContentUtils::RemoveScriptBlocker to minimize the number of memmove's
Comment 8 :Ehsan Akhgari 2011-03-23 09:32:50 PDT
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 :Ehsan Akhgari 2011-03-23 12:08:41 PDT
Created attachment 521268 [details] [diff] [review]
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 [:bz] 2011-03-23 20:39:41 PDT
Comment on attachment 521122 [details] [diff] [review]
Part 2: Optimize nsContentUtils::RemoveScriptBlocker to minimize the number of memmove's

r=me
Comment 11 :Ehsan Akhgari 2011-04-06 11:26:01 PDT
bsmedberg: ping?
Comment 12 :Ehsan Akhgari 2011-04-14 10:57:28 PDT
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] 2011-04-22 11:17:50 PDT
Documentation updated:

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

Also added a mention to Firefox 6 for developers.

Note You need to log in before you can comment on or make changes to this bug.