231995 - Exploring nsAString "defragmentation"

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P2)

Description

[Darin Fisher]

nsAString, as a string interface, provides great abstraction such that it can
represent all sorts of string implementations (shared string buffers, stack
based string buffers, fragmented string buffers, etc.).  but, this flexibility
is not without cost and complexity.  and there are fundamental bugs that are
difficult if not impossible to fix (e.g., the dreaded
concatenation-of-a-concatenation bug).

in our codebase, multi-fragment strings appear in exactly two places:

1) dependent concatenations of nsAStrings (r = a + b).
2) in the htmlparser (nsSlidingString)

case 1 could be nearly avoided if nsAString::Assign (and related methods)
accepted a reference to a nsDependentConcatenation directly.  there are cases
where it is nice to be able to pass a nsDependentConcatenation as a nsAString,
but that runs the risk of the previously mentioned dreaded
concatenation-of-a-concatenation bug.  (in fact, it is common practice to avoid
passing a+b to a XPCOM method b/c there is no guarantee that the implementation
of that method won't try to insert the argument into another dependent
concatenation.)  hence, you really want to avoid passing dependent
concatenations to arbitrary functions taking nsAString references.  IOW, even
today we typically flatten a dependent concatenation before passing it to an
XPCOM method.  so, there is little value in a dependent concatenation inheriting
from nsAString.

as for case 2, there's good reason to consider a specialized class that performs
the same job as nsSlidingString.  in fact, for the most part references to
nsSlidingString do not make their way out of the htmlparser.  it is only the
parser node interface that exposes nsSlidingSubstring as nsAString so as to
minimize copies.  however, that API is not frozen and can be reworked if it
proves to be a performance issue.

so, what are we left with.  in most cases, the nsAString objects are not
multi-fragmented.  we pay considerable cost all throughout the code to support a
multi-fragmented interface.  nsAString::const_iterator is very bloaty. 
operator++ calls normalize_forward() each time it is invoked.  this adds
unnecessary code bloat when strings are single fragment.

finally, there is the overhead of all the virtual functions.  it is unfortunate
that we pay the cost of accessing string attributes via virtual function calls.
 checking if a string is empty is very costly.  constructing a string is costly.
 string destructors are virtual.  when you remove multi-fragment strings from
the picture, it becomes difficult to justify nsAString's virtual function table.
 we could just have a single base-string implementation that could be used to
implement all of our single fragmented string types (nsString, nsAutoString,
nsXPIDLString, nsDependentString, nsDependentSubstring, nsPromiseFlatString, and
nsPrintfCString).  in fact, there is no good reason for nsAString to have a
virtual function table when you remove multi-fragmented strings from the picture.

ah, but what about embedders?  and the fact that nsAString is frozen?  that's
the nasty issue.  in fact, we cannot remove the vtable because it is part of our
XPCOM ABI.  ok, but maybe we can avoid invoking the vtable when we know that we
are talking to an nsAString that is implemented within xpcom.  (i have a
solution in mind... trust me!)

so, this bug is about exploring the simplification of nsAString so that it no
longer represents multi-fragment strings.  it's further about reducing the
string classes down to a single base class.  my feeling is that this is only
worth doing if the footprint and performance benefits are significant.

i want to use this bug as a place to outline my approach and collect feedback.  

i have created a branch (STRING_20040119_BRANCH), which contains a prototype
implementation.

lastly, we cannot afford to change the way poeple use nsAString.  so, this bug
is not about changing the nsAString API, per se.  i'm only looking at optimizing
the implementation.

Comment 1

[Darin Fisher]

as an update, STRING_20040119_BRANCH contains a working seamonkey build with a
new single fragment only string implementation.  the codesize savings is pretty
good.  on linux, there's a savings of nearly 120K from libxpcom.  there's
savings in all the libraries as well.  as for performance metrics, Tp and Ts
each improved slightly.  i'll be posting more complete data shortly.

Comment 2

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

