112548 - integrate atoms with the string hierarchy

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

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

I think it would be useful to integrate atoms with the string hierarchy in some
way.  We spend a good bit of time (close to 1% of new-window time for the
navigator window) coping strings out of atoms.

Perhaps the best solution would be to make AtomImpl::ToString assign from an
nsSharedBufferHandleWithAllocator<PRUnichar> whose allocator holds a reference
count on the atom.  The cleanest way to do this would perhaps be to, within that
ToString method, construct a string object on the stack that implemented |const
nsAFlatString| and returned an appropriate new buffer handle if
GetSharedBufferHandle were called.  (It could even hold on to the buffer handle
and free it in the destructor if the caller didn't add a reference.)  This might
require implementation (and renaming) of kIsShared so that we don't try to
modify the buffer, though!

Comment 1

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

A better solution than allocating a new buffer handle would be to either:
 * put the buffer handle inside the atom
 * give the atom class a pointer to the buffer handle, so that once we call
ToString on an atom once, we have the buffer handle as long as the atom lasts.

Comment 2

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

...but we can't do the first of the above because we call |delete| on the buffer
handle.

Comment 3

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

...nor the second.

Comment 4

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

Attachment: working patch, although with too much memory churn

I'd like to get the changes in mozilla/string in for this milestone, since they
fix two serious bugs with nsSharableString (although one of the two fixes may
be a temporary solution -- see the comment in the patch).

Comment 5

[jag (Peter Annema)]

Comment on attachment 61050 [details] [diff] [review]
working patch, although with too much memory churn

r=jag

Comment 6

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

Comment on attachment 61050 [details] [diff] [review]
working patch, although with too much memory churn

I moved the mozilla/string part of this patch to bug 114438.

Comment 7

[Scott Collins]

giving up ancient string bugs to the new string owner.  jag, you'll want to sort
through these and see which ones still apply and go with or against the
direction in which you intend strings evolve