Last Comment Bug 12755 - Atom table needs to be thread safe.
: Atom table needs to be thread safe.
Status: RESOLVED INCOMPLETE
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- major (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 12914 (view as bug list)
Depends on: 12702 12914
Blocks:
  Show dependency treegraph
 
Reported: 1999-08-28 23:55 PDT by Doug Turner (:dougt)
Modified: 2009-07-24 11:37 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, diff -wu (2.97 KB, patch)
2001-03-22 18:02 PST, Chris Waterson
no flags Details | Diff | Splinter Review

Description Doug Turner (:dougt) 1999-08-28 23:55:35 PDT
There is no way to compare nsIAtoms without "breaking" them open.  Currently
nsIAtoms are being compared by there pointers which is totally wrong.
Comment 1 Warren Harris 1999-08-30 16:01:59 PDT
That's the whole point of atoms -- that their compare routine is pointer
equality (for performance reasons). Logically, they're the same as strings.
Comment 2 Doug Turner (:dougt) 1999-08-30 18:26:59 PDT
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)
Comment 3 John Bandhauer 1999-08-30 18:53:59 PDT
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).
Comment 4 leger 1999-08-30 20:30:59 PDT
Clearing Invalid resolution due to reopen.
Comment 5 leger 1999-08-30 22:27:59 PDT
Putting on [Perf] radar.
Comment 6 Warren Harris 1999-08-31 00:52:59 PDT
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).
Comment 7 kipp 1999-08-31 09:31:59 PDT
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.
Comment 8 Doug Turner (:dougt) 1999-08-31 10:35:59 PDT
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.
Comment 9 John Bandhauer 1999-08-31 12:05:59 PDT
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.
Comment 10 Warren Harris 1999-08-31 12:29:59 PDT
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.
Comment 11 Doug Turner (:dougt) 1999-08-31 12:32:59 PDT
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.
Comment 12 Warren Harris 1999-08-31 13:09:59 PDT
> 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.)
Comment 13 Doug Turner (:dougt) 1999-08-31 13:31:59 PDT
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).
Comment 14 kipp 1999-08-31 17:34:59 PDT
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?
Comment 15 Doug Turner (:dougt) 1999-08-31 17:57:59 PDT
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.
Comment 16 Doug Turner (:dougt) 1999-09-13 12:40:59 PDT
once co-classes are a real thing, nsIAtoms need to be defined somehow as
free-threaded.  I am changing the Summary.
Comment 17 brendan-obsolete 1999-09-23 11:14:59 PDT
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
Comment 18 Scott Collins 1999-10-21 11:57:59 PDT
What is the bug for `need CoClasses'?  This bug obviously depends on that one.
Comment 19 leger 2000-01-07 08:21:59 PST
Bulk add of "perf" to new keyword field.  This will replace the [PERF] we were
using in the Status Summary field.
Comment 20 Doug Turner (:dougt) 2000-01-07 09:02:59 PST
This has nothing to do with preformance.  removing fields.
Comment 21 Scott Collins 2000-03-30 10:48:41 PST
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.
Comment 22 Doug Turner (:dougt) 2000-03-30 11:10:08 PST
scc, exactly.  two bugs.
Comment 23 Scott Collins 2000-05-15 01:37:33 PDT
mass re-assigning to my new bugzilla account
Comment 24 Dawn Endico 2000-12-21 14:09:54 PST
dp is no longer @netscape.com. changing qa contact to default for this product
Comment 25 Chris Waterson 2001-03-22 18:02:12 PST
Created attachment 28551 [details] [diff] [review]
patch, diff -wu
Comment 26 Chris Waterson 2001-03-22 18:03:37 PST
While tracking down what turned out to be another instance of bug 55143, I made
the atom table threadsafe. Patch above.
Comment 27 Edward Kandrot 2001-05-14 18:27:40 PDT
*** Bug 12914 has been marked as a duplicate of this bug. ***
Comment 28 Scott Collins 2002-11-03 13:11:26 PST
re-assigning to XPCOM owner
Comment 29 Doug Turner (:dougt) 2003-04-08 09:31:56 PDT
over to alec.
Comment 30 Alec Flett 2003-04-08 12:20:26 PDT
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 :)
Comment 31 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2003-04-08 12:24:42 PDT
Things that manipulate the atom table should in theory be threadsafe, since we
could have problems if the table is used from multiple threads.
Comment 32 Andrew Schultz 2004-03-02 16:09:35 PST
only drivers can set blocking flags
Comment 33 timeless 2004-03-02 21:12:53 PST
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);|
Comment 34 Alec Flett 2004-03-02 21:50:43 PST
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.
Comment 35 timeless 2004-03-02 22:08:24 PST
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.
Comment 36 Brendan Eich [:brendan] 2004-03-02 22:22:31 PST
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
Comment 37 David Bradley 2004-03-03 17:18:34 PST
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.
Comment 38 Alec Flett 2004-03-03 17:59:12 PST
as long as nobody is boneheaded enough to try to implement nsIAtom in
JavaScript, then the native pointers will always, always be equal. By "compare
native objects" I meant compare the native pointers. 

The same issue exists in C++ anyway - I could go and try to implement nsIAtom,
and then pass myself around.. if I was being dumb and wanted to confuse my callers.

if === means to compare native pointers, then we're fine and this who issue of
atom scriptability is not really an issue at all.
Comment 39 timeless 2004-03-04 08:56:11 PST
GetNewOrUsed, eww, you mean that very unthreadsafe thing which has crashed me
perhaps 50 times in 2 weeks.
Comment 40 Brendan Eich [:brendan] 2004-03-04 12:46:43 PST
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

Note You need to log in before you can comment on or make changes to this bug.