Use {Append,Assign,Equals}Literal where possible

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: alfredkayser, Assigned: poiru)

Tracking

Trunk
mozilla32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments, 2 obsolete attachments)

14.13 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
102.27 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
100.91 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
34.61 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
24.63 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
40.86 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
63.73 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
21.14 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
30.48 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
16.33 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
12.66 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
16.13 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
25.01 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
Reporter

Description

6 years ago
There are lots of place where Append(NS_LITERAL_STRING("...")) is used, but AppendLiteral("...") is shorter and faster as it prevents the creation of the String object. Also there are a number of cases where a string of length 1 is added, in which case a Append('c') is enough. 

For example:
http://mxr.mozilla.org/mozilla-central/source/netwerk/dash/mpd/nsDASHMPDParser.cpp#136:
136   for(int32_t i = 0; i < offset; i++)
137     ss.Append(NS_LITERAL_STRING(" "));
...
141   ss += NS_LITERAL_STRING("<");


Full list:
http://mxr.mozilla.org/mozilla-central/search?string=.Append%28NS_LITERAL_STRING

Same applies for .Assign(NS_LITERAL_STRING)
Reporter

Comment 1

6 years ago
This also applies to .Equals("...")
Reporter

Comment 3

6 years ago
Posted patch V2: Some more converts (obsolete) — Splinter Review
Attachment #747317 - Attachment is obsolete: true
Reporter

Updated