Attachment: dbaron's code review of changes on STRING_20040119_BRANCH at 2004-01-30 13:50 -0800 outside of xpcom/string/ and htmlparser/

Comment 3

[Darin Fisher]

Attachment: GRE codesize reduction, Linux GCC 3.2.2 [-Os]

Comment 4

[Darin Fisher]

The table I just uploaded shows the codesize savings for a Linux GRE build.  My
previous codesize numbers were based on a comparison between the branch and a
snapshot of the trunk from the day the branch was cut.  Today, I pulled and
built using the STRING_20040119_BASE tag to generate a better baseline.  I'm
glad I did because it seems there was some considerable difference between the
two "baseline" builds.  I trust the one I generated today more than the previous
one.

The total codesize savings for the Linux GRE appears to be on the order of 500 KiB.

Comment 5

[Darin Fisher]

Attachment: Seamonkey codesize reduction, Linux GCC 3.2.2 [-Os]

This attachment shows codesize comparisons for Seamonkey generated in the same
way as the previous GRE codesize comparison.

Comment 6

[Darin Fisher]

Attachment: response to dbaron's review questions (re: attachment 140440)

Comment 7

[Darin Fisher]

Attachment: GRE codesize reduction, Win32 MSVC 6-sp5

Comment 8

[Darin Fisher]

Attachment: Seamonkey codesize reduction, Win32 MSVC 6-sp5

Comment 9

[Alec Flett]

I find it rather fascinating that there is such a difference between the linux
reduction and the windows reduction - Is there something else at play here? Are
we eliminating unused methods that the windows linker was already culling out? 

Comment 10

[Darin Fisher]

well, there are several things at play for sure...

1) NS_COM is still being applied to some of the string classes (like nsString),
and that causes a lot of inline symbols to be exported.  this problem doesn't
exist under linux, so this might explain why the savings in xpcom is not as good
under MSVC.

2) more importantly though, MSVC just generates better code.  take a look at
codesize totals for example: linux seamonkey went from 21148 to 20196, and win32
seamonkey went from 13101 to 12821.  so, that's a 4.5% savings on linux and a
2.1% savings on win32.  in other words, though it appears that we are saving 3-4
times as much on linux, percentage wise the savings is actually just double.

3) remember that business about MSVC not being penalized by early returns as
much as GCC?  (i.e., GCC doesn't coalesce multiple return statements very well,
resulting in a lot of duplicated destructor code.)  i think that might be having
an affect here since it takes less code to call a DSO method than it does to
call a virtual method.  recall: i've made all methods on nsAString non-virtual.

that said, i definitely plan to spend some time looking at the MSVC generated
assembly to see if we are doing something that causes obvious problems for MSVC.

Comment 11

[Alec Flett]

yeah, I know gcc generates some big-ass code, but  the reason I asked about the
exporting symbols is that in this case:

NS_COM int foo();
int bar();

on Windows, only foo() will be exported, and in fact bar() might be culled out
of the resulting dll if nobody inside the DLL uses bar()

on Linux, both of these functions will be "exported" or at least exist in the dll.

In the case of classes, you can be smart on Windows and only declare specific
methods to be NS_COM or you can declare the whole class. But on linux, it
doesn't matter, it all survives.
(now maybe darin knew this, but I figure its good info to share with everyone
else reading)

I'll bet the early-return stuff is a big part of it. does nsAString no longer
have a virtual destructor, too? I wonder if one of the reasons we get such a hit
on early returns is because of inlined virtual destructors (even when they are
empty) - obviously I'm getting into more of an nsCOMPtr issue..probably belongs
in another bug.

I do have a script in tools/module-deps which tries to uncover unused exports
from windows dll. Basically you say "Here's XPCOM.DLL and all of its consumers"
and it hands you back a list of unused, or infrequently used methods... I've
used it to trim down nsString and friends before. Maybe now that nsAString is
non-virtual, we can apply the script and see if there are methods that are
rarely used. I've used it in the past to find things like "wow necko.dll is the
only dll that uses nsString.FindChar(const PRUnichar)!"

Comment 12

