XPCReadableJSStringWrapper is too malloc happy.

VERIFIED FIXED in mozilla0.9.9

Status

()

Core
XPConnect
P2
normal
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: jst, Assigned: jst)

Tracking

({perf})

Trunk
mozilla0.9.9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [HAVE FIX])

Attachments

(2 attachments)

(Assignee)

Description

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

Comment 1

16 years ago
Created attachment 65569 [details] [diff] [review]
Don't allocate non-shared buffer handles.
(Assignee)

Comment 2

16 years ago
I believe this patch is ready for reviews, anyone wanto have a look?
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P2
Whiteboard: [HAVE FIX]
Target Milestone: --- → mozilla0.9.9

Comment 3

16 years ago
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.
Attachment #65569 - Flags: superreview+
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.
(Assignee)

Comment 5

16 years ago
Created attachment 65678 [details] [diff] [review]
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 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.
Attachment #65678 - Flags: review+
(Assignee)

Comment 7

16 years ago
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

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

sr=jag
Attachment #65678 - Flags: superreview+
(Assignee)

Comment 9

16 years ago
Fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 10

16 years ago
Marking Verified -
Status: RESOLVED → VERIFIED

Updated

16 years ago
Blocks: 92580
You need to log in before you can comment on or make changes to this bug.