nsUnicharUtils need to be threadsafe

RESOLVED FIXED in mozilla1.5alpha

Status

()

Core
Internationalization
P2
normal
RESOLVED FIXED
15 years ago
12 years ago

People

(Reporter: Simon Fraser, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

Trunk
mozilla1.5alpha
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

15 years ago
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

15 years ago
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

15 years ago
I might get to this, but I'd love some help
Keywords: helpwanted
Priority: -- → P2
Target Milestone: --- → mozilla1.5alpha

Comment 3

13 years ago
Created attachment 172341 [details] [diff] [review]
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.
Assignee: alecf → timeless
Status: NEW → ASSIGNED

Updated

13 years ago
Attachment #172341 - Flags: superreview?(alecf)
Attachment #172341 - Flags: review?(smontagu)

Comment 4

13 years ago
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

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

Comment 6

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

Comment 7

13 years ago
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
Attachment #172341 - Flags: superreview?(alecf) → superreview+
It looks like this was forgotten... Are there still plans to incorporate this? Or has something changed elsewhere?
OS: MacOS X → All
Hardware: Macintosh → All
*** Bug 258382 has been marked as a duplicate of this bug. ***
Created attachment 226385 [details] [diff] [review]
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?
Attachment #172341 - Attachment is obsolete: true
Attachment #226385 - Flags: review?(smontagu)
Attachment #172341 - Flags: review?(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.
Attachment #226385 - Flags: review?(smontagu) → review+
Checked in on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Created attachment 226496 [details] [diff] [review]
Patch v1.1 (for the 1.8 branch)

For the 1.8 branch also?
Attachment #226496 - Flags: approval1.8.1?
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...
Attachment #226496 - Attachment is obsolete: true
Attachment #226496 - Flags: approval1.8.1?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #226385 - Attachment description: Patch v1.1 → Patch v1.1 [checked in on trunk]
Created attachment 226504 [details] [diff] [review]
Patch v2.0

Here's a potential fix... bsmedberg, what do you think?
Assignee: timeless → bent.mozilla
Status: REOPENED → ASSIGNED
Attachment #226504 - Flags: review?(benjamin)
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...
Attachment #226504 - Flags: review?(darin)
We're also leaking now. See bug 342425.
Blocks: 342425

Comment 18

12 years ago
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.
Attachment #226504 - Flags: review?(darin)
Attachment #226504 - Flags: review?(benjamin)
Attachment #226504 - Flags: review-

Comment 20

12 years ago
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?
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.
Filed bug 342898 to get an atomic pointer swap method in NSPR.
Created attachment 227321 [details] [diff] [review]
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.
Attachment #226504 - Attachment is obsolete: true
Attachment #227321 - Flags: review?(benjamin)

Comment 24

12 years ago
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.
Attachment #227321 - Flags: review?(benjamin) → review+
Created attachment 230800 [details] [diff] [review]
Patch v3.1

Good call on the signature change. I like this much better.
Attachment #227321 - Attachment is obsolete: true
Attachment #230800 - Flags: review?(benjamin)
Created attachment 232190 [details] [diff] [review]
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.
Attachment #232190 - Flags: review?(benjamin)

Updated

12 years ago
Attachment #230800 - Flags: review?(benjamin) → review+
Comment on attachment 232190 [details] [diff] [review]
Complete 1.8-branch patch

Clearing review on this one - it missed 1.8.
Attachment #232190 - Flags: review?(benjamin)
Comment on attachment 230800 [details] [diff] [review]
Patch v3.1

Fixed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Keywords: helpwanted
Resolution: --- → FIXED
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)?
This should be backed out if we don't find a solution to the Tp hit.
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.
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 :)
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 :)
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.
(In reply to comment #34)
> If you look here:

Ah, true, btek definitely jumped :-/

New bug?
Probably better that way, yes.
Filed bug 354787 for the Tp/Tdhtml regression.
You need to log in before you can comment on or make changes to this bug.