Skip to content

[runtime] move the empty Dictionary and Set singletons to rodata #5850

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 19, 2016
Merged

[runtime] move the empty Dictionary and Set singletons to rodata #5850

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions stdlib/public/SwiftShims/GlobalObjects.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,44 @@ struct _SwiftEmptyArrayStorage {
extern SWIFT_RUNTIME_STDLIB_INTERFACE
struct _SwiftEmptyArrayStorage _swiftEmptyArrayStorage;

struct _SwiftUnsafeBitMap {
__swift_uintptr_t *values;
__swift_intptr_t bitCount;
};

struct _SwiftDictionaryBodyStorage {
__swift_intptr_t capacity;
__swift_intptr_t count;
struct _SwiftUnsafeBitMap initializedEntries;
void *keys;
void *values;
};

struct _SwiftSetBodyStorage {
__swift_intptr_t capacity;
__swift_intptr_t count;
struct _SwiftUnsafeBitMap initializedEntries;
void *keys;
};

struct _SwiftEmptyDictionaryStorage {
struct HeapObject header;
struct _SwiftDictionaryBodyStorage body;
__swift_uintptr_t entries;
};

struct _SwiftEmptySetStorage {
struct HeapObject header;
struct _SwiftSetBodyStorage body;
__swift_uintptr_t entries;
};

extern SWIFT_RUNTIME_STDLIB_INTERFACE
struct _SwiftEmptyDictionaryStorage _swiftEmptyDictionaryStorage;

extern SWIFT_RUNTIME_STDLIB_INTERFACE
struct _SwiftEmptySetStorage _swiftEmptySetStorage;

struct _SwiftHashingSecretKey {
__swift_uint64_t key0;
__swift_uint64_t key1;
Expand Down
34 changes: 9 additions & 25 deletions stdlib/public/core/HashedCollections.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

@jrose-apple jrose-apple Nov 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These classes don't contain exactly one member; I'm not sure what you're picturing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. You're picturing defining it only once in Stubs, and then importing it into Swift?

Should work... what special rule is needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely plan for the compiler to be allowed to reorder class and struct members, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok wait does putting this stuff in rodata even make sense? The reference count has to be mutable, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I have no problem taking this patch in its current state.

// See stdlib/public/stubs/GlobalObjects.cpp for details.
@objc_non_lazy_realization
internal class _RawNative${Self}Storage:
_SwiftNativeNS${Self}, _NS${Self}Core
{
Expand All @@ -2496,31 +2501,10 @@ internal class _RawNative${Self}Storage:
/// The empty singleton that is used for every single Dictionary that is
/// created without any elements. The contents of the storage should never
/// be mutated.
// FIXME: this should be set up in rodata like the Array singleton
internal static let empty: RawStorage = {
let storage = Builtin.allocWithTailElems_1(RawStorage.self,
1._builtinWordValue, UInt.self)

// We set the capacity to 1 so that there's an empty hole to search.
// Any insertion will lead to a real storage being allocated, because
// Dictionary guarantees there's always another empty hole after insertion.
storage.capacity = 1
storage.count = 0

// Initialize pointers to garbage; they should never be loaded
storage.keys = UnsafeMutableRawPointer(bitPattern: 1)!
% if Self == 'Dictionary':
storage.values = UnsafeMutableRawPointer(bitPattern: 1)!
% end

let initializedEntries = _UnsafeBitMap(
storage: storage._initializedHashtableEntriesBitMapBuffer,
bitCount: 1)
initializedEntries.initializeToZero()

storage.initializedEntries = initializedEntries
return storage
}()
internal static var empty: RawStorage {
return Builtin.bridgeFromRawPointer(
Builtin.addressof(&_swiftEmpty${Self}Storage))
}

// This type is made with allocWithTailElems, so no init is ever called.
// But we still need to have an init to satisfy the compiler.
Expand Down
64 changes: 64 additions & 0 deletions stdlib/public/stubs/GlobalObjects.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ namespace swift {
// _direct type metadata for Swift._EmptyArrayStorage
SWIFT_RUNTIME_STDLIB_INTERFACE
extern "C" ClassMetadata _TMCs18_EmptyArrayStorage;

// _direct type metadata for Swift._RawNativeDictionaryStorage
SWIFT_RUNTIME_STDLIB_INTERFACE
extern "C" ClassMetadata _TMCs27_RawNativeDictionaryStorage;

// _direct type metadata for Swift._RawNativeSetStorage
SWIFT_RUNTIME_STDLIB_INTERFACE
extern "C" ClassMetadata _TMCs20_RawNativeSetStorage;
}

swift::_SwiftEmptyArrayStorage swift::_swiftEmptyArrayStorage = {
Expand All @@ -41,6 +49,62 @@ swift::_SwiftEmptyArrayStorage swift::_swiftEmptyArrayStorage = {
}
};



swift::_SwiftEmptyDictionaryStorage swift::_swiftEmptyDictionaryStorage = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you mark this const and constexpr so that we guarantee it ends up in rodata?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Want me to do that to the existing Array decl too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// HeapObject header;
{
&_TMCs27_RawNativeDictionaryStorage, // isa pointer
},

// _SwiftDictionaryBodyStorage body;
{
// We set the capacity to 1 so that there's an empty hole to search.
// Any insertion will lead to a real storage being allocated, because
// Dictionary guarantees there's always another empty hole after insertion.
1, // int capacity;
0, // int count;

// _SwiftUnsafeBitMap initializedEntries
{
&swift::_swiftEmptyDictionaryStorage.entries, // unsigned int* values;
1 // int bitCount; (1 so there's something for iterators to read)
},

(void*)1, // void* keys; (non-null garabage)
(void*)1 // void* values; (non-null garbage)
},

0 // int entries; (zero'd bits)
};


swift::_SwiftEmptySetStorage swift::_swiftEmptySetStorage = {
// HeapObject header;
{
&_TMCs20_RawNativeSetStorage, // isa pointer
},

// _SwiftDictionaryBodyStorage body;
{
// We set the capacity to 1 so that there's an empty hole to search.
// Any insertion will lead to a real storage being allocated, because
// Dictionary guarantees there's always another empty hole after insertion.
1, // int capacity;
0, // int count;

// _SwiftUnsafeBitMap initializedEntries
{
&swift::_swiftEmptySetStorage.entries, // unsigned int* values;
1 // int bitCount; (1 so there's something for iterators to read)
},

(void*)1 // void* keys; (non-null garabage)
},

0 // int entries; (zero'd bits)
};

static __swift_uint64_t randomUInt64() {
#if defined(__APPLE__)
return static_cast<__swift_uint64_t>(arc4random()) |
Expand Down