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

RESOLVED FIXED in mozilla6

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

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

Trunk
mozilla6
x86
Mac OS X
dev-doc-complete, perf
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.

Comment 3

7 years ago
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?
Blocks: 237842
Created attachment 521116 [details] [diff] [review]
Part 1: Add the new RemoveObjectsAt API to nsCOMArray
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #521116 - Flags: review?(benjamin)
Created attachment 521122 [details] [diff] [review]
Part 2: Optimize nsContentUtils::RemoveScriptBlocker to minimize the number of memmove's
Attachment #521122 - Flags: review?(bzbarsky)
Hmm, this second patch manages to leak all of the objects that someone might think of!  I don't understand the leak yet.
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.
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

6 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: 6 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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.