[Darin Fisher]

here's some interesting string stats from some light browsing (loaded a 3-4 sites):

nsStringStats
 => mAllocCount: 280878
 => mReallocCount: 98972
 => mFreeCount: 280878
 => mShareCount: 84737
 => mAdoptCount: 9489
 => mAdoptFreeCount: 9489

this output is generated when a debug build exits.  the share count lists how
many times a buffer was shared.  in this case we're avoiding nearly 25% of the
string allocations.

the adopt count is interesting.  those mostly correspond to getter_Copies.  this
string implementation does not allow for sharing of adopted buffers.  since the
number of adopted strings is very small, this seems reasonable.

Comment 13

[Darin Fisher]

Attachment: htmlparser patch v1

this patch includes the htmlparser changes necessary to eliminate
nsSlidingString. 

the following is done:

  - nsScannerString/nsScannerSubstring are introduced as classes not inheriting
from nsAString.  they have no virtual methods, but basically mimic the existing
nsSlidingString code.

  - nsScannerSubstring holds a nsString member that it lazily initializes
whenever someone wants to treat a nsScannerString as if it were a nsAString. 
this is needed by the nsIParserNode API.  there's a dirty flag that is used to
optimize this string flattening operation.

  - nsScannerIterator is introduced and provides the same methods that
nsReadingIterator has.	it is mostly just a copy of today's nsReadingIterator. 
NOTE: the new nsReadingIterator assumes single fragment storage, so we cannot
use it to iterate over a nsScannerSubstring.

  - methods from nsReadableUtils had to be copied over since they need to
operate on nsScannerIterator instead.

for the most part this implementation is as simple as it gets.	i've tried to
disturb the previous implementation as little as possible.  in the future, i
suspect we can optimize the scanner more and in particular the nsIParserNode
API.

Comment 14

[Darin Fisher]

Attachment: xpconnect patch v1

This patch includes changes to xpconnect to eliminate the sharing of strings
between JS and C++.  I'm not sure how wise this is.  However, given 1) the fact
that sharable strings don't propogate very far today and 2) the complexity of
the hash table required to manage the JS-to-C++ string sharing, I think this is
a reasonable change.  This doesn't seem to impact Tp and Ts, but those tests
probably don't exercise this enough.  We'd probably want to run some DOM
performance tests or something, which I know nothing about.

Eventually, this JS-to-C++ sharing code might makes its way back.  I already
have plans to support a "buffer handle" concept in the new string code.  It
will be much simpler than what we have today, but I think that should wait
until the branch lands.... provided this patch doesn't hurt performance too
much.

Comment 15

[Darin Fisher]

Comment on attachment 140649 [details] [diff] [review]
xpconnect patch v1

note the XXX comment.  see dbaron's attached review comment w.r.t. these
xpconnect changes.  thx jst!

Comment 16

[Boris Zbarsky [:bzbarsky]]

I have some "DHTML" perf tests lying about somewhere, I think... lemme know if
you want me to try to dig them up.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 140646 [details] [diff] [review]
htmlparser patch v1

+      // nsScannerIterator( const nsScannerIterator& );		    //
auto-generated copy-constructor OK
+      // nsScannerIterator<CharT>& operator=( const nsScannerIterator& );  //
auto-generated copy-assignment operator OK

You can loose that <CharT> in that comment, right?

- In nsScanner::FillBuffer(void):

