869836 - Use {Append,Assign,Equals}Literal where possible

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

Comment 1

[Alfred Kayser]

This also applies to .Equals("...")

Comment 2

[Alfred Kayser]

Attachment: V1: Most of it sed replacements and a for nsDASHMPDParser.cpp some manual changes

Comment 3

[Alfred Kayser]

Attachment: V2: Some more converts

Comment 4

[Alfred Kayser]

https://tbpl.mozilla.org/?tree=Try&rev=bb7bcf6f639f

Comment 5

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

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

Comment 6

[Alfred Kayser]

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.

Comment 7

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

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

Comment 8

[Jeff Walden [:Waldo]]

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.

Comment 10

[Biru [:poiru]]

Attachment: Part 1: Use EqualsLiteral instead of `== NS_LITERAL_STRING(...)`

Comment 11

[Biru [:poiru]]

Attachment: Part 2: Use AppendLiteral instead of `Append(NS_LITERAL_STRING(...))`

Comment 12

[Biru [:poiru]]

Attachment: Part 3: Use `Append('c')` instead of `AppendLiteral("c")`

Comment 13

[Biru [:poiru]]

Attachment: Part 4: Use EqualsLiteral instead of `Equals(NS_LITERAL_STRING(...))`

Comment 14

[Biru [:poiru]]

Attachment: Part 5: Use AssignLiteral instead of Assign where possible

Comment 15

[Biru [:poiru]]

Attachment: Part 6: Use EqualsLiteral instead of Equals where possible

Comment 16

[Biru [:poiru]]

Attachment: Part 7: Use AppendLiteral instead of Append where possible

Comment 17

[Biru [:poiru]]

Attachment: Part 8: Use `Append('c')` instead of `Append("c")`

Comment 18

[Biru [:poiru]]

Attachment: Part 9: Use AssignLiteral instead of `Assign(NS_LITERAL_STRING(...))`

Try push: https://tbpl.mozilla.org/?tree=Try&rev=76c86bd0d84b

Comment 19

[(no longer active)]

I doubt that I'll get to these reviews this week, sorry about that...

Comment 20

[Alfred Kayser]

Nice that this is picked up!

Comment 21

[(no longer active)]

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!

Comment 22

[Biru [:poiru]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2970d506fa3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb77a9a1ee9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e712a8ea3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/59c2ce52a920
https://hg.mozilla.org/integration/mozilla-inbound/rev/441c66cbe3d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4debbbbf42
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a64b22632bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff0eb85179cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/75287df86a00

Comment 23

[Biru [:poiru]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54424cdeceef

Comment 24

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/e26ec6db851c to fix more Windows build bustage.

Comment 25

[[e-mail]]

(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

Comment 26

[[e-mail]]

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.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2970d506fa3a
https://hg.mozilla.org/mozilla-central/rev/eb77a9a1ee9b
https://hg.mozilla.org/mozilla-central/rev/b8e712a8ea3f
https://hg.mozilla.org/mozilla-central/rev/59c2ce52a920
https://hg.mozilla.org/mozilla-central/rev/441c66cbe3d7
https://hg.mozilla.org/mozilla-central/rev/ff4debbbbf42
https://hg.mozilla.org/mozilla-central/rev/1a64b22632bc
https://hg.mozilla.org/mozilla-central/rev/ff0eb85179cd
https://hg.mozilla.org/mozilla-central/rev/75287df86a00
https://hg.mozilla.org/mozilla-central/rev/54424cdeceef
https://hg.mozilla.org/mozilla-central/rev/e26ec6db851c

Comment 28

[[e-mail]]

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

Comment 29

[[e-mail]]

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

Comment 30

[[e-mail]]

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.

Comment 31

[[e-mail]]

Attachment: Part 9a: Use AssignLiteral(MOZ_UTF16(...)) instead of Assign(NS_LITERAL_STRING(...))

Comment 32

[Biru [:poiru]]

(In reply to [e-mail] 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)?

Comment 33

[Biru [:poiru]]

Attachment: Part 10: Use Truncate instead of `AssignLiteral("")`

Comment 34

[Biru [:poiru]]

Attachment: Part 11: Use `Append('c')` instead of `AppendLiteral("c")`

Comment 35

[Biru [:poiru]]

Attachment: Part 12: Use `Assign('c')` instead of `AssignLiteral("c")`

Comment 36

[(no longer active)]

(In reply to Birunthan Mohanathas [:poiru] from comment #32)
> (In reply to [e-mail] 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?

Comment 37

[Biru [:poiru]]

(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

[(no longer active)]

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

Comment 39

[Biru [:poiru]]

(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

[(no longer active)]

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

Comment 41

[Biru [:poiru]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b8bbe7aa1cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/09385232f753
https://hg.mozilla.org/integration/mozilla-inbound/rev/64a0e8806b22
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfaa8aa62370

Comment 42

[[e-mail]]

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

Comment 43

[Biru [:poiru]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59eefc86501a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0180fb7eec1

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/09385232f753
https://hg.mozilla.org/mozilla-central/rev/64a0e8806b22
https://hg.mozilla.org/mozilla-central/rev/cfaa8aa62370
https://hg.mozilla.org/mozilla-central/rev/e0180fb7eec1

Comment 45

[Biru [:poiru]]

I'll continue this in a new bug later.