Repository: ClickHouse/ClickHouse
File: src/IO/CachedInMemoryReadBufferFromFile.cpp (C++)

Code before:
    , inner_read_until_position(read_until_position)
{
    cache_key.offset = 0;
}

String CachedInMemoryReadBufferFromFile::getFileName() const
{
    return cache_key.path;
}

String CachedInMemoryReadBufferFromFile::getInfoForLog()
{
    return "CachedInMemoryReadBufferFromFile(" + in->getInfoForLog() + ")";
}

bool CachedInMemoryReadBufferFromFile::isSeekCheap()
{
    /// If working buffer is empty then the next nextImpl() call will have to send a new read
    /// request anyway, seeking doesn't make it more expensive.
    return available() == 0 || last_read_hit_cache;
}

bool CachedInMemoryReadBufferFromFile::isContentCached(size_t offset, size_t /*size*/)
{
    /// There's a tradeoff here: we waste time doing the cache lookup twice (here, then in nextImpl),
    /// but save ThreadPoolRemoteFSReader the overhead of passing the task to another thread and back.
    /// Is this net faster? On the one test I tried it gave a small speedup.
    /// Would probably be better to change this API to actually perform a read if it's a cache hit,
    /// then we wouldn't need to look up twice.
    PageCacheKey temp_key = cache_key;
    temp_key.offset = offset / block_size * block_size;
    temp_key.size = std::min(block_size, file_size.value() - temp_key.offset);
    return cache->contains(temp_key, settings.page_cache_inject_eviction);
}

off_t CachedInMemoryReadBufferFromFile::seek(off_t off, int whence)
{
    if (whence != SEEK_SET)
        throw Exception(ErrorCodes::CANNOT_SEEK_THROUGH_FILE, "Only SEEK_SET mode is allowed.");

    size_t offset = static_cast<size_t>(off);
    if (offset > file_size.value())
        throw Exception(ErrorCodes::SEEK_POSITION_OUT_OF_BOUND, "Seek position is out of bounds. Offset: {}", off);

    if (offset >= file_offset_of_buffer_end - working_buffer.size() && offset <= file_offset_of_buffer_end)
    {
        pos = working_buffer.end() - (file_offset_of_buffer_end - offset);
        chassert(getPosition() == off);
        return off;
    }


Review comment (performance):
About double lookup, in fs cache there is similar code and there in `isContentCached` we do `cache->getOrSet` (not just `cache->contains`) and save the result inside the class and then use it on next `nextImpl`. Probably easy to do the same here? Anyway, just `contains()` would suffice for now probably.

Code after:
    PageCacheKey cache_key_, PageCachePtr cache_, std::unique_ptr<ReadBufferFromFileBase> in_, const ReadSettings & settings_)
    : ReadBufferFromFileBase(0, nullptr, 0, in_->getFileSize()), cache_key(cache_key_), cache(cache_)
    , settings(settings_)
    , in(std::move(in_)), read_until_position(file_size.value())
    , inner_read_until_position(read_until_position)
{
    cache_key.offset = 0;
}

String CachedInMemoryReadBufferFromFile::getFileName() const
{
    return cache_key.path;
}

String CachedInMemoryReadBufferFromFile::getInfoForLog()
{
    return "CachedInMemoryReadBufferFromFile(" + in->getInfoForLog() + ")";
}

bool CachedInMemoryReadBufferFromFile::isSeekCheap()
{
    /// Seek is cheap in the sense that seek()+nextImpl() is never much slower than ignore()+nextImpl()
    /// (which is what the caller cares about).
    return true;
}

off_t CachedInMemoryReadBufferFromFile::seek(off_t off, int whence)
{
    if (whence != SEEK_SET)
        throw Exception(ErrorCodes::CANNOT_SEEK_THROUGH_FILE, "Only SEEK_SET mode is allowed.");

    size_t offset = static_cast<size_t>(off);
    if (offset > file_size.value())
        throw Exception(ErrorCodes::SEEK_POSITION_OUT_OF_BOUND, "Seek position is out of bounds. Offset: {}", off);

    if (offset >= file_offset_of_buffer_end - working_buffer.size() && offset <= file_offset_of_buffer_end)
    {
        pos = working_buffer.end() - (file_offset_of_buffer_end - offset);
        chassert(getPosition() == off);
        return off;
    }

    resetWorkingBuffer();

    file_offset_of_buffer_end = offset;
    chunk.reset();

    chassert(getPosition() == off);
    return off;
}