120718 - XPCReadableJSStringWrapper is too malloc happy.

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P2)

Description

[Johnny Stenback (:jst)]

Every time a string is passed as a DOMString/AString from JS to C++ through
XPConnect and the C++ code touches the string, the string code ends up having to
allocate a buffer handle. The allocations (new's) show up when profiling DOM
code, and these allocations accounts for ~450 of the mallocs we do when starting
mozilla and immediately exiting. I bet these allocations happen quite frequently
while running the browser too. Patch coming up that makes the
XPCReadableJSStringWrapper code only allocate the buffer handle when a shared
handle is requested, not when a non-shared handle is requested. This is done at
the expense of increasing the size of the XPCReadableJSStringWrapper class, but
instances of the XPCReadableJSStringWrapper class are always transient, they're
never held in memory  for a long time.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Don't allocate non-shared buffer handles.

Comment 2

[Johnny Stenback (:jst)]

I believe this patch is ready for reviews, anyone wanto have a look?

Comment 3

[jag (Peter Annema)]

Comment on attachment 65569 [details] [diff] [review]
Don't allocate non-shared buffer handles.

Looks good, sr=jag. I'd appreciate it if dbaron could be the r= since this
builds on his code.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 65569 [details] [diff] [review]
Don't allocate non-shared buffer handles.

I did something similar in my work on AtomString that I never finished, and I'd
like to do something similar for nsXPIDL[C]String...

mBufferHandle should just be an nsBufferHandle<PRUnichar> -- why use a 5-word
handle when you can use a 2-word handle?

Perhaps you could rename |GetVoidBufferHandle| to |GetSharedEmptyBufferHandle|
for consistency with other string classes?  (Unless you meant it to set the
kIsNULL bit.)

For |GetBufferHandle|, I see no reason that you need the |!mStr| check -- won't
mBufferHandle always be valid?

I think you'll also want to override GetFlatBufferHandle so that calling
PromiseFlatString doesn't lead to the creation of a shared buffer handle -- it
can just call GetBufferHandle.

Comment 5

[Johnny Stenback (:jst)]

Attachment: Incorporate dbaron's suggestions

Thanks dbaron, how about this version? I decided to get rid of
GetVoidBufferHandle() and put the code inline in GetSharedBufferHandle() since
that's the only place it's needed (as you suggested).

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 65678 [details] [diff] [review]
Incorporate dbaron's suggestions

r=dbaron, although I'm not sure why you want to change it from inheriting from
...WithAllocator to ...WithDestroy.

Comment 7

[Johnny Stenback (:jst)]

Because then I don't need an allocator, if there are clear benefits to using
...WithAllocator over ...WithDestroy then I could switch back, let me know if I
should...

Comment 8

[jag (Peter Annema)]

Comment on attachment 65678 [details] [diff] [review]
Incorporate dbaron's suggestions

sr=jag

Comment 9

[Johnny Stenback (:jst)]

Fixed.

Comment 10

[Phil Schwartau]

Marking Verified -