311456 - Calling Truncate() on an nsAutoString makes next append work hard

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

The same problem happens with SetLength(0) (which is what Truncate() does).

Indeed, calling nsSubstring::SetLength just calls through to
nsSubstring::SetCapacity.  Which does:

506         if (capacity == 0)
507           {
508             ::ReleaseData(mData, mFlags);
509             mData = NS_CONST_CAST(char_type*, char_traits::sEmptyBuffer);
510             mLength = 0;
511             SetDataFlags(F_TERMINATED);
512           }

which among other things makes Capacity() return -1... as in, we've just
forgotten that nice stack-allocated buffer that's the whole point of using an
nsAutoString.  We remember about it later on when we're in
nsTSubstring_CharT::MutatePrep and we discover that our capacity is -1 and check
our flags, etc, but this still seems a little painful.  Especially since we then
have to mess with the "old data" that we never cared about in ReplacePrep.

I ran into this while profiling some CSS parsing.  What happens there is we
append chars to an nsAutoString one char at a time after first calling
Truncate()....

Comment 1

[Boris Zbarsky [:bzbarsky]]

So to put it more precisely, the following:

nsAutoString foo;
foo.Truncate();

should have Truncate() be a no-op, imo.

Comment 2

[Darin Fisher]

> We remember about it later on when we're in
> nsTSubstring_CharT::MutatePrep and we discover that our capacity is -1 and check
> our flags, etc, but this still seems a little painful.

So, what exactly is the bug then?

Comment 3

[Boris Zbarsky [:bzbarsky]]

The bug is that the first modification to an nsAutoString after a Truncate()
takes a lot more time than it does if you just have a new nsAutoString that you
never truncated.  That is, we do a lot more work in that case.

Viewed another way, the bug is that Truncate() on a brand-new nsAutoString
actually changes the state of the string.

Comment 4

[Darin Fisher]

> That is, we do a lot more work in that case.

I think I'm questioning that claim.  Can you show that the longer path through
ReplacePrep is costly?  Does this additional work really show up as a
significant cost in profiling?

IIRC, we want SetLength(0) to actually release memory, so maybe the solution
here is to make SetCapacity check the class flags.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Can you show that the longer path through ReplacePrep is costly? 

As far as I can tell it's a major cost in CSS parsing (5% of total at least). 
How that would compare to the shorter path I can't say without profiling both,
of course...

And yes, checking the flags in SetCapacity would make sense -- if we're already
using our fixed buffer we should just keep doing that, imo.

Comment 6

[Darin Fisher]

Attachment: v1 patch

OK, so I think we can optimize by comparing |length| to |mLength|.  If the
length is not changing, then we can just return early.	In the case where
mLength is zero, then we expect that there is no benefit to calling ReleaseData
because the string was either just initialized or it was previously truncated.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'll profile this tonight and see how it looks.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Something else I tried

Well, that patch didn't really help.  Neither does this one.

So it looks like you were right -- the cost of re-grabbing our fixed buffer as
needed is negligible.  :(

I guess that means there are no easy wins in the string code here -- it's
taking as much time as it is because there's just so much arithmetic going on
and because it's being called a lot...

Comment 9

[Boris Zbarsky [:bzbarsky]]

I guess this is invalid.  I'll file a more general bug on the interaction
between strings and CSS scanner here.  :(

Comment 10

[Darin Fisher]

It may be the case that there isn't much win here, but I still like the
short-cut path through SetLength.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 198825 [details] [diff] [review]
v1 patch

r+sr=bzbarsky

Comment 12

[Darin Fisher]

The v1 patch appears to make Truncate 3x faster when called on an empty string.
 Of course, it only matters if that call pattern occurs a lot, but what the
heck.  I'll commit the change on the trunk.

Comment 13

[Darin Fisher]

fixed-on-trunk

Comment 14

[Steve England [:stevee]]

could this have caused bug 311676?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Yes, it did (tested by local backout).  My first guess is this changed what the
(PRUni)char* operator on some nsXPIDLC?String does in some random case, probably
in editor.  :(

Comment 16

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

I backed this out, at least for now, because of bug 311676.  This certainly
doesn't seem intended to break API compatibility, but it seems like it broke at
least some nsXPIDL[C]String cases.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Ok, so I found one thing that fails with this patch (the one I think is biting
bug 311903).  Let |str| be an nsAutoString and consider the following call:

str.SetIsVoid(PR_TRUE);

Now SetIsVoid sets F_VOIDED on str, and calls Truncate(); Truncate(), with this
patch, just bails out.  So now we have mData pointing to our fixed buffer, but
F_VOIDED set (which should not be happening, as I understand it).

Still not sure how that causes problems with Assign....

Comment 18

[Darin Fisher]

> Let |str| be an nsAutoString and consider the following call:
> 
> str.SetIsVoid(PR_TRUE);
> 
> Now SetIsVoid sets F_VOIDED on str, and calls Truncate(); Truncate(), with this
> patch, just bails out.  So now we have mData pointing to our fixed buffer, but
> F_VOIDED set (which should not be happening, as I understand it).

No, that is working as expected.  SetIsVoid should ensure that mData points to
an empty buffer.  nsXPIDLString looks at the IsVoid flag to determine if .get()
should return null instead of whatever its mData points to.  You see, mData can
never be null.

Perhaps the bug is caused by the fact that Truncate() on an empty nsXPIDLString
causes F_VOIDED to be removed from mFlags?  And, now with the patch we've
introduced here, F_VOIDED is preserved when mLength == the argument passed to
SetLength.

Comment 19

[Boris Zbarsky [:bzbarsky]]

That wouldn't explain why the patch in bug 311903 helped.  All it did was change:

  str.SetIsVoid(PR_TRUE);
  str.Assign(str2);

to

  str.SetIsVoid(PR_TRUE);
  str.SetIsVoid(PR_FALSE);
  str.Assign(str2);

where str ought to be an nsAutoString, I believe.

The upshot was that for some reason str was ending up with F_VOID set even
though it had data, so when it was reflected into JS it ended up null instead of
as a JSString.

Comment 20

[Darin Fisher]

OK, could it be that str2 was null in that case?  i.e., was it a null valued
|const char*|?

Comment 21

[Boris Zbarsky [:bzbarsky]]

I don't know, but I doubt it was, since the change led to text appearing instead
of null, if I understood Blake correctly.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Boris understands what I said. What was happening there was that after all was
said and done, when XPConnect was dealing with the return value, it was taking
the string (which had an mData pointing to what I'd input), but testing
IsVoid(), which was returning true, and ignoring the value.

Comment 23

[Darin Fisher]

Ah, I got it now.  The bug is that Assign only modifies mFlags when it is forced
to increase the capacity available at mData.  This can include making mData
point at an nsAutoString's fixed buffer.  Assign should always clear F_VOIDED.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Ah, indeed.  That would make a lot of sense.

Comment 25

[Darin Fisher]

Attachment: v1.1 patch

Clear F_VOIDED whenever the string is mutated, and throw in the SetLength optimization.

Comment 26

[Darin Fisher]

fixed-on-trunk