-
Notifications
You must be signed in to change notification settings - Fork 429
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.
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
Bag
improvements.
#354
Bag
improvements.
#354
Conversation
Do you have a benchmark to show what this accomplishes? |
Benchmark Source |
Those benchmarks don't seem like real-world usage. I would expect most bags to have <10 elements and nearly all to have <100 elements. Do you agree? If so, could you re-run your benchmark with realistic sizes? |
Sources/Bag.swift
Outdated
|
||
lastTimestamp = timestamp | ||
|
||
let token = Token(timestamp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, it doesn't seem like there's any point in using a timestamp. This could just use a counter? Bag
requires mutual exclusion for cross-thread use anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of a timestamp here is that the monotonic clock never overflows because the processor is going to break down way way before this happens...
I avoided monotonic counter because it was the original Bag
implementation ICYMI. Somehow it was worried that the monotonic counter would overflow, so a reference-type RemovalToken
was born to allow "reindexing" when it overflows.
That said like the 64-bit monotonic clock, the overflow practically can never happen unless we got 100% uptime for 100+ years running a tick per clock cycle at 4 GHz, yet a typical load-inc-store is gonna take >3 clock cycles. So it is up to us to do it on our own, or rely on the platform libraries, libdispatch in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure… but if we still used a UInt64
, then this would have the same domain. Using a counter would start at an earlier value. It would also only increment by 1 for each insertion—instead of skipping values in the case of a timestamp. So using a counter would be less likely to overflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, true.
@mdiep The rewritten benchmark. It is either operations on 1 or 8 reference-counted elements/observers, running for 2 million samples. Compiled with
Iterations are no doubt faster even at small element count — this alone is enough to justify the PR IMO. Insertions are not leading significantly here, despite showing a huge gap in the original benchmark with large iteration count above. Removals and copying are slower, and the assumption is that d6e525f leads to more work for these two operations. That said these are just one-time occurrence for every observation, and way less important than the iteration performance. |
Without a real world benchmark, I don't know how we can evaluate the performance of those two implementations and make an informed choice. |
Iteration alone is already fairly relevant, if one considers the multiplicative effect in composed signal graphs. Edit: For sure it is negligible in low-frequency paths, but then in high speed scrolling it is increasingly relevant with the amount of signals that were pulled by the collection/scroll view. |
But these seem like very low overhead. Will having from If not, we should consider the conciseness/maintainability of the code. |
I considered it qualifying both conciseness and speediness, after removing the opaque token type and the binary search. Pretty clean and straightforward, isn't it? :) Stepping aside from any claim in performance, the core of the PR is replacing the object token with a
I can't say it makes a difference on iOS given the practical amount of information expected to fit in the screen real estate with a reasonable scrolling speed. But together with cuts here and there, they could stack up to a considerable portion of CPU time, say on Mac when one has a dense collection view. Moreover, it isn't too bad to save some joules, is it? P.S. I am not sure how common one would use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall approach looks good now. Just a couple things need revision.
Sources/Bag.swift
Outdated
fileprivate var elements: ContiguousArray<BagElement<Element>> = [] | ||
/// A uniquely identifying token for removing a value that was inserted into a | ||
/// Bag. | ||
public typealias Token = UInt64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make this a struct
—not just a typealias
. A type alias won't prevent someone from passing a random UInt64
to remove(using:)
. Token
s shouldn't be publicly instantiable.
Sources/Bag.swift
Outdated
fileprivate var elements: ContiguousArray<Element> = [] | ||
fileprivate var tokens: ContiguousArray<Token> = [] | ||
|
||
private var identifier: UInt64 = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call this nextToken
?
Rename
RemovalToken
toBag.Token
. This could be source breaking, sinceBag
has to be parameterised.Replace the token object with a monotonic
UInt64
timestamp. In other words,Bag
itself no longer incurs an ARC overhead except for itsContiguousArray
storage. This improves the performance in iterations and copying.Store the elements and the tokens in SoA form, improving cache locality in iterations and token searching during removals. That said best case removals seem to be affected by this change.
Exploit the timestamps to accelerate average case removals via binary search.