This is an archive of the discontinued LLVM Phabricator instance.

Convert some ObjC retain/release msgSends to runtime calls
ClosedPublic

Authored by pete on Dec 18 2018, 4:52 PM.

Details

Summary

It is faster to directly call the ObjC runtime for methods such as retain/release instead of sending a message to those functions.

This is an extension of the work we already do to convert alloc message sends to objc_alloc calls.

Diff Detail

Repository
rC Clang

Event Timeline

pete created this revision.Dec 18 2018, 4:52 PM

So, once upon a time this was a problem because we were rewriting the method calls in the runtime itself. Can you explain how that's not a problem now? Do we just expect the runtime to be compiled with the right switch?

include/clang/Basic/ObjCRuntime.h
191

You're also rewriting autorelease.

207

Case indentation.

lib/CodeGen/CodeGenModule.h
170

Period (and on the other comments here)

pete marked 3 inline comments as done.Dec 19 2018, 10:57 PM

So, once upon a time this was a problem because we were rewriting the method calls in the runtime itself. Can you explain how that's not a problem now? Do we just expect the runtime to be compiled with the right switch?

Ah yeah, good point.

In the patch for rewriting objc_alloc messages (r348687) I added a new option (-fno-objc-convert-messages-to-runtime-calls) which the objc runtime will need to adopt.

pete updated this revision to Diff 179017.Dec 19 2018, 10:58 PM

Okay. That's also presumably true for open-source runtimes that support ARC; tagging David Chisnall and Jonathan Schleifer to get their input.

js added a comment.Dec 20 2018, 9:06 AM

Thanks for tagging me!

The ObjFW runtime itself does not contain anything about release, retain or autorelease - these are all part of ObjFW (as in the framework). As ObjFW also supports the Apple runtime, as well as mixing with Foundation code, it so far only provides objc_retain and objc_release when they are missing, to make ARC work. They just call into the corresponding methods on the object and do nothing else.

How will this change work from the Apple runtime? Will objc_retain/objc_release call the retain/release on the object if it implements its own? Should I drop my own retain/release implementation from my root object if I am compiling against the Apple runtime?

I'm mostly concerned about mixing ObjFW and Foundation code while using the Apple runtime here. ObjFW with the ObjFW runtime and no Foundation is easy to change.

Okay. That's also presumably true for open-source runtimes that support ARC; tagging David Chisnall and Jonathan Schleifer to get their input.

shouldUseARCFunctionsForRetainRelease() returns false for the other runtimes. This seems like the right thing to do until/unless they add support.

Would make sense for the driver to additionally imply -fno-objc-convert-messages-to-runtime-calls for other runtimes as a bigger hammer?

js added a comment.Dec 20 2018, 9:16 AM

Okay. That's also presumably true for open-source runtimes that support ARC; tagging David Chisnall and Jonathan Schleifer to get their input.

shouldUseARCFunctionsForRetainRelease() returns false for the other runtimes. This seems like the right thing to do until/unless they add support.

Would make sense for the driver to additionally imply -fno-objc-convert-messages-to-runtime-calls for other runtimes as a bigger hammer?

We could make it true here (happy to implement it), but the question about how this works with multiple root objects with different retain/release implementations remains.

In D55869#1337706, @js wrote:

The ObjFW runtime itself does not contain anything about release, retain or autorelease - these are all part of ObjFW (as in the framework). As ObjFW also supports the Apple runtime, as well as mixing with Foundation code, it so far only provides objc_retain and objc_release when they are missing, to make ARC work. They just call into the corresponding methods on the object and do nothing else.

How will this change work from the Apple runtime? Will objc_retain/objc_release call the retain/release on the object if it implements its own? Should I drop my own retain/release implementation from my root object if I am compiling against the Apple runtime?

Yes, the idea is that the specialized runtime functions are fast paths for the common case, but they still fall back if the object has overridden them.

js added a comment.Dec 20 2018, 9:19 AM
In D55869#1337706, @js wrote:

The ObjFW runtime itself does not contain anything about release, retain or autorelease - these are all part of ObjFW (as in the framework). As ObjFW also supports the Apple runtime, as well as mixing with Foundation code, it so far only provides objc_retain and objc_release when they are missing, to make ARC work. They just call into the corresponding methods on the object and do nothing else.

How will this change work from the Apple runtime? Will objc_retain/objc_release call the retain/release on the object if it implements its own? Should I drop my own retain/release implementation from my root object if I am compiling against the Apple runtime?

Yes, the idea is that the specialized runtime functions are fast paths for the common case, but they still fall back if the object has overridden them.

I am guessing not just overridden, but also a root class providing one? IOW, the fast path is used when the class does not have the retain or release method at all? In that case, LGTM as is.

pete added a comment.Dec 20 2018, 9:32 AM
In D55869#1337723, @js wrote:
In D55869#1337706, @js wrote:

The ObjFW runtime itself does not contain anything about release, retain or autorelease - these are all part of ObjFW (as in the framework). As ObjFW also supports the Apple runtime, as well as mixing with Foundation code, it so far only provides objc_retain and objc_release when they are missing, to make ARC work. They just call into the corresponding methods on the object and do nothing else.

