204114 - nsUnicharUtils need to be threadsafe

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P2)

Description

[Simon Fraser [no longer active]]

There are situations, actually seen in some embedding apps, where the functions
in nsUnicharUtils can be called from different threads, and maybe called the
first time on a non-UI thread. The code needs to be made threadsafe.

Comment 1

[Alec Flett]

hmm.. As I recall, 99% of this is threadsafe, I think the only part that is not
is the retrieval of the service. 

Comment 2

[Alec Flett]

I might get to this, but I'd love some help

Comment 3

[timeless]

Attachment: mark unicharutils threadsafe along with the other intl conspirators

one of these others has patches in a couple of bugs which are better than the
changes here. it's not essential that my changes which overlap those other bugs
be committed, although not committing something means xpinstall asserts.

Comment 4

[Alec Flett]

I'm kind of wondering why we need to use gInit to create a single instance of
gUpperMap and gLowerMap? Why aren't consumers just retrieving
nsCaseConversionImp2 as a service instead?

In fact, since there is no COM stuff in the creation of nsCompressedMap, I kind
of wonder if it might finally be legal to create a static instantiation of that
class.. I mean in 2005 I would hope we might finally support static, non-COM
classes. Might want to ask someone (dbaron? darin?) about this.

i.e. hopefully we can just declare

static nsCompressedMap gUpperMap(NS_REINTERPRET_CAST(const PRUnichar*,
&gToLower[0]), gToLowerItems);

Comment 5

[timeless]

alecf: isn't that the conclusion expressed by my patch?

Comment 6

[Alec Flett]

sorry yes you're right that gInit went away - thanks.

Comment 7

[Alec Flett]

Comment on attachment 172341 [details] [diff] [review]
mark unicharutils threadsafe along with the other intl conspirators

ok, sorry for the long delay, sr=alecf on this but I'd still like to see
gUpper/gLowerMap become non-heap-based as I describe above

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

It looks like this was forgotten... Are there still plans to incorporate this? Or has something changed elsewhere?

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v1.1 [checked in on trunk]

Here's a recent patch that incorporates the previous as well as alecf's comments about static maps. smontagu, are you the right one for this review?

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 226385 [details] [diff] [review]
Patch v1.1 [checked in on trunk]

Please remove gInit from nsCaseConversionImp2.h also. With that, r=smontagu.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Checked in on trunk.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v1.1 (for the 1.8 branch)

For the 1.8 branch also?

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 226496 [details] [diff] [review]
Patch v1.1 (for the 1.8 branch)

Actually, bsmedberg pointed out that we still have a race condition in nsUnicharUtils over getting gCaseConv during Initialization and Shutdown...

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v2.0

Here's a potential fix... bsmedberg, what do you think?

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 226504 [details] [diff] [review]
Patch v2.0

I guess I'm at a loss for what to do if we get to the ShutdownObserver when the status is PENDING...

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We're also leaking now. See bug 342425.

Comment 18

[Benjamin Smedberg]

Comment on attachment 226504 [details] [diff] [review]
Patch v2.0

>Index: intl/unicharutil/util/nsUnicharUtils.cpp

