263973 - [FIX]Make it possible to use StringUnicharInputStream without allocating

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

If we know our string data will outlive the stream, we should be able to tell it
to just use the data without taking ownership of it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 161877 [details] [diff] [review]
Patch 

Comments?  Note that I don't want to just use nsAString here, with assignment
into an nsString, because the string coming from JS when inline style is being
set ends up just being an nsDependentString around the JS string ptr/length. 
Since all the stream _really_ wants is a PRUnichar* pointer and a length, it
can use the dependent string without copying, but would have to copy to get an
nsString out of it.

Comment 3

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

Comment on attachment 161877 [details] [diff] [review]
Patch 

>-                                 nsString* aString);
>+                                 const nsAFlatString* aString,

How about nsSubstring?

Also, how hard would it be to just make it take const nsSubstring& and assign
to its own nsString object, which in many cases wouldn't copy?	Would that
cause additional copies for any callers?

> nsresult StringUnicharInputStream::Close()
> {
>   mPos = mLen;
>-  if (nsnull != mString) {
>+  if (mString && mOwnsString) {
>     delete mString;
>-    mString = 0;
>+    mString = nsnull;
>   }

You want to assign null outside the if.

Comment 4

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

(In reply to comment #3)
> Also, how hard would it be to just make it take const nsSubstring& and assign
> to its own nsString object, which in many cases wouldn't copy?	Would that
> cause additional copies for any callers?

Feel free to ignore this now that I read comment 2.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I can do nsSubstring if that's now the preferred thing, sure.

And yes, the assignment should be outside the if.  Will do.

Comment 6

[Darin Fisher]

Comment on attachment 161877 [details] [diff] [review]
Patch 

>Index: xpcom/io/nsIUnicharInputStream.h

>+ * it destroys the string if aTakeOwnership is set.  If aTakeOwnership
>+ * is not st, you must ensure that the string outlives the stream!

    "is not set"

Do you want to mention how it destroys the string (i.e., that it calls
|delete aString;|).


> extern NS_COM nsresult
>   NS_NewStringUnicharInputStream(nsIUnicharInputStream** aInstancePtrResult,
>+                                 const nsAFlatString* aString,

nsAFlatString is a typedef for nsString, and really serves no purpose
anymore.  I'd rather see us eliminate nsAFlatString, and just use 
nsString in its place.	Fewer string types is better ;-)

also, why not allow |const nsAString *|?  you can easily get a single
fragment buffer from a nsAString like so:

  const nsAString &str = ...;

  nsAString::const_iterator begin;
  str.BeginReading(begin);

  const PRUnichar *buf = begin.get();

then you would not need to use PromiseFlatString at the callsites.


>Index: xpcom/io/nsUnicharInputStream.cpp

>+  NS_PRECONDITION(aString, "null ptr");
...
>+  if (!aString) {
>     return NS_ERROR_NULL_POINTER;
>   }

maybe just use this instead:

    NS_ENSURE_ARG_POINTER(aString);


r=darin

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, good point.  I keep forgetting "all strings are flat".

I'll change to const nsAString& with the iterator fu as needed.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

xpcom/io/nsIUnicharInputStream.h
 If aTakeOwnership
+ * is not st, you must ensure that the string outlives the stream!

"st" should be "set", I suppose :-)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 10

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

Comment on attachment 161921 [details] [diff] [review]
Updated to comments

>+  if (mString && mOwnsString) {
>     delete mString;
>   }

Some compilers don't like deleting a const pointer.  You probably need

nsAString* mutable_string = NS_CONST_CAST(nsAString*, mString);
delete mutable_string;

Comment 11

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

Comment on attachment 161921 [details] [diff] [review]
Updated to comments

with my previous comment (twice), sr=dbaron

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to comments

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fixed.