656607 - Reduce the use of FromData on nsStringBuffer in atom implementations

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

The FromData method causes a cast alignment warning when building with clang and it's a weird kind of method anyways. It would be good to use a more type safe approach.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: WIP

Comment 2

[(no longer active)]

Example warning from Clang:

nsSMILCSSProperty.cpp
In file included from /Users/ehsanakhgari/moz/mozilla-central/content/smil/nsSMILCSSProperty.cpp:40:
In file included from /Users/ehsanakhgari/moz/mozilla-central/content/smil/nsSMILCSSProperty.h:44:
In file included from ../../dist/include/nsIAtom.h:19:
../../dist/include/nsStringBuffer.h:109:18: warning: cast from 'char *' to 'nsStringBuffer *' increases required alignment from 1 to 4 [-Wcast-align]
          return (nsStringBuffer*) ( ((char*) data) - sizeof(nsStringBuffer) );
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Comment 3

[(no longer active)]

Attachment: Patch (v1)

Comment 4

[Boris Zbarsky [:bzbarsky]]

This adds extra arithmetic on all the atom string access, right?  That seems undesirable....

On the other hand, I guess before we had that extra arithmetic at the FromData() callsites.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't understand why this only touches atom code? Surely our string code uses FromData in order to addref/release string buffers?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Another solution to this is to not cast down to char* and do the adjustment there, but rather to cast to PRUint32* and adjust the offset.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #4)
> This adds extra arithmetic on all the atom string access, right?  That seems
> undesirable....
> 
> On the other hand, I guess before we had that extra arithmetic at the
> FromData() callsites.

In the worst case we could always have a type safe wrapper of PRUnichar* that knows it's actually nsStringBuffer and but keeps the arithmetic in the same places.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That would work for atoms, but not for strings.

Comment 9

[(no longer active)]

FWIW: http://tbpl.mozilla.org/?tree=Try&rev=d8078fc9279e

Comment 10

[(no longer active)]

Can somebody try to explain to me why nsStringBuffer is necessary at all?  I have a hard time figuring out what problem it's trying to solve, and I wonder if there isn't a better way to solve that problem.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

A nsStringBuffer represents a refcounted buffer which holds characters. This allows us to share, rather than copy, data between nsString instances.

(IIRC there was a very large drop both in used cycles and in used memory when we added them, though the cycle drop was hard to measure since the same patch made other perf-improving changes as well).

When modifying a string which currently uses a nsStringBuffer we do a copy if the buffers refcount is larger than 1. Otherwise we modify the buffer in-place.

I.e. nsStringBuffers is how we implement copy-on-write for strings.

The reason atoms use nsStringBuffers is because atoms are basically interned strings. So we can share data between strings and atoms, both when creating atoms, and when reading string-data out from atoms.

We also use nsStringBuffers to allow nsAttrValues to share data with strings.

Comment 12

[Boris Zbarsky [:bzbarsky]]

We also use nsStringBuffers to implement the limited sharing we have between XPCOM strings and JSString.  Also, some places in style data hold string buffers directly.

Templating stringbuffers on the char type would be a fine idea if it helps things.

Comment 13

[Benjamin Smedberg]

Comment on attachment 531983 [details] [diff] [review]
Patch (v1)

I really don't think that this is a useful patch. We should template nsStringBuffer, in an ideal world, but the pointer math it does isn't actually wrong, and making this change just to reduce the incidence of warnings doesn't seem helpful.

Comment 14

[(no longer active)]

Attachment: Patch (v2)

How about just silencing the warnings, like we did in bug 659546?  These warnings are extremely annoying when building with Clang, and hinder the usage of the rest of the compiler diagnostics (which are actually useful!).

Comment 15

[(no longer active)]

Attachment: Patch (v2)

Sorry, the previous version contained unrelated debugging changes.

Comment 16

[Benjamin Smedberg]

Comment on attachment 537862 [details] [diff] [review]
Patch (v2)

Might be nice to use C++-style reinterpret_cast, but sure.

Comment 17

[(no longer active)]

Attachment: Let the compiler do the pointer arithmetics

Actually Neil gave me a better idea on IRC.  How about we let the compiler do its job (figuring out the pointer arithmetics) instead of doing that manually?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 538131 [details] [diff] [review]
Let the compiler do the pointer arithmetics

Now THIS I can get behind! :)

Comment 19

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/eff4d5de06f4