Make RawTable::insert_no_grow unsafe
#254
Conversation
For performance reasons, this method _assumes_ that there is sufficient capacity for the new element, and it misbehaves otherwise, breaking invariants or even segfaulting. The necessary conditions could be checked, but if we're to keep it lean, it should be `unsafe`, so the burden is on the caller to ensure capacity. Fixes rust-lang#253.
| self.table.growth_left -= special_is_empty(old_ctrl) as usize; | ||
| // If we are replacing a DELETED entry then we don't need to update | ||
| // the load counter. | ||
| self.table.growth_left -= special_is_empty(old_ctrl) as usize; |
There was a problem hiding this comment.
Since this function is already unsafe, we could add an extra assume_no_deleted argument which assumes that the table is fresh and does not contain any DELETED entries? This would skip the check on old_ctrl and might improve performance in some cases.
However it might be exposing too much of the hash table internals. What do you think?
There was a problem hiding this comment.
So assume_no_deleted would still be a condition there, but one that you would hope is constant-propagated?
I think it's too much of a micro-optimization, especially for exposing internals. I ran some indexmap benchmarks that are heavy on rebuild_hash_table, which is a clear followed by a loop of insert_no_grow. About 50% of that function's perf profile is simply the bucket.write, mostly due to cache misses. Most of the remaining time is spread around finding the insert slot, and less than 4% on updating growth_left. I even tried hard-coding that to growth_left -= 1, but the difference was lost in the noise.
|
@bors r+ |
|
📌 Commit 26f725b has been approved by |
|
☀️ Test successful - checks-travis |
For performance reasons, this method assumes that there is sufficient
capacity for the new element, and it misbehaves otherwise, breaking
invariants or even segfaulting. The necessary conditions could be
checked, but if we're to keep it lean, it should be
unsafe, so theburden is on the caller to ensure capacity.
Fixes #253.