bugfix: fix prefix cache evict bug. #55
Conversation
Signed-off-by: fangyuanhong1 <[email protected]>
yq33victor
force-pushed
the
dev/fix_hash_prefix
branch
from
04bfa25 to
4e37a0c
Compare
| @@ -166,7 +166,7 @@ void ChunkedPrefillScheduler::handle_running_queue_requests( | |||
| } | |||
|
|
|||
| // if (sequence->if_cache_block_for_prefill()) { | |||
| // block_manager_->cache(sequence.get()); | |||
| // block_manager_pool_->cache(sequence.get()); | |||
There was a problem hiding this comment.
remove these useless codes please.
| @@ -100,7 +100,7 @@ class BlockCapacityGuard { | |||
| std::vector<SequenceStatus> get_running_sequence_status(); | |||
|
|
|||
| private: | |||
| BlockManagerPool* block_manager_; | |||
| BlockManagerPool* block_manager_pool_; | |||
There was a problem hiding this comment.
it would be better to use smart pointer.
| @@ -233,6 +233,7 @@ size_t PrefixCacheHashMurmur3::evict(size_t n_blocks) { | |||
| delete del_node; | |||
| ++evict_count; | |||
| --num_blocks_; | |||
| ++i; | |||
There was a problem hiding this comment.
Using ++i in the for loop will cause a code logic error, it leads to an incorrect counting.
There was a problem hiding this comment.
Because we want to evict a total of n_blocks blocks here, but we will skip the blocks that are being shared. Moving ++i here prevents the for loop from stopping before evicting n_blocks blocks.
Maybe i is redundant here. we can directly use while(evict_count<n_blocks){...;++evict_count}.
| @@ -185,6 +185,7 @@ size_t PrefixCacheHashSha256::evict(size_t n_blocks) { | |||
| delete del_node; | |||
|
|
|||
| --num_blocks_; | |||
| ++evict_count; | |||
| @@ -77,7 +77,7 @@ void ChunkedPrefillScheduler::handle_abnormal_request( | |||
| << "Running queue size is not 1, there maybe a bug of request " | |||
| "preemption logic. running_queue_.size =" | |||
| << running_queue_.size(); | |||
| if (util::sum(block_manager_->num_used_blocks()) != | |||
| if (util::sum(block_manager_pool_->num_used_blocks()) != | |||
There was a problem hiding this comment.
can block_manager_pool_ be nullptr?
There was a problem hiding this comment.
block_manager_pool_ will never be a nullptr.
| @@ -126,7 +126,7 @@ class ContinuousScheduler : public Scheduler { | |||
| Engine* engine_; | |||
|
|
|||
| // the block manager to manage the cache blocks | |||
| BlockManagerPool* block_manager_; | |||
| BlockManagerPool* block_manager_pool_; | |||
There was a problem hiding this comment.
it would be better to move "renaming" to be a separate PR, to make the bugfix PR clear.
There was a problem hiding this comment.
it would be better to move "renaming" to be a separate PR, to make the bugfix PR clear.
ok, I will split the pr into two.
No description provided.