Optimized reading rows in a sequential mode, if they are already in the buffer#3428
Conversation
There was a problem hiding this comment.
Thanks for investigating and optimizing this @vonzshik... great to see sequential getting some love :) Do you have the updated stats on sequential vs. non-sequential? I'm curious where the gap is (if at all) after this.
One thought about this... If I'm reading the code right, your optimization only works if the entire message fits in the buffer. If it doesn't, and the user reads additional data into the buffer, and then does Read to go to the next row, we still invoke ConsumeRowSequential although we don't have to. In other words, unless I'm mistaken, we could update _dataMsgEnd as we read row data, so that the optimization would kick in even for rows that are larger than the buffer. I'm not sure exactly how valuable this would be, what do you think?
Yeah, that's exactly how it works. I did take a look, and that's a very tricky to fix. The problem is, |
Not sure what you've meant here - do you want me to test some other cases? |
|
@vonzshik am just curious if after your changes sequential is still slower than non-sequential, and by how much :) But not urgent or blocking for this PR of course. |
Yeah, I figured as much. FWIW the usage of _dataMsgEnd is slightly weird to me; previously it really was just an end position of the row in the buffer (because the entire row was always contained in the buffer). Now it can also be a point way beyond the end of the buffer, which actually has no real meaning in that case. I think it should be possible to update it as we read more data into the buffer (or some similar mechanism), but I'm also fine with merging as-is if you don't feel like the extra effort is worth it (this is already a really nice improvement). |
|
Backported to 5.0.2 via 945adc7 |
|
@roji I did some more benchmarking - in some cases sequential read might be slower by 2-3%. I'll create another issue and post my findings there. |
|
Note for the future: I think we shouldn't be releasing perf optimizations such as this in patch versions... We should strive to only fix bugs, and where it really makes sense, merge very low-risk features which have a lot of value. |
Fully agree with this. Although, I do think th 8000 is pr is relatively low-risk and does bring a lot of value. But if you're still against it, I'm willing to revert it. |
|
I do think we're better off reverting this - after all it really is a perf optimization, rather than a feature that could unblocked someone out there. The code around sequential/non-sequential reading has also proven to be trickier than it looks, so from a risk perspective this isn't great. But I'll leave it up to your judgement... I just prefer to not have to worry about new bugs in patch releases... |
|
Personally, I would like to have it in the upcoming release since it tunes perf a bit, but without drastic changes, and it doesn't look dangerous. It's clean and simple. |
|
😩 |
|
You still can revert the revert by force pushing to the hotfix branch 😈 |
|
Well, if we're going to do it right, we will do it right. We can always release 5.1 in the next few months. |
|
Not sure about it since there's not so much changes for 5.1, so I bet the next release is 6.0. |
|
We can indeed decide to release 5.1 at some point - this may be a good reason to hold off on breaking changes (since we don't want those in 6.0). Th 3D16 e question is if we'll have enough interesting stuff to warrant a 5.1. |
Fixes #2053
For comparison:
Before
After