[Bug libobjc/47012] New: nonatimic Properties behave wrong

[Bug libobjc/47012] New: nonatimic Properties behave wrong

[js-gcc at webkeks dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47012

           Summary: nonatimic Properties behave wrong
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libobjc
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: js-gcc@webkeks.org


It seems nonatomic properties retain and autorelease the result, which is
breaking existing code targeting the Apple runtime.

This bug has been also in the GNUstep runtime and the Cocotron runtime, it
seems this bug has been copied to the new GNU runtime now.

The Apple doc says:
"If you specify nonatomic, then a synthesized accessor for an object property
simply returns the value directly."
<http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/ObjectiveC/Articles/ocProperties.html>

I have written a property implementation which is known to be compatible to the
Apple runtime which is quite short and I'd like to contribute. It is currently
licensed under the QPL, but I plan to relicense it as GPL if you are
interested.

If you need a testcase, there is a test for properties included in ObjFW
<https://webkeks.org/hg/objfw>, in src/PropertiesTests.m, which also fails. It
works well with the Apple runtime and the included properties implementation in
src/objc-properties.m - this is also the implementation I'd like to contribute.
Let me know if you are interested.

Direct links:
Runtime I'd like to contribute:
<https://webkeks.org/hg/objfw/file/tip/src/objc_properties.m> (Will relicense
if there's interest!)
Tests that fail:
<https://webkeks.org/hg/objfw/file/tip/tests/PropertiesTests.m>

[Bug libobjc/47012] nonatomic Properties behave wrong

[nicola at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47012

Nicola Pero <nicola at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2010.12.19 18:31:43
                 CC|                            |nicola at gcc dot gnu.org
     Ever Confirmed|0                           |1

--- Comment #1 from Nicola Pero <nicola at gcc dot gnu.org> 2010-12-19 18:31:43 UTC ---
Hi Jonathan

nice to hear from you again (we met at Fosdem a few years ago, not sure you
remember me; I do remember you). ;-)

You are right that the GNU runtime does [[x retain] autorelease] before 
returning the value for an object, nonatomic synthesized property .  The reason 
everyone but Apple does it, I suppose, it is because everyone felt it is a bug 
in the Apple implementation. ;-)

The additional retain/autorelease should make it safer (but slower) than the
Apple implementation; I don't think it should cause it to behave differently
(other than prevent the object from being released until the next autorelease
pool is emptied).  Do you have a testcase where an actual difference in 
behaviour (other than the object being safe to use for longer, and the 
implementation with autorelease/retain being obviously slower) can be seen ?

I guess you're concerned about performance ?

Do you have any evidence that Apple did that on purpose and it's not a bug ?  
If you have any such evidence [ie, they won't "fix" it at the next release and 
we'll suddenly be the "broken" ones ;-)], we should certainly change it to 
behave in the same way.

Maybe we should change it to behave in the same way anyway ;-)

Thanks

PS: Your contribution to GCC's libobjc would be much appreciated.  The existing 
implementation of properties accessors is fairly finished though --

http://gcc.gnu.org/viewcvs/trunk/libobjc/accessors.m?revision=165903&view=markup

It would be a one-liner to make the change if we want to.

[Bug libobjc/47012] nonatomic Properties behave wrong

[js-gcc at webkeks dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47012

--- Comment #2 from js-gcc at webkeks dot org <js-gcc at webkeks dot org> 2010-12-19 18:49:39 UTC ---
Hi Nicola,

yes, I do remember our talk at FOSDEM and I plan to attend it next year again,
since I unfortunately could not attend it this year.

Well, I guess it's not really like this because everyone felt this is better, I
guess it is because Cocotron was the first to implement it (IIRC), then GNUstep
looked how they solved it and now GCC looked how GNUstep solved it ;).

I think it's not a bug, but intended, as not only the Apple runtime handles it
like that, but the doc also says it's "just a return". So the documented
behaviour and the real behaviour are in sync, I guess the name "nonatomic" was
just a bad choice and the real idea was to have a way to create a lightweight
accessor which is mostly used internally. There are several hints that
nonatomic is thought for internal use in the formulations used by Apple. Plus,
if you care that much about performance that the spinlock is too much for you,
then autoreleasing would definitely also be a problem for you ;).

I mostly use nonatomic in some special cases like exceptions where the object
which caused the exception should not be retained and autoreleased to prevent
exception-loops (for example, if autorelease caused the exception to be thrown,
that'd be a problem here).

In the code you linked, is that objc_mutex_t a spinlock? If not, this might be
performance problem.

Speaking of performance and contributing: I wrote my own runtime some time ago.
The lookup is twice as fast as in libobjc in my tests (with ASM enabled even
more). Maybe we could work on merging that into libobjc on next FOSDEM? I
mostly wrote my runtime because there weren't any changes in the GNU runtime
for a long time and I was happy to see that things finally changed with gcc 4.6
:).

If you want to have a look: https://webkeks.org/hg/objfw-rt
(Hint: Thread-safety is still missing, though the data structure for the lookup
only needs a write-lock and no read-lock and can still be read while it is
written and it's also missing exceptions (copying the exception.c from GNU
libobjc works))

