Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Reduce the use of FromData on nsStringBuffer in atom implementations

RESOLVED FIXED in mozilla7

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jrmuizel, Assigned: Ehsan)

Tracking

unspecified
mozilla7
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 531929 [details] [diff] [review]
WIP
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) );
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Summary: Reduce the use of FromData on nsStringBuffer → Reduce the use of FromData on nsStringBuffer in atom implementations
Created attachment 531983 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Attachment #531929 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #531983 - Flags: review?(benjamin)

Comment 4

6 years ago
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.
I don't understand why this only touches atom code? Surely our string code uses FromData in order to addref/release string buffers?
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.
(Reporter)

Comment 7

6 years ago
(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.
That would work for atoms, but not for strings.
FWIW: http://tbpl.mozilla.org/?tree=Try&rev=d8078fc9279e
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.
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

6 years ago
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 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.
Attachment #531983 - Flags: review?(benjamin)
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!).
Attachment #531983 - Attachment is obsolete: true
Attachment #537861 - Flags: review?(benjamin)
Created attachment 537862 [details] [diff] [review]
Patch (v2)

Sorry, the previous version contained unrelated debugging changes.
Attachment #537861 - Attachment is obsolete: true
Attachment #537861 - Flags: review?(benjamin)
Attachment #537862 - Flags: review?(benjamin)
Comment on attachment 537862 [details] [diff] [review]
Patch (v2)

Might be nice to use C++-style reinterpret_cast, but sure.
Attachment #537862 - Flags: review?(benjamin) → review+
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?
Attachment #537862 - Attachment is obsolete: true
Attachment #538131 - Flags: review?(benjamin)
Comment on attachment 538131 [details] [diff] [review]
Let the compiler do the pointer arithmetics

Now THIS I can get behind! :)
Attachment #538131 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/eff4d5de06f4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Attachment #538131 - Flags: review?(benjamin) → review+
You need to log in before you can comment on or make changes to this bug.