Closed Bug 408139 Opened 14 years ago Closed 14 years ago

XPCStringConvert::JSStringToReadable() is slow and malloc happy.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: jst, Assigned: jst)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(4 files, 1 obsolete file)

Attached patch Wrong diff. (obsolete) — Splinter Review
PCStringConvert::JSStringToReadable() tens to show up in profiles when looking at where we spend time going between JS and XPCOM. The reason is primarily due to us calling new on an XPCReadableJSStringWrapper every time the function is called. Rather than doing that, we could keep a cached string wrapper in thread local storage and use that, as long as it's not already used. That would mean that as long as we're not nesting calls through XPConnect, we'll use the cache, which is the common case by far. See attached patch.
Flags: blocking1.9+
Attachment #292899 - Flags: superreview?(bzbarsky)
Attachment #292899 - Flags: review?(jonas)
Attachment #292899 - Attachment description: Don't always allocate new string wrappers. → Wrong diff.
Attachment #292899 - Attachment is obsolete: true
Attachment #292899 - Flags: superreview?(bzbarsky)
Attachment #292899 - Flags: review?(jonas)
Attachment #292900 - Flags: superreview?(bzbarsky)
Attachment #292900 - Flags: review?(jonas)
We'll also miss the cache if a function takes more than one string argument, right?  Do we have any data on how often we miss the cache and on how much bumping the cache size up to 2 strings helps cache misses?

I'd seen this in the profiles myself, and had been wondering whether it would be possible to do stack allocation here or something... but haven't thought of anything yet.
Priority: -- → P2
What we could fairly easily do is to take the code I wrote and move it into XPCCallContext, and make it have a number (2 probably) of strings as part of it. Then we could use those to avoid allocating, and that way nested calls wouldn't be hurt by the cached strings already being in use. I'll code that up and see what it does...
The thing with putting it into the XPCCallContext is that we'll pay the construction cost for those strings on every context construction.... not sure we want that.
Yes, but they're nsDependentStrings, whose constructors are noise compared to the rest of the work that's done in the XPCCallContext ctor. We could alternatively let users of XPCCallContext pass in an array of strings (and bits for telling whether they're in use or not, and a count etc), or we could make XPCCallContext have space for the strings, but only call the ctors when someone requests them. Neither is particularly pretty...
FWIW, using a string wrapper cache capable of holding only one string in a XPCCallContext (i.e. on the stack, so nesting doesn't impact cache availability), we end up allocating ~130 string wrappers per window. If we bump the size of the cache up to 2, then we don't allocate a single string wrapper starting up.
bz, what do you think about this? This only calls the string ctor/dtor when the strings are actually being used, and there cache can hold two string wrappers. With this patch we don't allocate a single string wrapper on startup.
Attachment #293358 - Flags: superreview?(bzbarsky)
Attachment #293358 - Flags: review?(bzbarsky)
Attachment #292900 - Flags: superreview?(bzbarsky)
Comment on attachment 293358 [details] [diff] [review]
Reserve space in XPCCallContext for string wrappers, but only initialize it as it's needed.

This is missing the js/src/xpconnect/src/xpcwrappednative.cpp
 change.  I assume that change is the same as in the previous patch?

>+XPCCallContext::DeleteString(nsAString *string)
>+            // One of our internal string is no 

"strings"

r+sr=bzbarsky with the commment nit fixed and the xpcwrappednative.cpp change added.
Attachment #293358 - Flags: superreview?(bzbarsky)
Attachment #293358 - Flags: superreview+
Attachment #293358 - Flags: review?(bzbarsky)
Attachment #293358 - Flags: review+
Indeed, that change got lost in the last diff. Same code, except calling DeleteString() on the call context rather than on the thread local storage data.

Thanks for the reviews!
Hmm.  Could we also add an assert in the xpccallcontext dtor that the cache is not in use by that point?
Fix checked in, marking FIXED.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Depends on: 639162
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.