Last Comment Bug 656607 - Reduce the use of FromData on nsStringBuffer in atom implementations
: Reduce the use of FromData on nsStringBuffer in atom implementations
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla7
Assigned To: :Ehsan Akhgari (out sick)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-12 07:33 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2011-06-13 09:45 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (4.91 KB, patch)
2011-05-12 07:37 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review
Patch (v1) (7.48 KB, patch)
2011-05-12 10:43 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Patch (v2) (3.01 KB, patch)
2011-06-07 13:52 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Patch (v2) (1.09 KB, patch)
2011-06-07 13:53 PDT, :Ehsan Akhgari (out sick)
benjamin: review+
Details | Diff | Review
Let the compiler do the pointer arithmetics (1.37 KB, patch)
2011-06-08 15:44 PDT, :Ehsan Akhgari (out sick)
benjamin: review+
jonas: review+
Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2011-05-12 07:33:34 PDT
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] 2011-05-12 07:37:16 PDT
Created attachment 531929 [details] [diff] [review]
WIP
Comment 2 :Ehsan Akhgari (out sick) 2011-05-12 07:38:52 PDT
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 :Ehsan Akhgari (out sick) 2011-05-12 10:43:56 PDT
Created attachment 531983 [details] [diff] [review]
Patch (v1)
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-12 11:52:59 PDT
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) PTO Until July 5th 2011-05-12 13:34:11 PDT
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) PTO Until July 5th 2011-05-12 13:39:22 PDT
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] 2011-05-12 13:57:49 PDT
(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) PTO Until July 5th 2011-05-12 14:23:51 PDT
That would work for atoms, but not for strings.
Comment 9 :Ehsan Akhgari (out sick) 2011-05-12 15:00:47 PDT
FWIW: http://tbpl.mozilla.org/?tree=Try&rev=d8078fc9279e
Comment 10 :Ehsan Akhgari (out sick) 2011-05-12 15:43:04 PDT
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) PTO Until July 5th 2011-05-12 17:32:40 PDT
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 [:bz] (Out June 25-July 6) 2011-05-12 17:37:57 PDT
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 [:bsmedberg] 2011-06-07 08:07:53 PDT
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 :Ehsan Akhgari (out sick) 2011-06-07 13:52:44 PDT
Created attachment 537861 [details] [diff] [review]
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 :Ehsan Akhgari (out sick) 2011-06-07 13:53:57 PDT
Created attachment 537862 [details] [diff] [review]
Patch (v2)

Sorry, the previous version contained unrelated debugging changes.
Comment 16 Benjamin Smedberg [:bsmedberg] 2011-06-08 13:07:22 PDT
Comment on attachment 537862 [details] [diff] [review]
Patch (v2)

Might be nice to use C++-style reinterpret_cast, but sure.
Comment 17 :Ehsan Akhgari (out sick) 2011-06-08 15:44:23 PDT
Created attachment 538131 [details] [diff] [review]
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) PTO Until July 5th 2011-06-08 17:54:47 PDT
Comment on attachment 538131 [details] [diff] [review]
Let the compiler do the pointer arithmetics

Now THIS I can get behind! :)
Comment 19 :Ehsan Akhgari (out sick) 2011-06-09 15:36:07 PDT
http://hg.mozilla.org/mozilla-central/rev/eff4d5de06f4

Note You need to log in before you can comment on or make changes to this bug.