Closed mbutrovich closed 6 years ago
coveralls
commented
6 years ago Coverage increased (+0.05%) to 77.91% when pulling 3476c5f47247204d5ebf49c964d61ceac0585938 on mbutrovich:mvto into bf7ff625e86f2917bc939f85c41e810eec3588ec on cmu-db:master.
pmenon
commented
6 years ago @mbutrovich Prior to this PR, a tuple's write lock was inlined into its header. It looks like you've extracted it out, requiring one more random reference to lock a tuple. Have you thought about the performance implications of doing this? Also, did you happen to run tpcc to see how this impacts performance? I realize performance is bad as it is, but I suspect as we fix those issues this will come up. @yingjunwu can chime in here, too.
Also, why was this merged without a review from someone familiar with storage/CC? And why was this merged without passing the Jenkins build, as well?
mbutrovich
commented
6 years ago @pmenon TPC-C and YCSB looked the same to me, but you're right, the added lookup might slow us down in the future when other areas are sped up.
Re: storage/CC, AFAIK @poojanilangekar would probably be the only one at this point, but not sure her availability for refactor PRs so went with @pervazea
Re: Jenkins, everything was failing on Friday with Jenkins when they'd fail to install packages or pull from Git. Need to revisit on Monday.
This is just a refactor of the TileGroupHeader to make it a bit more modifiable/maintainable. I got rid of the char array that used hardcoded pointer arithmetic to access tuple "fields". Made each tuple header slot be a struct of fields, and made an array of those to represent all of the tuples in the TileGroup. These are all managed by smart pointers now.
There's also no "reserved" area in a tuple header anymore, since it was full anyway for other purposes (spin latch and last reader) so I broke those out into proper fields.