48486 - LiteralImpl has a nsAutoString member, should it?

Status: VERIFIED FIXED

(Core Graveyard :: RDF, defect, P3)

Description

[John Bandhauer]

about:bloat shows:

76 LiteralImpl    164    52152      382      318 (  166.28 +/-   101.20)     
4319      482 (  306.04 +/-   140.82)

I see we leak LiteralImpls too.

Does the speed or the likely length of the contents justify the use of 
nsAutoString?

Comment 1

[Chris Waterson]

jband: no, even use of nsString is not justified here. Could you file leaks on 
me as a separate bug.

Comment 2

[Chris Waterson]

Attachment: use bare PRUnichar* instead of nsAutoString

Comment 3

[John Bandhauer]

r=jband

P.S. I can't reproduce the leak I saw. It was after some odd assert after a JS 
code screwup that I 'ignored'. Looks like a fslse alarm on the leak.

Comment 4

[Chris Waterson]

Attachment: better fix; fuse allocation of object & string

Comment 5

[Chris Waterson]

cc'ing scc, who may know better C++ kung fu...what do you think of the last
patch? Is there a "preferred" way of doing what I'm trying to do?

Comment 6

[Scott Collins]

for the implementation of |PRUnichar* nsCRT::strcpy(PRUnichar* aDest, const 
PRUnichar* aSource)|, you'll want to use |wmemcpy| if it is available.

You can't glue together the object with data using a member variable at the end 
being the start of the array.  It's just too dangerous.  The C++ compiler does 
not give you enough guarantees about object layout to make this work.  The right 
way to do this is shown in

  <http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsSharedString.h#174>

don't use the array allocator, use raw |operator new|, then placement |new| (line 
186) to get construction to happen.  The math at the beginning of this function 
ensures proper alignment of the variable sized data buffer.

In fact, is there any reason that |nsShared[C]String| couldn't be your answer 
here?  It doesn't carry much overhead per instance: just a vptr, a reference 
count, the length of the string, and (unfortunately) a pointer to the databuffer 
that falls just off the end of the structure, see

  <http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsSharedString.h#88>

No biggie if even this is too much, but use this construction scheme as your 
guide.

Comment 7

[Chris Waterson]

Attachment: patch that incorporates scc's feedback

Comment 8

[Chris Waterson]

scc: incorporated your feedback, modulo:

1. wmemcpy() would require us to strlen(); is that really going to be better? I
suppose I could improve on the code in LiteralImpl::Create() to do a memcpy()...

2. Can't use nsSharedString b/c need to support nsIRDFLiteral. In all other ways
this is an nsIAtom/nsSharedString. Oh well, we'll fix it in RDF 2.0.

Let me know if any other comments...

Comment 9

[Chris Waterson]

FWIW, I'd estimate memory savings are ~140 bytes per LiteralImpl (raw object
size down to 12 bytes from 164; assume the string is five to ten characters --
ten to twenty bytes). I created about 600 LiteralImpl objects doing a "typical"
run, including mail/news. So this is on the order of a 50KB to 100KB win.

Comment 10

[Chris Waterson]

Attachment: final patch that scc and I worked out.

Comment 11

[Chris Waterson]

fixed

Comment 12

[Tom Everingham]

verified