How will this change work from the Apple runtime? Will objc_retain/objc_release call the retain/release on the object if it implements its own? Should I drop my own retain/release implementation from my root object if I am compiling against the Apple runtime?

Yes, the idea is that the specialized runtime functions are fast paths for the common case, but they still fall back if the object has overridden them.

I am guessing not just overridden, but also a root class providing one? IOW, the fast path is used when the class does not have the retain or release method at all? In that case, LGTM as is.

The Apple runtime is using a bit on each realized class to track the overriding. The root class defaults to not having a custom RR, ie, it appears that its version of RR is considered equivalent to objc_retain.

Presumably that would apply to other root classes too, although i'm not 100% sure. I did see some special handling of NSObject in there too so perhaps only its RR is equivalent to objc_retain/objc_release.

In D55869#1337706, @js wrote:

Thanks for tagging me!

The ObjFW runtime itself does not contain anything about release, retain or autorelease - these are all part of ObjFW (as in the framework). As ObjFW also supports the Apple runtime, as well as mixing with Foundation code, it so far only provides objc_retain and objc_release when they are missing, to make ARC work. They just call into the corresponding methods on the object and do nothing else.

How will this change work from the Apple runtime? Will objc_retain/objc_release call the retain/release on the object if it implements its own? Should I drop my own retain/release implementation from my root object if I am compiling against the Apple runtime?

Nothing is fundamentally changing. The Apple runtime has fast paths in objc_retain / objc_release for NSObject's implementation of retain/release, and this patch allows us to take advantage of that in non-ARC files which explicitly call -retain and -release. Of course, the runtime's own implementation of objc_retain and objc_release contains such calls, which would be miscompiled into recursive calls to objc_retain and objc_release without the command-line flag.

I'm mostly concerned about mixing ObjFW and Foundation code while using the Apple runtime here. ObjFW with the ObjFW runtime and no Foundation is easy to change.

I assume you only define objc_retain and objc_release when compiling against your own runtime. In that case, as Duncan reminded me, there's no risk of miscompiling as long as shouldUseARCFunctionsForRetainRelease() returns false for your runtime. Since it sounds like your implementation doesn't try to avoid the message-send for common classes, there's no reason you would make that method return true; but if you ever want to, you'll need this compiler flag to avoid miscompiles.

In D55869#1337723, @js wrote:
In D55869#1337706, @js wrote:

The ObjFW runtime itself does not contain anything about release, retain or autorelease - these are all part of ObjFW (as in the framework). As ObjFW also supports the Apple runtime, as well as mixing with Foundation code, it so far only provides objc_retain and objc_release when they are missing, to make ARC work. They just call into the corresponding methods on the object and do nothing else.

How will this change work from the Apple runtime? Will objc_retain/objc_release call the retain/release on the object if it implements its own? Should I drop my own retain/release implementation from my root object if I am compiling against the Apple runtime?

Yes, the idea is that the specialized runtime functions are fast paths for the common case, but they still fall back if the object has overridden them.

I am guessing not just overridden, but also a root class providing one? IOW, the fast path is used when the class does not have the retain or release method at all? In that case, LGTM as is.

The Apple runtime is using a bit on each realized class to track the overriding. The root class defaults to not having a custom RR, ie, it appears that its version of RR is considered equivalent to objc_retain.

Presumably that would apply to other root classes too, although i'm not 100% sure. I did see some special handling of NSObject in there too so perhaps only its RR is equivalent to objc_retain/objc_release.

objc_retain and objc_release are always defined as being equivalent to doing a normal message send of the corresponding messages. Apple's implementations detect when a class simply inherits NSObject's implementations: if so, they just directly use the NSObject RR implementation, and if no, they do a normal message send. That fast path is only triggered if the class's root class is NSObject and it doesn't override any of the RR methods.

This should be fine for the GNUstep runtime (the GCC runtime doesn't support ARC at all). My main concern is that it will break already-released versions of the runtime built with a newer version of clang. I can easily enable a new flag in the next release, but doing so for older ones is more problematic.

This should be fine for the GNUstep runtime (the GCC runtime doesn't support ARC at all). My main concern is that it will break already-released versions of the runtime built with a newer version of clang. I can easily enable a new flag in the next release, but doing so for older ones is more problematic.

Well, it won't break as long as you don't tell the compiler that this is an acceptable rewrite to do. I'm not sure there's a perfect way to stage this if/when you start thinking about enabling that — you can of course start passing the "don't rewrite message sends" flag in new releases of GNUstep, but it'll always be *possible* that someone might use a newer Clang to compile an older version of the runtime that doesn't do that workaround. On the other hand, the brokenness of the result will not be subtle.

pete added a comment.Dec 21 2018, 9:08 AM

Thanks for all the feedback so far. Is there anything else you'd like me to change before I can land this?

rjmccall accepted this revision.Dec 21 2018, 12:35 PM

It sounds like it's fine.

This revision is now accepted and ready to land.Dec 21 2018, 12:35 PM
This revision was automatically updated to reflect the committed changes.
pete added a comment.Dec 21 2018, 1:04 PM

It sounds like it's fine.

Thanks for the review! Just pushed it as r349952.