6 years ago
Attachment #747856 - Flags: review?(dbaron)
(In reply to Alfred Kayser from comment #0)
> There are lots of place where Append(NS_LITERAL_STRING("...")) is used, but
> AppendLiteral("...") is shorter and faster as it prevents the creation of
> the String object. Also there are a number of cases where a string of length
> 1 is added, in which case a Append('c') is enough. 

Did you measure that it's faster, or are you guessing that it's faster?
Flags: needinfo?(alfredkayser)
Reporter

Comment 6

6 years ago
There is not a specific measurement or test (except a synthetic one (calling Append("...")  and AppendLiteral("...") a million times). 
The "proof" is simple:
Append(NS_LITERAL always creates an object structure around the static string, whereas AppendLiteral not.
As for Append('c'): instead of creating an object, just the one character is added to the string.
Flags: needinfo?(alfredkayser)
Comment on attachment 747856 [details] [diff] [review]
V2: Some more converts

ok, I guess I'm ok with the concept, then.

Handing the actual review of the code off to waldo (though I also considered jlebar).
Attachment #747856 - Flags: review?(dbaron) → review?(jwalden+bmo)
Comment on attachment 747856 [details] [diff] [review]
V2: Some more converts

Review of attachment 747856 [details] [diff] [review]:
-----------------------------------------------------------------

If we're serious about this, and I don't see why we shouldn't be, we should (in a followup patch, no need to overload one patch with both changes, especially when this'll probably bitrot easily) add this:

  template<size_t N>
  void Append(const char (&data)[N]) MOZ_DELETE;
  template<size_t N>
  void NS_FASTCALL Assign(const char (&data)[N]);
  // AssignLiteral("c") should be Assign('c')
  void AssignLiteral(const char (&data)[2]) MOZ_DELETE;

Then it'll be *impossible* to call Append or Assign with a string literal.  (We probably should do similarly to AppendASCII called with a single-character string, but I think you have to do some acrobatics to do that, that might not all be writable directly inline in the class definition.  Still worth doing.)

It looks like the same treatment should probably be done to operator+= as well.

But followup-patch-land for all of this, certainly.
Attachment #747856 - Flags: review?(jwalden+bmo) → review+
Assignee

Updated

5 years ago
Assignee: alfredkayser → birunthan
Summary: Replace Append(NS_LITERAL_STRING("...")) with AppendLiteral("...") → Use {Append,Assign,Equals}Literal where possible
Version: unspecified → Trunk
Assignee

Updated

5 years ago
Duplicate of this bug: 869837

Comment 19

5 years ago
I doubt that I'll get to these reviews this week, sorry about that...
Reporter

Comment 20

5 years ago
Nice that this is picked up!

Updated

5 years ago
Attachment #8420688 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #8420689 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #8420690 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #8420691 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #8420692 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #8420693 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #8420694 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #8420695 - Flags: review?(ehsan) → review+

Comment 21

5 years ago
Comment on attachment 8420696 [details] [diff] [review]
Part 9: Use AssignLiteral instead of `Assign(NS_LITERAL_STRING(...))`

Review of attachment 8420696 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work!
Attachment #8420696 - Flags: review?(ehsan) → review+
(In reply to Alfred Kayser)
> There are lots of place where Append(NS_LITERAL_STRING("...")) is used, but
> AppendLiteral("...") is shorter and faster as it prevents the creation of
> the String object.

Note that as of bug 514173 you can now write AppendLiteral(MOZ_UTF16("...")). It doesn't have to inflate the characters as it copies, but obviously the UTF16 literal consumes twice as much memory as the ASCII literal would.

AssignLiteral(MOZ_UTF16("...")) just makes the target depend on the UTF16 literal, which is really fast. (AppendLiteral does this if the target string was originally empty.) InsertLiteral also has a UTF16 overload.

I seem to have forgotten to add a UTF16 overload for EqualsLiteral though, anyone want to file a bug for that?

template<int N>
inline bool EqualsLiteral( const char_type (&str)[N] ) const
{
  return Equals(str, N - 1);
}
#ifdef CharT_is_PRUnichar
inline bool EqualsLiteral( const char (&str)[N] ) const
{
  return EqualsASCII(str, N - 1);
}
#endif
In fact bug 514173 made Assign(NS_LITERAL_STRING("...")) share the underlying string literal instead of copying, so switching to AssignLiteral("...") might actually reduce performance.
(In reply to comment #25)
> I seem to have forgotten to add a UTF16 overload for EqualsLiteral though

...because there's no Equals(const char_type*, size_type) overload.
(In reply to Jeff Walden from comment #8)
> If we're serious about this, and I don't see why we shouldn't be, we should
> (in a followup patch, no need to overload one patch with both changes,
> especially when this'll probably bitrot easily) add this:
> 
>   template<size_t N>
>   void Append(const char (&data)[N]) MOZ_DELETE;
>   template<size_t N>
>   void NS_FASTCALL Assign(const char (&data)[N]);
>   // AssignLiteral("c") should be Assign('c')
>   void AssignLiteral(const char (&data)[2]) MOZ_DELETE;
> 
> Then it'll be *impossible* to call Append or Assign with a string literal. 
No it won't, because Append(const char *) is a better overload match (because it's not a template), so the compiler ignores the deleted operator altogether.
I just checked and there's no point creating a wide version of EqualsLiteral as the code would be almost identical to EqualsASCII. (Except when using -Og, gcc is able to simplify the comparison loop to use x86 array indexing addressing modes. When using -Og gcc needs you to specify a size_t array index instead of pointer arithmetic in order to generate reasonable code.)

The same argument does not necessarily apply to InsertLiteral or AppendLiteral, depending on whether memcpy is more efficient than a naïve loop. (The string code explicitly invokes memcpy in this case. In one of my tests I was able to persuade gcc to transform my manual copy loop into a call to memcpy but I was unable to reproduce the result.) And I definitely don't think it applies to AssignLiteral now that bug 514173 has stopped it from copying when the character size matches.

Updated

5 years ago
Attachment #8428392 - Flags: review?(ehsan) → review+
Assignee

Comment 32

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #29)
> (In reply to Jeff Walden from comment #8)
> > If we're serious about this, and I don't see why we shouldn't be, we should
> > (in a followup patch, no need to overload one patch with both changes,
> > especially when this'll probably bitrot easily) add this:
> > 
> >   template<size_t N>
> >   void Append(const char (&data)[N]) MOZ_DELETE;
> >   template<size_t N>
> >   void NS_FASTCALL Assign(const char (&data)[N]);
> >   // AssignLiteral("c") should be Assign('c')
> >   void AssignLiteral(const char (&data)[2]) MOZ_DELETE;
> > 
> > Then it'll be *impossible* to call Append or Assign with a string literal. 
> No it won't, because Append(const char *) is a better overload match
> (because it's not a template), so the compiler ignores the deleted operator
> altogether.

We could use static_assert, though. Should we do it?

If we do, there are cases like `AssignLiteral(SOME_DEFINE)` where SOME_DEFINE may cause the assertion to fail in some configurations. Should we simply change those cases to Assign or should we have a non-asserting variant of the function (e.g. AssignAnyLiteral)?
Flags: needinfo?(ehsan)

Comment 36

5 years ago
(In reply to Birunthan Mohanathas [:poiru] from comment #32)
> (In reply to neil@parkwaycc.co.uk from comment #29)
> > (In reply to Jeff Walden from comment #8)
> > > If we're serious about this, and I don't see why we shouldn't be, we should
> > > (in a followup patch, no need to overload one patch with both changes,
> > > especially when this'll probably bitrot easily) add this:
> > > 
> > >   template<size_t N>
> > >   void Append(const char (&data)[N]) MOZ_DELETE;
> > >   template<size_t N>
> > >   void NS_FASTCALL Assign(const char (&data)[N]);
> > >   // AssignLiteral("c") should be Assign('c')
> > >   void AssignLiteral(const char (&data)[2]) MOZ_DELETE;
> > > 
> > > Then it'll be *impossible* to call Append or Assign with a string literal. 
> > No it won't, because Append(const char *) is a better overload match
> > (because it's not a template), so the compiler ignores the deleted operator
> > altogether.
> 
> We could use static_assert, though. Should we do it?
> 
> If we do, there are cases like `AssignLiteral(SOME_DEFINE)` where
> SOME_DEFINE may cause the assertion to fail in some configurations. Should
> we simply change those cases to Assign or should we have a non-asserting
> variant of the function (e.g. AssignAnyLiteral)?

What are you proposing to static_assert?
Flags: needinfo?(ehsan)
Assignee

Comment 37

5 years ago
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #36)
> What are you proposing to static_assert?

For example:

  template<int N>
  void AssignLiteral( const char_type (&str)[N] )
  {
    static_assert(N != 1, "Use Truncate instead");
    static_assert(N != 2, "Use Assign instead");
    AssignLiteral(str, N - 1);
  }

Comment 38

5 years ago
(In reply to comment #37)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #36)
> > What are you proposing to static_assert?
> 
> For example:
> 
>   template<int N>
>   void AssignLiteral( const char_type (&str)[N] )
>   {
>     static_assert(N != 1, "Use Truncate instead");
>     static_assert(N != 2, "Use Assign instead");
>     AssignLiteral(str, N - 1);
>   }

Oh, right.  Yeah, I think that would work.  I wouldn't worry about the constant issue unless if you run into it while trying this out.

Updated

5 years ago
Attachment #8428710 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #8428711 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #8428712 - Flags: review?(ehsan) → review+
Assignee

Comment 39

5 years ago
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #38)
> Oh, right.  Yeah, I think that would work.  I wouldn't worry about the
> constant issue unless if you run into it while trying this out.

I actually did run in to it (e.g. http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#286). Any thoughts on the solutions described in comment 32?

Comment 40

5 years ago
(In reply to Birunthan Mohanathas [:poiru] from comment #39)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #38)
> > Oh, right.  Yeah, I think that would work.  I wouldn't worry about the
> > constant issue unless if you run into it while trying this out.
> 
> I actually did run in to it (e.g.
> http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpHandler.cpp#286). Any thoughts on the solutions described in comment
> 32?

Oh crap!  Is that defined to a 1 or 2 letter string anywhere?

I guess falling back to using Assign() for these cases would work for me.
(In reply to Birunthan Mohanathas from comment #37)
> For example:
> 
>   template<int N>
>   void AssignLiteral( const char_type (&str)[N] )
>   {
>     static_assert(N != 1, "Use Truncate instead");
>     static_assert(N != 2, "Use Assign instead");
>     AssignLiteral(str, N - 1);
>   }

Perhaps you could write
void AssignLiteral( const char_type (&str)[1] )
{
  Truncate();
}
but the difference between AssignLiteral(MOZ_UTF16(""))* and Truncate() is going to be very minor**, and AssignLiteral(MOZ_UTF16("x"))* is still better than Assign('x') if you're not going to subsequently mutate the string (or even if you are in some edge cases, as it might avoid reallocating the string).

* Or plain AssignLiteral for 8-bit string classes.
** Even less now that Truncate() also points the buffer to a readonly string.
Assignee

Comment 45

5 years ago
I'll continue this in a new bug later.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.