Atom table needs to be thread safe.

RESOLVED INCOMPLETE

Status

()

Core
XPCOM
--
major
RESOLVED INCOMPLETE
18 years ago
5 months ago

People

(Reporter: dougt, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
There is no way to compare nsIAtoms without "breaking" them open.  Currently
nsIAtoms are being compared by there pointers which is totally wrong.

Updated

18 years ago
Assignee: dp → scc

Updated

18 years ago
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → INVALID

Comment 1

18 years ago
That's the whole point of atoms -- that their compare routine is pointer
equality (for performance reasons). Logically, they're the same as strings.
(Reporter)

Updated

18 years ago
Status: RESOLVED → REOPENED
(Reporter)

Comment 2

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

18 years ago
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).
(Reporter)

Updated

18 years ago
Depends on: 12702

Updated

18 years ago
Resolution: INVALID → ---

Comment 4

18 years ago
Clearing Invalid resolution due to reopen.

Updated

18 years ago
Whiteboard: [Perf]

Comment 5

18 years ago
Putting on [Perf] radar.

Comment 6

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

18 years ago
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.
(Reporter)

Comment 8

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

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

18 years ago
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.
(Reporter)

Comment 11

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

18 years ago
> 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.)
(Reporter)

Comment 13

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

18 years ago
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?
(Reporter)

Comment 15

18 years ago
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.
(Reporter)

Updated

18 years ago
Summary: nsIAtom's do not have a compare routine → nsIAtom's need to be marked as free-threaded in coclass.
(Reporter)

Comment 16

18 years ago
once co-classes are a real thing, nsIAtoms need to be defined somehow as
free-threaded.  I am changing the Summary.

Updated

18 years ago
Status: REOPENED → ASSIGNED

Comment 17

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

Updated

18 years ago
Depends on: 12914

Updated

18 years ago
QA Contact: beppe → dp

Updated

18 years ago
Target Milestone: M16

Comment 18

18 years ago
What is the bug for `need CoClasses'?  This bug obviously depends on that one.

Updated

18 years ago
Keywords: perf

Comment 19

18 years ago
Bulk add of "perf" to new keyword field.  This will replace the [PERF] we were
using in the Status Summary field.
(Reporter)

Updated

18 years ago
Keywords: perf
Whiteboard: [Perf]
(Reporter)

Comment 20

18 years ago
This has nothing to do with preformance.  removing fields.

Comment 21

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

Updated

18 years ago
Summary: nsIAtom's need to be marked as free-threaded in coclass. → Atom table needs to be thread safe.
(Reporter)

Comment 22

18 years ago
scc, exactly.  two bugs.

Updated

17 years ago
Target Milestone: M17 → Future

Comment 23

17 years ago
mass re-assigning to my new bugzilla account
Assignee: scc → scc
Status: ASSIGNED → NEW

Updated

17 years ago
Status: NEW → ASSIGNED

Comment 24

17 years ago
dp is no longer @netscape.com. changing qa contact to default for this product
QA Contact: dp → kandrot

Comment 25

17 years ago
Created attachment 28551 [details] [diff] [review]
patch, diff -wu

Comment 26

17 years ago
While tracking down what turned out to be another instance of bug 55143, I made
the atom table threadsafe. Patch above.
Keywords: patch

Comment 27

16 years ago
*** Bug 12914 has been marked as a duplicate of this bug. ***

Comment 28

15 years ago
re-assigning to XPCOM owner
Assignee: scc → dougt
Status: ASSIGNED → NEW
QA Contact: kandrot → scc
(Reporter)

Comment 29

14 years ago
over to alec.
Assignee: dougt → alecf

Comment 30

14 years ago
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.

Updated

14 years ago
Flags: blocking1.4.2+

Comment 32

14 years ago
only drivers can set blocking flags
Flags: blocking1.4.2+

Comment 33

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

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

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

Comment 37

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

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

14 years ago
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
Last Resolved: 18 years ago8 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.