Closed
Bug 204114
Opened 21 years ago
Closed 18 years ago
nsUnicharUtils need to be threadsafe
Categories
(Core :: Internationalization, defect, P2)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla1.5alpha
People
(Reporter: sfraser_bugs, Assigned: bent.mozilla)
References
Details
Attachments
(3 files, 4 obsolete files)
7.26 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
16.57 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
32.72 KB,
patch
|
Details | Diff | Splinter Review |
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•21 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•21 years ago
|
||
I might get to this, but I'd love some help
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
Attachment #172341 -
Flags: superreview?(alecf)
Attachment #172341 -
Flags: review?(smontagu)
Comment 4•20 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 6•20 years ago
|
||
sorry yes you're right that gInit went away - thanks.
Comment 7•19 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+
Assignee | ||
Comment 8•18 years ago
|
||
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
Assignee | ||
Comment 9•18 years ago
|
||
*** Bug 258382 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 10•18 years ago
|
||
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 11•18 years ago
|
||
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+
Assignee | ||
Comment 12•18 years ago
|
||
Checked in on trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 13•18 years ago
|
||
For the 1.8 branch also?
Attachment #226496 -
Flags: approval1.8.1?
Assignee | ||
Comment 14•18 years ago
|
||
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?
Assignee | ||
Updated•18 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•18 years ago
|
Attachment #226385 -
Attachment description: Patch v1.1 → Patch v1.1 [checked in on trunk]
Assignee | ||
Comment 15•18 years ago
|
||
Here's a potential fix... bsmedberg, what do you think?
Assignee: timeless → bent.mozilla
Status: REOPENED → ASSIGNED
Attachment #226504 -
Flags: review?(benjamin)
Assignee | ||
Comment 16•18 years ago
|
||
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...
Assignee | ||
Updated•18 years ago
|
Attachment #226504 -
Flags: review?(darin)
Comment 18•18 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 19•18 years ago
|
||
RCU locking: http://en.wikipedia.org/wiki/Read-copy-update
Comment 20•18 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?
Assignee | ||
Comment 21•18 years ago
|
||
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.
Assignee | ||
Comment 22•18 years ago
|
||
Filed bug 342898 to get an atomic pointer swap method in NSPR.
Assignee | ||
Comment 23•18 years ago
|
||
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•18 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+
Assignee | ||
Comment 25•18 years ago
|
||
Good call on the signature change. I like this much better.
Attachment #227321 -
Attachment is obsolete: true
Attachment #230800 -
Flags: review?(benjamin)
Assignee | ||
Comment 26•18 years ago
|
||
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•18 years ago
|
Attachment #230800 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 27•18 years ago
|
||
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)
Assignee | ||
Comment 28•18 years ago
|
||
Comment on attachment 230800 [details] [diff] [review] Patch v3.1 Fixed on trunk
Assignee | ||
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago → 18 years ago
Keywords: helpwanted
Resolution: --- → FIXED
Comment 29•18 years ago
|
||
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.
Assignee | ||
Comment 32•18 years ago
|
||
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 :)
Assignee | ||
Comment 33•18 years ago
|
||
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.
Assignee | ||
Comment 35•18 years ago
|
||
(In reply to comment #34) > If you look here: Ah, true, btek definitely jumped :-/ New bug?
Comment 36•18 years ago
|
||
Probably better that way, yes.
Assignee | ||
Comment 37•18 years ago
|
||
Filed bug 354787 for the Tp/Tdhtml regression.
You need to log in
before you can comment on or make changes to this bug.
Description
•