Skip to content

DiskManager Performs Blocking IO #1637

Closed
Closed
DiskManager Performs Blocking IO#1637
Labels
enhancementNew feature or request

Description

@tustvold
tustvold
opened on Jan 21, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

DiskManager performs potentially blocking file IO despite most likely being called from a tokio worker thread. This is unfortunate, as it may stall out a worker thread.

Describe the solution you'd like

DiskManager should make use of tokio::fs::File or tokio::spawn_blocking to avoid blocking threads. This will likely involve making some of these methods async

Describe alternatives you've considered

The methods could document that they perform blocking IO, and punt the burden onto the user to handle this, but this seems unfortunate given Datafusion's existing coupling to tokio.

Additional context

May be related to #1636 which concerns the construction of RuntimeEnv, and may allow RuntimeEnv::new() to be async without also making ExecutionContext.

FYI @yjshen

Activity

  1. tustvold
    added
    enhancementNew feature or request
    on Jan 21, 2022

  2. alamb commented on Jan 21, 2022

    @alamb
    alamb
    on Jan 21, 2022
    Contributor

    Added as a worklist item on #587

  3. yjshen commented on Apr 22, 2022

    @yjshen
    yjshen
    on Apr 22, 2022
    Member

    This could be closed according to the discussions in #2226 Google Docs?

    @tustvold I'm suggesting just doing the sync disk IO in the same dedicated threadpool used for all query computation. In my benchmarks, I've not seen a compelling advantage to offloading the IO elsewhere as the loss of thread-locality hurts performance, the complexity cost is high, and frankly most realistic workloads are not close to being IO bound
    In the case of spilling to disk, one could make a convincing case that you're actually memory bound and doing something else concurrently would be actively detrimental...

    @houqp I see, that's certainly an option. I don't have a good intuition on whether this will a more optimal setup or not without some hands on benchmarks. My understanding is that to make this work well, we will need to create slightly more threads in the CPU thread pool to reduce core idleness caused by IO at the cost of slightly more context switches resulted from preemptive scheduling.
    I also don't like the complexity of async io, so it's good idea to start with something simple, then benchmark and iterate from there.

  4. tustvold commented on Apr 22, 2022

    @tustvold
    tustvold
    on Apr 22, 2022
    ContributorAuthor

    I would not look to implement async IO for the disk manager at this stage, although I'm not sure we have enough data to say definitively that this is not something we want to consider down the line. Whether that means we should close this ticket, idk...

    Fwiw given the use of flatbuffers, mmap might be something to explore also. I think currently we effectively double the memory on write/read in order to buffer the flatbuffer representation.

    Edit: could also use the row representation, as that could trivially be streamed to disk 🤔

    Edit edit: let's close and reopen if we decide to revisit this

  5. tustvold
    closed this as completedon Apr 22, 2022

  6. alamb commented on Apr 24, 2022

    @alamb
    alamb
    on Apr 24, 2022
    Contributor

    Edit: could also use the row representation, as that could trivially be streamed to disk 🤔

    This is the approach I would personally recommend. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions