dict: Change to a sticky Copy-On-Write based table. #259
Conversation
This improves the speed of the previous dict implementation through a reduction in the number of atomic loads in the read path (max 1 when the dict is read-only - think globals) as well as the number of allocations needed in the write path. Overall the performance is improved by about 30%. Some of the major changes are as follows: * The internal table layout was changed from []*dictEntry to []dictEntry reducing a memory indirection as well as hopefully improving the speed of slot probing in insertAbsentEntry as well as lookupEntry. * Many iteration operations which might have needed to grab a relatively expensive lock previously can now do so without locking if the dict is in the read-only mode. * The sizeof(Dict) increased some as a few variables (used and fill) were moved from the dictTable to Dict itself. The addition of a `write` and `misses` values to the Dict makes the overall memory usage of Dict generally larger. This is offset by the type change of dictTable and the reduction of additional pointers there. An empty dict weighs in at 304 bytes compared to 176 previously. At 4 elements, both the old and new implementation use 304 bytes of memory. From that point on, the new implementation actually uses less memory.
|
This is really cool. Thanks for preparing this PR. It's a bit involved so I may take a few days to get through it. At a high level, I wanted to point out that the immutability of dictEntry objects is an important invariant of the current design. I can't remember all the details off hand but I think it ultimately comes down to not being able to update both the key and value atomically. Can you comment on why this is no longer important in this design? |
|
Sorry about the size. It wouldn't surprise me to take some time - the nice part is that this is a performance change, not new functionality so time is on our side. As an overview, the implementation is a blending of the table layout of the Python dict implementation, and a concurrency implementation borrowed from golang/sync@54b13b0. In that light, writes to the table are serialized by a mutex, while reads are performed from a immutable table (assuming the map has transitioned to fast reads). This means that for heavy read-only dicts (think globals) most reads only have the cost of a single atomic read. For heavy read/write dicts, the cost mostly the overhead of locking the If anything I write here would make better sense in the code or commit comment, let me know. I'd also be happy to add/modify existing benchmarks if you think that would be good to make sure this is as good an improvement as I think it is. The main goal for me has been to try to get the CallSimple benchmark closer to the speed that CPython is able to execute it (12ms). |
There was a problem hiding this comment.
Overall I just have a few style comments. The implementation is really interesting. For mostly-write and mostly-read dicts it makes a lot of sense to me why this would be more efficient. But are there workloads where it would be less efficient? ISTM you may do a lot of copying when you do a write/read/write/read access pattern. Maybe a benchmark around this would be useful.
| @@ -47,25 +53,16 @@ type dictEntry struct { | |||
| value *Object | |||
| } | |||
|
|
|||
| func (d dictEntry) isEmpty() bool { return d.key == nil } | |||
There was a problem hiding this comment.
Can these be *dictEntry methods, even though we don't explicitly use *dictEntry anymore? My Go understanding here is a little confused. I know that you can call *Mutex methods on var m Mutex so it stands to reason you could call *dictEntry methods on elements of []dictEntry
| // By having it a value type, the compiler is able to make all | ||
| // `&deletedEntry` effectively constant (avoids a memory read if this | ||
| // had a pointer type). | ||
| deletedEntry Object |
There was a problem hiding this comment.
Maybe this should now be called deletedKey
| } | ||
| d.mutex.Unlock(f) | ||
| } | ||
| return |
| } | ||
| return entry | ||
| return |
| table.insertAbsentEntry(&dictEntry{hashString(key), NewStr(key).ToObject(), value}) | ||
| table.insertAbsentEntry(dictEntry{hashString(key), NewStr(key).ToObject(), value}) | ||
| } | ||
| d := &Dict{ |
There was a problem hiding this comment.
Why not:
return &Dict{
...
}
| // pointer to d.write (which would be bad). | ||
| atomic.StorePointer(d.unsafeReadTablePointer(), unsafe.Pointer(&table)) | ||
| d.misses = 0 | ||
| return |
| @@ -323,14 +343,41 @@ func (d *Dict) incVersion() { | |||
| atomic.AddInt64(&d.version, 1) | |||
| } | |||
|
|
|||
| // populateWriteTable makes sure that d.write is populated with the dict's | |||
| // table, possibly copying it from the read table. | |||
There was a problem hiding this comment.
Mention that lock must be held during this call
| // then the programmer hasn't bothered to implement proper locking in | ||
| // their code and in reality they don't know which statement is | ||
| // happening before and which is happening after (mutator vs. eq). | ||
| // Additionally, it shouldn't matter |
There was a problem hiding this comment.
Adjust line wrap formatting
| table = *d.loadReadTable() | ||
| d.mutex.Unlock(f) | ||
| goto top | ||
| } else if d.misses > d.used { |
There was a problem hiding this comment.
This line could use a comment. It's not clear to me what this comparison means.
There was a problem hiding this comment.
If more people is writing than reading, the de facto table is the wrote one, not the read one, IIUC.
This improves the speed of the previous dict implementation through a reduction in the number of atomic loads in the read path (max 1 when the dict is read-only - think globals) as well as the number of allocations needed in the write path.
Overall the performance is improved by about 30%.
Some of the major changes are as follows:
The internal table layout was changed from []*dictEntry to []dictEntry reducing a memory indirection as well as hopefully improving the speed of slot probing in insertAbsentEntry as well as lookupEntry.
Many iteration operations which might have needed to grab a relatively expensive lock previously can now do so without locking if the dict is in the read-only mode.
The sizeof(Dict) increased some as a few variables (used and fill) were moved from the dictTable to Dict itself. The addition of a
writeandmissesvalues to the Dict makes the overall memory usage of Dict generally larger. This is offset by the type change of dictTable and the reduction of additional pointers there. An empty dict weighs in at 304 bytes compared to 176 previously. At 4 elements, both the old and new implementation use 304 bytes of memory. From that point on, the new implementation actually uses less memory.Benchmark data (unchanged/statistically insignificant results removed):