> static nsresult NS_InitCaseConversion() {
>-    if (gCaseConv)
>+    if (gCaseConvStatus == UNICHARUTILS_STATUS_AVAILABLE)
>       return NS_OK;

>-     
>+    else if (gCaseConvStatus == UNICHARUTILS_STATUS_PENDING)
>+      return NS_ERROR_NOT_INITIALIZED;

This destroys the point of having the atomic variable.

If doing a simple getService every time we do a case conversion is too costly (and it probably is), and locking is too costly (which it may be), the next option would be RCU locking. You pay the cost of an AddRef/Release pair per-call, but you don't pay the cost of any locking. However, it requires an atomic-set operation for pointers, which it doesn't look like we have:

static nsICaseConversion* gCaseConversion;

static already_AddRefed<nsICaseConversion> PR_InitCaseConversion()
{
  // assume pointer-size reads are atomic?
  nsICaseConversion *cc = gCaseConversion;

  if (!cc) {
    CallGetService(..., &cc);

    nsICaseConversion *cc2 = PR_AtomicSetPointer(&gCaseConversion, cc);
    NS_IF_RELEASE(cc2);
  }

  NS_ADDREF(cc);
  return cc;
}

Darin should really comment on whether a pointer-size read is really atomic, and whether a PR_AtomicSetPointer function is practical.

Comment 19

[Benjamin Smedberg]

RCU locking: http://en.wikipedia.org/wiki/Read-copy-update

Comment 20

[Darin Fisher]

nsCaseConversionImp2 has no member variables.  This whole problem is completely bogus and caused by our use of XPCOM.  Maybe there is a better way that avoids having to access nsCaseConversionImp2 via nsICaseConversion?

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Chatted some more with darin and he suggested only allowing the main thread to set gCaseConv. All threads can read it. If the main thread reads it and it's null then it will call do_GetService and set gCaseConv. If a non-main thread reads it and it's null then it can use do_GetService. Sound ok? Patch in a little bit.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Filed bug 342898 to get an atomic pointer swap method in NSPR.

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v3.0 [NOT FOR CHECKIN (-w version)]

So I had a slight OCD moment when I decided that no matter how I did my whitespace I was going to disagree with something else in this file... And so I picked one style from the three (more?) others and changed everything. Here's the -w version of the patch so that you can actually read it.

Anyway, this is the third option. It uses the cached service whenever possible and only allows that to be set from the main thread. Other threads must simply use the service manager if the cached version isn't available.

Comment 24

[Benjamin Smedberg]

Comment on attachment 227321 [details] [diff] [review]
Patch v3.0 [NOT FOR CHECKIN (-w version)]

>Index: intl/unicharutil/util/nsUnicharUtils.cpp

>+NS_IMETHODIMP
>+NS_GetCaseConversion(nsICaseConversion** _retval)

Make this "nsresult" instead of "NS_IMETHODIMP", because we're not implementing a virtual method. It seems to me that changing this method signature to

already_AddRefed<nsICaseConversion>
NS_GetCaseConversion();

could reduce indirection a bit. If you go that route, I'd like to see the patch again before checkin.

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v3.1

Good call on the signature change. I like this much better.

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Complete 1.8-branch patch

This is a complete patch for the 1.8 branch (I know, probably not going to make it, but worth a shot?).

It includes attachment 226385 [details] [diff] [review] (Patch v1.1), attachment 230800 [details] [diff] [review] (Patch v3.1), and the fix for bug 342425.

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 232190 [details] [diff] [review]
Complete 1.8-branch patch

Clearing review on this one - it missed 1.8.

Comment 28

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 230800 [details] [diff] [review]
Patch v3.1

Fixed on trunk

Comment 29

[Boris Zbarsky [:bzbarsky]]

So is it me, or did this cause a bit of a Tp/Tdhtml hit (order of 1%)?  Would it make sense to find the critical-path users of this stuff (style system?  Or something else?) and have them cache the service pointer or something (e.g. in nsContentUtils)?

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This should be backed out if we don't find a solution to the Tp hit.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

These functions using nsICaseConversion are in the same module as nsCaseConversionImp2, so why are they using gCaseConv at all? They could call directly into some shared non-virtual functions.

Comment 32

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm happy to look into this further, but can someone point me to a Tp graph that looks bad? I'm having a hard time seeing the performance hit.

Not that I doubt you, but I want to see it too :)

Comment 33

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm happy to look into this further, but can someone point me to a Tp graph that looks bad? I'm having a hard time seeing the performance hit.

Not that I doubt you, but I want to see it too :)

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If you look here:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey&hours=24&maxdate=1159438176&legend=0
almost all the btek Tp numbers are in the low 920s before your checkin. After your checkin they're mostly up in the 930s.

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #34)
> If you look here:

Ah, true, btek definitely jumped :-/

New bug?

Comment 36

[Boris Zbarsky [:bzbarsky]]

Probably better that way, yes.

Comment 37

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Filed bug 354787 for the Tp/Tdhtml regression.