Skip to content
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

Optimise FlatDictionary init for scenarios where no values are stored and llow inlining access #456

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Optimise FlatDictionary init for scenarios where no values are stored and llow inlining access #456

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c1777d
Optimise FlatDictionary init for scenarios where no values are stored…
Joannis May 19, 2024
1fb5a7f
Make FlatDictionary.Storage sendable
adam-fowler May 28, 2024
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
118 changes: 84 additions & 34 deletions Sources/HummingbirdCore/Utils/FlatDictionary.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,104 +26,154 @@ public struct FlatDictionary<Key: Hashable, Value>: Collection, ExpressibleByDic
public typealias Element = (key: Key, value: Value)
public typealias Index = Array<Element>.Index

@usableFromInline
struct Storage {
@usableFromInline internal /* private */ var elements: [Element]
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the commented out /* private */ if we require everything to be internal

@usableFromInline internal /* private */ var hashKeys: [Int]

@usableFromInline
init(elements: [Element], hashKeys: [Int]) {
self.elements = elements
self.hashKeys = hashKeys
}
}

@usableFromInline internal /* private */ var storage: Storage?

// MARK: Collection requirements

/// The position of the first element
public var startIndex: Index { self.elements.startIndex }
@inlinable
public var startIndex: Index { self.storage?.elements.startIndex ?? 0 }
/// The position of the element just after the last element
public var endIndex: Index { self.elements.endIndex }
@inlinable
public var endIndex: Index { self.storage?.elements.endIndex ?? 0 }
/// Access element at specific position
public subscript(_ index: Index) -> Element { return self.elements[index] }
@inlinable
public subscript(_ index: Index) -> Element { return self.storage!.elements[index] }
Copy link
Member

Choose a reason for hiding this comment

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

! isn't ideal here, perhaps we should add a precondition that storage exist and provide an out of range message or something similar, given the index will be out of range

/// Returns the index immediately after the given index
public func index(after index: Index) -> Index { self.elements.index(after: index) }
@inlinable
public func index(after index: Index) -> Index { index + 1 }
Copy link
Member

Choose a reason for hiding this comment

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

Any reason you changed this? I know index+1 is correct in most cases, but what is the expected result when index == endIndex?


/// Create a new FlatDictionary
public init() {
self.elements = []
self.hashKeys = []
}
@inlinable
public init() {}

/// Create a new FlatDictionary initialized with a dictionary literal
public init(dictionaryLiteral elements: (Key, Value)...) {
self.elements = elements.map { (key: $0.0, value: $0.1) }
self.hashKeys = elements.map {
Self.hashKey($0.0)
}
self.storage = Storage(
elements: elements.map { (key: $0.0, value: $0.1) },
hashKeys: elements.map {
Self.hashKey($0.0)
}
)
}

/// Create a new FlatDictionary from an array of key value pairs
public init(_ values: [Element]) {
self.elements = values
self.hashKeys = values.map {
Self.hashKey($0.key)
}
self.storage = Storage(
elements: values,
hashKeys: values.map {
Self.hashKey($0.0)
}
)
}

/// Access the value associated with a given key for reading and writing
///
/// Because FlatDictionary allows for key clashes this function will
/// return the first entry in the array with the associated key
@inlinable
public subscript(_ key: Key) -> Value? {
get {
let hashKey = Self.hashKey(key)
if let index = hashKeys.firstIndex(of: hashKey) {
return self.elements[index].value
if let storage, let index = storage.hashKeys.firstIndex(of: hashKey) {
return storage.elements[index].value
} else {
return nil
}
}
set {
let hashKey = Self.hashKey(key)
if let index = hashKeys.firstIndex(of: hashKey) {
if var storage, let index = storage.hashKeys.firstIndex(of: hashKey) {
Copy link
Member

Choose a reason for hiding this comment

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

The ordering of the if statements here is slightly confusing. Currently you test for the existence of storage twice when the key doesn't exist. Perhaps it should be

if let storage {
    // do stuff when storage already exists
} else {
    // create storage if needed
}

if let newValue {
self.elements[index].value = newValue
storage.elements[index].value = newValue
} else {
self.elements.remove(at: index)
self.hashKeys.remove(at: index)
storage.elements.remove(at: index)
storage.hashKeys.remove(at: index)
}

self.storage = storage
} else if let newValue {
self.elements.append((key: key, value: newValue))
self.hashKeys.append(hashKey)
if var storage {
storage.elements.append((key: key, value: newValue))
storage.hashKeys.append(hashKey)
self.storage = storage
} else {
self.storage = Storage(
elements: [(key: key, value: newValue)],
hashKeys: [hashKey]
)
}
}
}
}

/// Return all the values, associated with a given key
@inlinable
public subscript(values key: Key) -> [Value] {
guard let storage else {
return []
}

var values: [Value] = []
let hashKey = Self.hashKey(key)

for hashIndex in 0..<self.hashKeys.count {
if self.hashKeys[hashIndex] == hashKey {
values.append(self.elements[hashIndex].value)
for hashIndex in 0..<storage.hashKeys.count {
if storage.hashKeys[hashIndex] == hashKey {
values.append(storage.elements[hashIndex].value)
}
}
return values
}

/// Return if dictionary has this value
/// - Parameter key:
@inlinable
public func has(_ key: Key) -> Bool {
guard let storage else {
return false
}

let hashKey = Self.hashKey(key)
return self.hashKeys.firstIndex(of: hashKey) != nil
return storage.hashKeys.firstIndex(of: hashKey) != nil
}

/// Append a new key value pair to the list of key value pairs
@inlinable
public mutating func append(key: Key, value: Value) {
let hashKey = Self.hashKey(key)
self.elements.append((key: key, value: value))
self.hashKeys.append(hashKey)

if var storage {
storage.elements.append((key: key, value: value))
storage.hashKeys.append(hashKey)
self.storage = storage
} else {
self.storage = Storage(
elements: [(key: key, value: value)],
hashKeys: [hashKey]
)
}
}

private static func hashKey(_ key: Key) -> Int {
@usableFromInline
internal /* private */ static func hashKey(_ key: Key) -> Int {
var hasher = Hasher()
hasher.combine(key)
return hasher.finalize()
}

private var elements: [Element]
private var hashKeys: [Int]
}

// FlatDictionary is Sendable when Key and Value are Sendable
extension FlatDictionary.Storage: Sendable where Key: Sendable, Value: Sendable {}
Copy link
Member

Choose a reason for hiding this comment

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

I added this in to get rid of a Sendable warning

extension FlatDictionary: Sendable where Key: Sendable, Value: Sendable {}
Loading