bustub icon indicating copy to clipboard operation
bustub copied to clipboard

Replacer API leading to student confusion

Open lmwnshn opened this issue 6 years ago • 3 comments
trafficstars

The buffer pool manager has Pin() and Unpin() functions. This is standard terminology, and we should probably keep it that way.

In fall 2019, we renamed the replacer API to have Pin() and Unpin() functions. This led to some student confusion; they were mixing up pinning and unpinning of replacer frames and buffer pool pages.

We should think about better names or a better API for the replacer.

lmwnshn avatar Sep 24 '19 20:09 lmwnshn

The names pin() and unpin() are actually good.

What really leads to the confusion is the documentation of the contracts of these APIs. It's unclear from the docs of a Replacer interface should the pin() track a pinned page. Class homework provides relatively clear API spec yet it does not feel very natural that pin() is a remove operation by semantics and unpin() as an idempotent addition.

sitano avatar Oct 15 '19 14:10 sitano

There is also a typo in a doc on victim method. It can't return nullptr:

  • @param[out] frame_id id of frame that was removed, nullptr if no victim was found

Also as this is C++ and not C, it could return a tuple (frame_id_t, bool) or (Option<frame_id_t>) instead of using [out] *ptr as in pure C.

sitano avatar Oct 15 '19 14:10 sitano

In my opinion, Unpin and Pin APIs should be complementary. So, Unpin should not be idempotent. Otherwise, it causes source of bugs where we are unpinning a page which was never pinned or we forgot to pin a page.

In current requirements, i can't put assert in unpin page according to this test line

ashishnegi avatar Sep 21 '20 23:09 ashishnegi

2022 fall's LRU-K should be less confusing.

skyzh avatar Sep 16 '22 04:09 skyzh