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

RESOLVED FIXED in mozilla6

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks 1 bug, {dev-doc-complete, perf})

Trunk
mozilla6
x86
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Keywords: perf
nsCOMArray doesn't have such an API, but nsTArray does.  We should switch, imo.
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.
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.
(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.
(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?
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #521116 - Flags: review?(benjamin)
Hmm, this second patch manages to leak all of the objects that someone might think of!  I don't understand the leak yet.
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.
Attachment #521116 - Attachment is obsolete: true
Attachment #521116 - Flags: review?(benjamin)
Attachment #521268 - Flags: review?(benjamin)
Comment on attachment 521122 [details] [diff] [review]
Part 2: Optimize nsContentUtils::RemoveScriptBlocker to minimize the number of memmove's

r=me
Attachment #521122 - Flags: review?(bzbarsky) → review+
bsmedberg: ping?

Updated

8 years ago
Attachment #521268 - Flags: review?(benjamin) → review+
http://hg.mozilla.org/mozilla-central/rev/57494f03fba3
http://hg.mozilla.org/mozilla-central/rev/ff2e6be782da

(dev-doc-needed for the new nsCOMArray API)
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: dev-doc-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Documentation updated:

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

Also added a mention to Firefox 6 for developers.
You need to log in before you can comment on or make changes to this bug.