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

RESOLVED FIXED in mozilla1.9beta3

Status

()

P2
normal
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: jst, Assigned: jst)

Tracking

({memory-footprint, perf})

unspecified
mozilla1.9beta3
memory-footprint, perf
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
Created attachment 292899 [details] [diff] [review]
Wrong diff.

XPCStringConvert::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)
(Assignee)

Updated

11 years ago
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)
(Assignee)

Comment 1

11 years ago
Created attachment 292900 [details] [diff] [review]
Don't always allocate new string wrappers.
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.

Updated

11 years ago
Priority: -- → P2
(Assignee)

Comment 3

11 years ago
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.
(Assignee)

Comment 5

11 years ago
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...
(Assignee)

Comment 6

11 years ago
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.
(Assignee)

Comment 7

11 years ago
Created attachment 293358 [details] [diff] [review]
Reserve space in XPCCallContext for string wrappers, but only initialize it as it's needed.

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+
(Assignee)

Comment 9

11 years ago
Created attachment 293480 [details] [diff] [review]
Updated diff for checkin.

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?
(Assignee)

Comment 11

11 years ago
Created attachment 293609 [details] [diff] [review]
Fix that landed (including bustage fixes).
(Assignee)

Comment 12

11 years ago
Fix checked in, marking FIXED.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.