Closed Bug 12755 Opened 23 years ago Closed 13 years ago
Atom table needs to be thread safe
There is no way to compare nsIAtoms without "breaking" them open. Currently nsIAtoms are being compared by there pointers which is totally wrong.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → INVALID
That's the whole point of atoms -- that their compare routine is pointer equality (for performance reasons). Logically, they're the same as strings.
Pointer equality does not always mean that they are equal. jband or I or other xptcall users, can "wrap" a object. For instance, cookie proxies nsIAtoms as parameters. when they are compared in: nsHTTPHeaderArray::GetEntry(nsIAtom* aHeader, nsHeaderEntry** aResult) the in aHeader may be 'wrapped' and have a different pointer. Maybe a solution would be to have an == operand in the wrapped classes that would "expose" the real pointer. (evil, evil, evil... any thought jband)
dougt has a point. Pointer equality just isn't enough in xpcom as soon as xpconnect and proxy are in the mix - and they are. An equals method makes some sense to me. Still, since either or both of the objects being compared could be proxied, implementing equals would require first doing a pointer compare and then falling back on a strcmp against the other object's GetUnicode(). No? Are we claiming that there will always be one and only one supplier of nsIAtoms? Comparing to nsIAtoms created in some other space is not required? Perhaps a hashCode method would do the trick? This would have to be of fixed size - presumably 32 bits - so just mapping pointer into hashCode might not make 64bit systems happy. For must uses you could continue to use pointer equality, but the new equals method could compare hashCodes for a good answer. As long as we have only one supplier of nsIAtoms then the hashCode can be a pointer and an equals method based on it would be perfect (modulo the 32bits/ptr issue).
Clearing Invalid resolution due to reopen.
Putting on [Perf] radar.
The whole reason for the existence of atoms is to make the string equality be a pointer comparison. If you eliminate that fact, you might as well eliminate atoms altogether. I don't think we want to do that, because atoms provide a legitimate benefit for the parts of the system that don't involve these special curcumstances. I think js and the proxy stuff needs to understand when it's dealing with atoms (or objects standing in for atoms) and deal with them specially. Cc'ing Kipp for his wisdom on the subject (I think he wrote the atom stuff).
The *entire* reason for atom's is so that you can write "a == b" and know that its a pointer comparison. Just shoot layout in the head if you need to change this. It's a *HUGE* big deal and changing it to be a method code will probably increase the code size by some interesting percentage AND slow the entire thing down by another interesting percentage. I don't have an answer for your proxy problems, but changing nsIAtom's behavior isn't one of them.
jband, maybe a solution it to add an attribute to the typelib which basically says "do not proxy this as a parameter". It can be used for all nsISupports that have built in thread safety or are special exceptions.
I wasn't suggesting that '==' stop working or that existing users of the class need to change their usage. You guys are getting all bothered and arguing against change in nsIAtom. I thought we were talking about extending it not undermining it. I don't think it makes sense for proxy to not wrap these things as they are now. This throws thread safety out the window. AtomImpl has no mutators, but its ctor and dtor manipulate a global value and bad things could happen if you were to create or do the final release of one of these on some other thread. If the impl were made threadsafe then this looks better. So, doug's statement about "have built in thread safety or are special exceptions" becomes "have built in thread safety *and* are special exceptions". Are you guys willing to incur locking overhead when creating and destroying atoms? Without some reasonable protocol for comparison nsIAtoms are a lot less useful to JS too. Not proxying them is not an option. :) These nsIAtoms are pretty whacky as xpcom objects go. There is this implicit assumption that only one place in the code can produce objects that correctly conform to the protocol of the interface. It seems to me that they really don't scale very well out into the component world. FWIW my recomendations are: 1) add HashCode that returns the address of the object cast as a PRUint32. 2) add Equals that just compares 'this' to other->HashCode() on 32bit systems and is #ifdef'd to do a string compare on systems where 32bits is not enough for an address. 3) continue to use '==' in existing code. 4) Use Equals in proxied and JS code. This adds no bloat to the atoms or to their normal usage. We can also look at adding a 'threadsafe' (or whatever) flag. But this is a statement about a particular implementation of an interface, not about the interface itself. I think that this should describe the component CLSID not the interface IID. I don't think this belongs in typelibs.
I don't have any problem extending nsIAtom, provide == still works. I would suggest defining some sort of nsIObject interface that has HashCode and Equals -- there's lots of places that could be using that, and it would be good to standardize this. It would also be handy for building more generic sorts of lists and hash tables.
continuing to use '==' in existing code may break if proxied nsIAtoms propagate into other places. The first place that I had to fix was the HTTP protocol. I am not sure what the code paths between layout and necko are, but if more proxied object find their way into layout, more equality operands will break. I would append jband's recommendation to state: 4. Use Equals all new code.
> 4. Use Equals all new code. This doesn't solve anything, since as Kipp pointed out, we can't change layout. I think the proxy code needs to resolve proxied atoms to real atoms so that equality continues to work. (Although I'm not sure why we'd ever have a proxied atom -- I though atoms would only appear as arguments to methods invoked on another thread, not as proxied/wrapped objects.)
The HTTP protocol is using xpcom/proxy to notify COOKIE. It passes an nsIAtom to the notifiee, this nsIAtom is wrapped to ensure that it is only called back on the notifier's thread. In the STA model of client apps, parameters are proxied in to and out of any proxied call to maintain strict thread safety. Inside DCom, pp 152: "The client, however, must follow COM's threading rules and always marshall interface pointers between apartments". This 'however' that was in the text talks about a "free-thread marshaler". I have not implmented this yet, and was not planing on it. we could implement something similar to this which will allow us to pass the interface pointer directly between appartments. This would require that a new attribute in the idl. When this attribute is added to an IDL, it must mean that ALL implementation will handle their own thread safety. (if fact, IMAP was asking me about something similar to this last week).
What (jband) has been suggested sounds reasonable; however, it's now a bug that the atom table is not thread safe now that we are getting more threaded...Another bug report perhaps?
regarding thread safety: http://bugzilla.mozilla.org/show_bug.cgi?id=12914 Regarding a new IDL attribute (I assigned to you jband.) http://bugzilla.mozilla.org/show_bug.cgi?id=12915 If this is acceptable with everyone, we should close this bug.
Summary: nsIAtom's do not have a compare routine → nsIAtom's need to be marked as free-threaded in coclass.
once co-classes are a real thing, nsIAtoms need to be defined somehow as free-threaded. I am changing the Summary.
Jumping in late, after talking to jband last night (and a few weeks ago): Pointer equality in a given apartment should work whether atoms are threadsafe or proxied, provided you uniquely associate atoms to their strings in each apartment. If you find you have an atom pointer for "foo" and a proxy for that atom in your hands, you've probably found a thread safety bug (i'm assuming atoms are not threadsafe or "free-threaded" yet): one of the pointers, probably the one that's not a proxy, could be in use by another thread. I agree we should make atoms thread-safe and get on with our lives, but I wanted to point out what may be a hole in our proxy system: it should enable the less efficient, but equivalent under ==, alternative: give each piece of code that can access an atom for "foo" the same atom, every time; never let another apartment's proxy get into the wrong apartment. BTW, jband sez "coclass" is a misleading MS-ism; he and I think "component declaration" is better, but it's a mouthful. Does CDL help? That'll be the language acronym. /be
What is the bug for `need CoClasses'? This bug obviously depends on that one.
Bulk add of "perf" to new keyword field. This will replace the [PERF] we were using in the Status Summary field.
This has nothing to do with preformance. removing fields.
I understand brendans comment. Does that mean that this is two bugs? (1) getting a proxy to the same object in the same apartment must always return the same proxy. (2) the atom table does not provide thread safe access As a side note: the new |nsShared[C]String| class has atom-like properties. We might consider using them in the atom table. They might facilitate other clients having `local' atom collections of their own. There is also the question that has been raised in another bug about the efficacy of a single atom table. This may indicate that a (probably cached) hash in addition to pointer equality is reasonable.
Target Milestone: M16 → M17
Summary: nsIAtom's need to be marked as free-threaded in coclass. → Atom table needs to be thread safe.
scc, exactly. two bugs.
mass re-assigning to my new bugzilla account
Assignee: scc → scc
Status: ASSIGNED → NEW
dp is no longer @netscape.com. changing qa contact to default for this product
QA Contact: dp → kandrot
While tracking down what turned out to be another instance of bug 55143, I made the atom table threadsafe. Patch above.
*** Bug 12914 has been marked as a duplicate of this bug. ***
re-assigning to XPCOM owner
Assignee: scc → dougt
Status: ASSIGNED → NEW
QA Contact: kandrot → scc
over to alec.
Assignee: dougt → alecf
what IS this bug at this point? I don't event know. Is this about pointer equality or having an Equals() method, or making the table threadsafe? And how should it be threadsafe, do we wrap lookups/creations with locks? just creation? help! someone better explain this to me before I mark this WONTFIX :)
Things that manipulate the atom table should in theory be threadsafe, since we could have problems if the table is used from multiple threads.
only drivers can set blocking flags
if you don't mind, i'd like to see the table made threadsafe, and eventually an additional method added to nsIAtom. Given that this bug hasn't morphed in about 4 years, (and only morphed during the first 6months of its life), I'd say that this bug should focus on the current summary (Atom table needs to be thread safe), I can file a new bug about adding a |boolean equalsAtom(in nsIAtom aAtom);|
why do you need equalsAtom()? for scripting? I believe xpconnect has it set up so that if you do atom === atom it will compare the native objects.
yeah, i'm scripting on threads. we haven't yet run into atom problems, but darin warned me about them on friday when we talked about steps to make pieces of necko threadsafe.
Cc'ing people who can confirm alecf's belief in comment 34: "I believe xpconnect has it set up so that if you do atom === atom it will compare the native objects." /be
I don't believe there is any such logic in XPConnect for ===. Not trusting my memory, I ran a simple test in XPCShell and only the JSOP_NEW_EQ is used and the macro it uses appears to only treat strings and doubles as special cases everything else compares the jsval's. However, I think the atom service is returning the same pointer value for a given atom so it's probably essentially true, since XPConnect would lookup the native pointer via GetNewOrUsed and return the previous JS object associated with the native. So if the atom service is always returning the same native pointer for a given string, is there really a need for equalsAtom? Can you create an atom outside of this service that might have a different native pointer value? One other question that comes to mind is double wrapping. It's possible that in a double wrapping situation that the === might return false, but I'd have to give that some more thought to know if this could really occur.
GetNewOrUsed, eww, you mean that very unthreadsafe thing which has crashed me perhaps 50 times in 2 weeks.
Timeless, what is your point? If you're trying to justify adding an equalsAtom method by pointing to a thread-safety bug, you're way off base. Fix the damn thread safety bug, or help get it fixed. Don't pile bloat on top to paper over the thread safety bug. /be
Assignee: alecf → nobody
QA Contact: scc → xpcom
OS: Windows NT → All
Priority: P3 → --
Target Milestone: Future → ---
Status: NEW → RESOLVED
Closed: 23 years ago → 13 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.