[runtime] move the empty Dictionary and Set singletons to rodata #5850
Conversation
|
@swift-ci please test |
| @@ -41,6 +49,62 @@ swift::_SwiftEmptyArrayStorage swift::_swiftEmptyArrayStorage = { | |||
| } | |||
| }; | |||
|
|
|||
|
|
|||
|
|
|||
| swift::_SwiftEmptyDictionaryStorage swift::_swiftEmptyDictionaryStorage = { | |||
There was a problem hiding this comment.
Can you mark this const and constexpr so that we guarantee it ends up in rodata?
There was a problem hiding this comment.
Want me to do that to the existing Array decl too?
There was a problem hiding this comment.
Argh:
/ContiguousArrayBuffer.swift:59:23: error: cannot pass immutable value as inout argument: '_swiftEmptyArrayStorage' is a 'let' constant
Builtin.addressof(&_swiftEmptyArrayStorage))
^~~~~~~~~~~~~~~~~~~~~~~~
Any bright ideas, or are we forced to give up here?
There was a problem hiding this comment.
Also: (void*)1 is illegal in a constexpr. I gave up and just made it nullptr with the understanding that there's no conceivable way a null non-nullable pointer that is never looked at could matter.
| @@ -2473,6 +2473,11 @@ collections = [ | |||
| /// keys, followed by the values. | |||
| // | |||
| // See the docs at the top of the file for more details on this type | |||
| // | |||
| // NOTE: The precise layout of this type is relied on in the runtime | |||
| // to provide a statically allocated empty singleton. | |||
There was a problem hiding this comment.
This is pretty awful and we should stop doing it [later]. Maybe we can make some rule about classes containing exactly one member, which could then be defined as a C struct in SwiftShims.
There was a problem hiding this comment.
These classes don't contain exactly one member; I'm not sure what you're picturing?
There was a problem hiding this comment.
Oh. You're picturing defining it only once in Stubs, and then importing it into Swift?
Should work... what special rule is needed?
There was a problem hiding this comment.
You'd need some rule that promises that the layout algorithm is what you expect, i.e. that the single member immediately follows the retain count header (modulo any alignment requirements), even given the empty superclass. I can't see what layout algorithm wouldn't do this, but then again @slavapestov recently discovered that our ObjC layout wasted 8 bytes for no particular reason on certain classes, so...
There was a problem hiding this comment.
We definitely plan for the compiler to be allowed to reorder class and struct members, though.
There was a problem hiding this comment.
Ok wait does putting this stuff in rodata even make sense? The reference count has to be mutable, right?
There was a problem hiding this comment.
Yeah, and even then it couldn't be in true const memory anyway because it contains absolute addresses that would need to be relocated at load time (until libswiftCore lives in the dyld shared cache, at least).
There was a problem hiding this comment.
I still think it'd be nice to put the Swift compiler in charge of emitting these objects instead of C++ code. Doesn't seem like it'd be terribly difficult to wire up, if you want to take a shot at it.
There was a problem hiding this comment.
In the interest of incremental improvement, are you fine with moving forward with this PR as-is for now? The Dictionary/Array layouts at this level in the system are basically finalized, so the only thing that could break these are someone implementing a new class layout algorithm.
There was a problem hiding this comment.
Sure, I have no problem taking this patch in its current state.
|
Build failed |
|
Build failed |
|
@swift-ci please test |
|
Build failed |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
Ah good, master is just broken. 🙃 |
|
Bruno reverted the offending commit. @swift-ci Please test |
|
Build failed |
|
Build failed |
|
@swift-ci please test |
|
Build failed |
|
@swift-ci please smoke test Linux Platform |
|
The Linux Test that's failing seems to be REPL tests looking for xcrun? I sincerely hope this PR isn't causing that... |
Basically just a mirror of what Array does.
Resolves rdar://18220063