+    // XXX we know that mInputStream is non-null !!
+
     if(mInputStream) {

Yeah, we do... wanna remove the check while you're at it?

+      AppendToBuffer(NS_ConvertASCIItoUTF16(buf, numread));

I see two of these... would it be worth adding an AppendASCIItoBuffer() to
avoid an extra copy?

+// XXX looks like this is unused --darin
+nsresult nsScanner::ReadNumber(nsScannerIterator& aStart,
+				nsScannerIterator& aEnd,
				PRInt32 aBase) {

Yup, I'd say remove it.

This looks really good, with the exception of the AsString() getters. But that
can be dealt with later (by moving the copy-and-convert-newlines etc, or
somesuch, into the parser nodes).

r=jst

Comment 18

[Darin Fisher]

thanks for the review comments jst.  i went ahead and added an
AppendASCIItoBuffer method.  it uses LossyConvertEncoding<char, PRUnichar> to
perform the conversion into a pre-allocated nsScannerString::Buffer object.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 140649 [details] [diff] [review]
xpconnect patch v1

This looks good in general, the only thing I'd note is that it seems like it
would be easy enoug to keep sharing strings when crossing into JS from XPCOM,
so it might be worth keeping that code in some shape or form (and darin is
looking into doing that, but probably as a post-initial-landing change).

r=jst

Comment 20

[Darin Fisher]

Attachment: xpcom glue changes v1

this patch is for the glue.  it is really part of the new simplified string API
for embedders.	it makes the glue not require a nsAString vtable
implementation.

the trickiest part is in handling initialization of the glue from a component. 
in the case of a component, xpcom has already been loaded.  however, the
component needs to pass the path of the xpcom library into XPCOMGlueStartup so
it can setup its local copy of the glue.  to get the path to the xpcom library,
the glue currently inspects the directory service.  that yields the nsIFile
pointing to the xpcom library, but the glue needs a way to call GetNativePath. 
that is impossible without an implementation of the nsAString vtable or access
to the C-style API.  unfortunately, it hasn't yet loaded xpcom (catch-22), so
it can't have access to the C-style API!  my solution is to use
PR_FindSymbolAndLibrary to locate NS_GetFrozenFunctions and the current
instance of the xpcom library.	this seems to work great under Linux and
Windows and should work everywhere else too.

PR_FindSymbolAndLibrary is usually pretty fast in this case b/c xpcom will be
one of the libraries that was loaded earlier.  if not loaded explicitly by
PR_LoadLibrary, then it should still work b/c NSPR first searches the "default
module" (whatever that is on each platform).

Comment 21

[Darin Fisher]

jst: actually, it would be even easier now than before to share strings from C++
to JS.  we don't need the hash table that xpcstring.cpp currently maintains! 
the reason is that shared buffers are always prefixed with a header that
contains the reference count, etc.  so, it should be trivial to make
FinalizeDOMString release the shared buffer w/o needing any reference to the
original nsAString.  all it needs is the |PRUnichar*| that the JSString is
holding.  hmm... but, i think i'd still prefer to postpone until after the
branch lands.

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 141017 [details] [diff] [review]
xpcom glue changes v1

Looks great.

Could you fix the "post 1.4" version check:

if (functions->size >= sizeof(XPCOMFunctions)) {

could you fix up the two samples to show that in version 1.6 and above you can
use your new classes?

when do you plan on landing this?  I would like to run a few test cases.  Given
my time constaints, this will take a few days.	

What platforms have you tested a GRE build on?	

Does the same component have any additional imports? 

Do we save any footprint in the sample component?

Comment 23

[Doug Turner (:dougt)]

Comment on attachment 141017 [details] [diff] [review]
xpcom glue changes v1

Looks great.

Could you fix the "post 1.4" version check:

if (functions->size >= sizeof(XPCOMFunctions)) {

could you fix up the two samples to show that in version 1.6 and above you can
use your new classes?

when do you plan on landing this?  I would like to run a few test cases.  Given
my time constaints, this will take a few days.	

What platforms have you tested a GRE build on?	

Does the same component have any additional imports? 

Do we save any footprint in the sample component?

Comment 24

[Darin Fisher]

(In reply to comment #23)
> (From update of attachment 141017 [details] [diff] [review])
> 
> Could you fix the "post 1.4" version check:
> 
> if (functions->size >= sizeof(XPCOMFunctions)) {

whoops!  thanks for catching that!


> could you fix up the two samples to show that in version 1.6 and above you can
> use your new classes?

i'm not sure i understand what you're asking for.  are you talking about
nsSample.cpp and nsTestSample.cpp?  what do you mean by "show that in version
1.6 and above..."?


> when do you plan on landing this?  I would like to run a few test cases.  Given
> my time constaints, this will take a few days.	

i'm not sure when i will get a chance to land this, but i think it is getting
close now.


> What platforms have you tested a GRE build on?	

linux and windows.


> Does the same component have any additional imports? 

actually, it has quite a bit fewer imports.  maybe 6 less?  but they are just
standard ones like calloc, memchr, etc. that were only being pulled into
libtestsample.so because of libembedstring_s.


> Do we save any footprint in the sample component?

the sample component (libtestsample.so) is about 1/2 the size in a stripped
debug build (perhaps a full optimized build would show less savings in the new
world).


thanks doug!

Comment 25

[Darin Fisher]

Some performance numbers
------------------------

Using TestStrings.cpp, a program I wrote to test out various string operations
to make sure that the string code is working correctly, I have collected some
performance numbers.  It's hard to say how this testcase corresponds to reality,
but it is interesting nonetheless.

You can find TestStrings.cpp under xpcom/tests on the STRING_20040119_BRANCH. 
Running this test with a -Os GCC build under Linux, I got these results:

[trunk] time ./TestStrings 100000 > /dev/null
  => 7.723 - 7.859 seconds

[branch] time ./TestStrings 100000 > /dev/null
  => 3.997 - 4.148 seconds

This shows quite a savings.  I think the thing that might make this testcase
unrealistic is the fact that most of the tests involve very short strings.  With
very short strings, the overhead of string function calls really adds up to
something very significant.  obviously if the string data is very large, then
the overhead of the string function calls becomes less important.

I also ran TestStandardURL to generate some before and after performance
numbers.  This test basically exercises the nsStandardURL class.  It's
interesting sine nsIURI performance has historically been significant to
mozilla.  (e.g., SetSpec + GetSpec probably represent roughly 3% of mozilla
startup time.)

Here's the breakdown:

URL    : long bugzilla query URL (so, heavy on the query string component).
cycles : 100000

test name                   base              branch
-----------------------------------------------------------------------
SetSpec                     2057              2046
GetSpec                      170                22
Resolve                      249               171
SetScheme                    109                45
GetScheme                     94                32
[GS]etHost                   546               293
SetPath                     1411               864
GetPath                       97                44
[GS]etQuery                  499               241
[GS]etRef                    467               191

I've also run Ts and Tp numbers on Linux (2 Ghz, 1 Gb RAM), and i only noticed a
slight improvement (on the order of 1-2%).

Comment 26

[Darin Fisher]

i should have mentioned that the TestStandardURL output seemed to vary (roughly)
by +/- 2-3%.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Hmm.. It basically looks like Assign() is about twice as fast with the new
string classes, huh?

Comment 28

[Darin Fisher]

(In reply to comment #27)
> Hmm.. It basically looks like Assign() is about twice as fast with the new
> string classes, huh?

yeah, it's definitely the case that Assign got faster, but you can see that
readonly operations also got faster.  i should note that it is only the GetSpec
case that is improved by string sharing.  the other parts of TestStandardURL do
not involve any string sharing on Assign.  most of the nsStandardURL setters
result in calls to "Replace" a portion of the URL (e.g., SetScheme).

Comment 29

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

Attachment: dbaron's review comments of code in xpcom/string

I already sent darin part of these by email (the part before the last -----),
and he responded by email.

Comment 30

[Darin Fisher]

dbaron and i discussed the remaining issues in his latest review comments.  i've
made revisions accordingly, and i'm now in the process of merging with the trunk.

Comment 31

[Jungshik Shin]

(In reply to comment #20)

> to the C-style API.  unfortunately, it hasn't yet loaded xpcom (catch-22), so
> it can't have access to the C-style API!  my solution is to use
> PR_FindSymbolAndLibrary to locate NS_GetFrozenFunctions and the current
> instance of the xpcom library.	this seems to work great under Linux and
> Windows and should work everywhere else too.

  Just FYI, this will not work on Windows 2k/XP if somebody installed Mozilla
with default locale set to 'Japanese' and later changed the default locale to
'Russian'. See bug 162361. I'm not saying something has to be done at the
moment, but we have to remember that PR_Find..Library doesn't work as well as it
could on Win 2k/XP (another trouble with 'two-tier' APIs on Win32). 

Comment 32

[Wolfgang Rosenauer [:wolfiR]]

This seems to break builds with SVG libart renderer?
nsSVGRendererLibart.cpp: In function `nsresult
NS_NewSVGRendererLibart(nsISVGRenderer**)':
nsSVGRendererLibart.cpp:123: error: no matching function for call to
`nsXPIDLString::nsXPIDLString(const nsString&)'

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Checked in this fix for the libart bustage

Comment 34

[Darin Fisher]

(In reply to comment #31)
>   Just FYI, this will not work on Windows 2k/XP if somebody installed Mozilla
> with default locale set to 'Japanese' and later changed the default locale to
> 'Russian'. See bug 162361. I'm not saying something has to be done at the
> moment, but we have to remember that PR_Find..Library doesn't work as well as it
> could on Win 2k/XP (another trouble with 'two-tier' APIs on Win32). 

jshin: can you please elaborate on this issue.  i'm not sure i understand.  my
only requirement is that PR_FindSymbolAndLibrary be able to find
NS_GetFrozenFunctions from the already loaded instance of xpcom.dll.  I'm not
sure why locale changes from one installation to another would be an issue.

Comment 35

[Darin Fisher]

Attachment: rough canvas of tinderbox tests (shortly after landing)

i've probably quoted too much precision in these numbers, and it's hard to know
how stable these results will be... but, overall these look good.  there is
only one glarring issue, and that is the fact that the number of overall heap
allocations increased greatly (the brad "A" metric).  bug 232927 has some ideas
about what might be going on here.  (i need to spend some time comparing
trace-malloc logs...)

Comment 36

[cls]

I'm not sure why but my native mingw win32 builds are failing now with this error:

In file included from c:/root/mozilla/xpcom/obsolete/nsIFileStream.cpp:45:
c:/root/mozilla/xpcom/io/nsSegmentedBuffer.h:76: error: function `PRUint32 
   nsSegmentedBuffer::GetSegmentCount()' definition is marked dllimport.
c:/root/mozilla/xpcom/io/nsSegmentedBuffer.h:83: error: function `PRUint32 
   nsSegmentedBuffer::GetSegmentSize()' definition is marked dllimport.
c:/root/mozilla/xpcom/io/nsSegmentedBuffer.h:87: error: function `char* 
   nsSegmentedBuffer::GetSegment(unsigned int)' definition is marked dllimport.
c:/root/mozilla/xpcom/obsolete/nsIFileStream.cpp: In member function `
   FileImpl::~FileImpl()':
c:/root/mozilla/xpcom/obsolete/nsIFileStream.cpp:184: warning: unused variable 
   `nsresult rv'

My cross-compiled builds still work fine though with the same version of the
compiler & binutils.  Go figure.  Using mingw 3.3.1 20030804-1 & binutils
2.14.90 20030807-1 .

If I revert my tree to before these changes landed (say 6pm), then I don't hit
this problem.

Comment 37

[Jungshik Shin]

(In reply to comment #34)

Sorry I thought the reference to bug 162361 would suffice.

> jshin: can you please elaborate on this issue.  i'm not sure i understand.  my
> only requirement is that PR_FindSymbolAndLibrary be able to find
> NS_GetFrozenFunctions from the already loaded instance of xpcom.dll.  I'm not
> sure why locale changes from one installation to another would be an issue.

Suppose somebody installed Mozilla in a directory with a Japanese name on a
Japanese Win2k/XP with the default system locale set to Japanese. xpcom.dll
would be in C:\JAPANESE_NAME\mozilla\....\xpcom.dll. Once she reboots her
computer with the default system locale switched to Russian, xpcom.dll can't be
found any more because PR_FindSymbolAndLibrary uses Win32 'A' APIs (as opposed
to 'W' APIs) and cannot access the directory with a Japanese name. The same is
true of all PR_* file open-related APIs. I wrote a few times to wtc (and once to
leaf) about PR_Open*, but never heard back.
 

Comment 38

[Scott MacGregor]

Hi Darin,
mailnews seems really crashy today. We crash a lot in

nsCharTraits<char>::length(char const*)

We weren't crashing in builds from the 18th. I'm wondering if it could be
related to the strings changes that went in.

See Bug #234908 for more details.

Comment 39

[Jungshik Shin]

Xft builds crash at MathML pages because mysteriously encoding.get() returns
null in the following code although GetEncoding() fails unless |encoding| is set
to non-null. I wrote up a bug report with details, but apparently I quitted
Mozilla before pressing the 'submit' button. 

http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsFontMetricsXft.cpp#2774

....     nsXPIDLCString encoding;
..... 
2774     if (NS_SUCCEEDED(GetEncoding(family, getter_Copies(encoding), 
2775                      fontType, ftEncoding)) &&
2776         NS_SUCCEEDED(GetConverter(encoding.get(),
getter_AddRefs(converter)))) {

Comment 40

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

It looks like you filed bug 234911.

Comment 41

[cls]

Attachment: Fix mingw bustage

I think the mingw bustage is caused by a bug in gcc.  It looks as though it's
not dropping the dllimport attribute when it inlines the function.  This causes
gcc to complain about attemtping to dllimport code that it defines locally.

Comment 42

[cls]

Comment on attachment 141769 [details] [diff] [review]
Fix mingw bustage

Forgot to mention that this fix is the one Darin & I discussed on irc.	It
removes the NS_COM declarations on the nsSegmentedBuffer functions which are
inlined in nsSegmentedBuffer.h .  This patch adds those 3 functions to the list
of functions that we expect to be inlined by their callers so that they can be
used outside of their class w/o being explicitly (dll)exported.

Comment 43

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

Comment on attachment 141769 [details] [diff] [review]
Fix mingw bustage

What's the error this is fixing?  This seems like it could be a problem for,
e.g., a DEBUG build where all the inline methods are emitted along with the
first non-inline method and then code assumes it can link with those methods.

Comment 44

[Darin Fisher]

actually, .... there's no reason to export any of the nsSegmentedBuffer methods.
 they are no longer used externally.  iirc, they might have been used by necko's
memory cache a long long long long time ago.

dbaron: apparently those inline exports screw up something in the MingW build.

but more to the point, we don't export a lot of inline methods.  we assume that
xpcom and the consumers will be compiled using the same compiler options.  the
only time different compilers matter is when you consider embedders and external
component developers.  but in those cases they are constrained to a fixed API
that does not include any "inline" XPCOM functions.  ... oh wait...
nsStringAPI.h does define some inline function.  hmm... perhaps the windows
compiler will be smart enough to know that it can't expect those symbols to
exist in the xpcom library because they are not decorated with __declspec :-/

Comment 45

[Darin Fisher]

(In reply to comment #44)
> actually, .... there's no reason to export any of the nsSegmentedBuffer methods.

ok, i'm lame.  obviously, xpcom/obsolete needs these methods.  duh!

Comment 46

[Prognathous]

I'm not sure that this bug is related to cases of extreme slowness in editing
textareas with large amounts of text (e.g. wikis), but if it is, then Bug 188294
includes a testcase which shows no performance improvement in recent builds
(note that although that bug is marked for Mac, it's also quite visible on other
platforms).

Prog.

Comment 47

[cls]

Comment on attachment 141769 [details] [diff] [review]
Fix mingw bustage

I'm moving the discussion for the mingw gcc changes over to bug 226609 which
has a new patch.

Comment 48

[Darin Fisher]

Attachment: rough canvas of tinderbox tests (shortly after brad:A regression fix)

Comment 49

[Darin Fisher]

marking FIXED

Comment 50

[timeless]

*** Bug 211252 has been marked as a duplicate of this bug. ***