If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
String
--
enhancement
RESOLVED FIXED
12 years ago
4 years ago

People

(Reporter: bz, Assigned: Darin Fisher)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla1.9alpha1
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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()....
Blocks: 311458
So to put it more precisely, the following:

nsAutoString foo;
foo.Truncate();

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

Comment 2

12 years ago
> 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.
(Assignee)

Comment 4

12 years ago
> 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.
(Assignee)

Comment 6

12 years ago
Created attachment 198825 [details] [diff] [review]
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.
Assignee: nobody → darin
Status: NEW → ASSIGNED
Attachment #198825 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
Severity: normal → enhancement
Target Milestone: --- → mozilla1.9alpha
I'll profile this tonight and see how it looks.
Created attachment 198839 [details] [diff] [review]
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...
I guess this is invalid.  I'll file a more general bug on the interaction
between strings and CSS scanner here.  :(
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → INVALID
Attachment #198825 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

12 years ago
It may be the case that there isn't much win here, but I still like the
short-cut path through SetLength.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Comment on attachment 198825 [details] [diff] [review]
v1 patch

r+sr=bzbarsky
Attachment #198825 - Flags: superreview+
Attachment #198825 - Flags: review+
(Assignee)

Comment 12

12 years ago
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.
(Assignee)

Comment 13

12 years ago
fixed-on-trunk
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
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.  :(
Depends on: 311676
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 311903
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....
(Assignee)

Comment 18

12 years ago
> 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.
(Assignee)

Comment 20

12 years ago
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.
(Assignee)

Comment 23

12 years ago
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.
(Assignee)

Comment 25

12 years ago
Created attachment 210644 [details] [diff] [review]
v1.1 patch

Clear F_VOIDED whenever the string is mutated, and throw in the SetLength optimization.
Attachment #198825 - Attachment is obsolete: true
Attachment #210644 - Flags: review?(bzbarsky)
Attachment #210644 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 26

12 years ago
fixed-on-trunk
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.