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()....

So to put it more precisely, the following: nsAutoString foo; foo.Truncate(); should have Truncate() be a no-op, imo.

> 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?

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.

> 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.

> 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.

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.

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

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...

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

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

Comment on attachment 198825 [details] [diff] [review] v1 patch r+sr=bzbarsky

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.

fixed-on-trunk

could this have caused bug 311676?

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. :(

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.

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....

> 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.

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.

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

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

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.

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.

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

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

fixed-on-trunk