I think it would be great to also work with the guys from Clang. I think it
would only make sense to have the same runtime on all non-Apple systems,
regardless of the compiler.

[Bug libobjc/47012] nonatomic Properties behave wrong

[nicola at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47012

--- Comment #3 from Nicola Pero <nicola at gcc dot gnu.org> 2010-12-19 19:10:29 UTC ---
Author: nicola
Date: Sun Dec 19 19:10:26 2010
New Revision: 168070

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=168070
Log:
In libobjc/:
2010-12-19  Nicola Pero  <nicola.pero@meta-innovation.com>

    PR libobjc/47012
    * accessors.m (objc_getProperty): If not atomic, do not
    retain/autorelease the returned value. (Problem reported by

Modified:
    trunk/libobjc/ChangeLog
    trunk/libobjc/accessors.m

[Bug libobjc/47012] nonatomic Properties behave wrong

[nicola at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47012

--- Comment #4 from Nicola Pero <nicola at gcc dot gnu.org> 2010-12-19 19:12:33 UTC ---
Yes, I was actually thinking about this, and you're right - it makes sense not
to use retain/autorelease! ;-)

'nonatomic' means that other threads are not involved.  Which also means that
the programmer calling the accessor has full control of what happens (there
aren't alternative flows of execution that may jump in); he should do the
retain/autorelease himself if there is a risk that something he does while
using the object returned may call the accessor setter and trigger a release of
the object; else, he can get away without a retain/autorelease and get a good
speedup.

And doing the same that Apple does is obviously helpful for portability.

So I made the change in subversion.

Thanks

PS: GCC is currently in bug-fixing mode only for 4.6 so we can't accept
non-bug-fix changes, but as soon as it reopens, you're very welcome to
contribute.  Faster method lookup sounds very exciting (and non-trivial).

[Bug libobjc/47012] nonatomic Properties behave wrong

[nicola at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47012

Nicola Pero <nicola at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #5 from Nicola Pero <nicola at gcc dot gnu.org> 2010-12-19 19:14:49 UTC ---
Fixed in trunk (will be GCC 4.6).

Thanks

[Bug libobjc/47012] nonatomic Properties behave wrong

[js-gcc at webkeks dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47012

--- Comment #6 from js-gcc at webkeks dot org <js-gcc at webkeks dot org> 2010-12-19 19:19:33 UTC ---
Well, I did not plan to get that included in 4.6.

If you are interested in optimizing the lookup: The lookup could be greatly
improved if we also change the ABI, which is currently the limitation as the
current ABI does not allow for coherent inline caching etc. This needs changes
in the compiler, therefore I did not do that - maintaining a runtime AND
patchset for two compilers really is too much work for a single person ;).

I'd love to work on that, will you visit FOSDEM next year? If so, we could meet
up there and hack on that :). I'd really love to reduce the extremely huge
speed difference between the GNU runtime and the Apple runtime. At the moment,
dispatch is more than 5 times faster with GNU (at least that's what I measured
some time ago on Leopard, I guess with Snow Leopard it might be even more).

[Bug libobjc/47012] nonatomic Properties behave wrong

[nicola at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47012

--- Comment #7 from Nicola Pero <nicola at gcc dot gnu.org> 2010-12-21 10:56:33 UTC ---
Yes, I'll visit Fosdem - let's meet up there.

> In the code you linked, is that objc_mutex_t a spinlock? If not,
> this might be performance problem.

You are right, it's a normal lock.  Currently, libobjc just uses plain mutex
locks everywhere.  We should improve on that in GCC 4.7 - there's lot that
can be done.

Thanks

[Bug libobjc/47012] nonatomic Properties behave wrong

[js-gcc at webkeks dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47012

--- Comment #8 from js-gcc at webkeks dot org <js-gcc at webkeks dot org> 2010-12-21 11:26:37 UTC ---
Hm, a mutex can be a real problem there, I guess. Accessors are used all the
time, having a kernel lock each time will be a giant slowdown, especially as
most of the time, the lock is not held anyway and if it is, it's only held for
a very short period of time.

I think this should be changed to spinlocks, even for 4.6. Should I create a
bug report for that?

If the problem is that we don't know whether we have a spinlock implementation
on the system and would need to patch configure.ac and this is too much change
before the release, we could just use gcc's __sync_bool_compare_and_swap, spin
10 times and if we still could not acquire our spinlock give control to another
process using yield().

[Bug libobjc/47012] nonatomic Properties behave wrong

[nicola at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47012

--- Comment #9 from Nicola Pero <nicola at gcc dot gnu.org> 2010-12-21 11:38:56 UTC ---
Sure, go ahead and create a bug for it.  We can make the change for 4.6 if we
make it safe.  By the way, changing configure.ac is not a problem, we can do
that, particularly if it makes the change safer. ;-)

It could make the change safer - we could check that spinlocks are available,
and use them if so; if not, we just fall back to using a standard mutex, which
is slower but works on all platforms.

Thanks