[jira] [Resolved] (IMPALA-6290) Simplify ScannerContext buffer management to only use one I/O buffer at a time.

["Tim Armstrong (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/IMPALA-6290?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Tim Armstrong resolved IMPALA-6290.
-----------------------------------
       Resolution: Fixed
    Fix Version/s: Impala 2.12.0

IMPALA-6290: limit ScannerContext to 1 buffer at a time

This is a prerequisite for constraining the number of buffers per scan
range. Before this patch, calling ReadBytes(), SkipBytes(), etc could
cause an arbitrary number of I/O buffers to accumulate in
'completed_io_buffers_'. E.g. if we allocated 3 * 8MB I/O buffers for
a range and then called ReadBytes(30MB), we would hit resource
exhaustion as soon as 3 buffers were accumulated in
'completed_io_buffers_'.

The fix is to avoid accumulating any buffers in 'completed_io_buffers_'.
Instead of adding them to 'completed_io_buffers_', completed buffers
are just returned to the I/O manager. It turned out that this did not
weaken the ScannerContext's guarantees about memory lifetime, because
ScannerContext::GetBytesInternal() cleared 'boundary_buffer_' each
time it was called regardless. I checked that this behaviour wasn't
a bug by inspecting the scanner code. I could not find any cases
where scanners depended on returned memory remaining valid beyond
the next Read*()/Get*()/Skip*() call on the stream.

This change makes that lifetime explicit in the comments. A
side-effect of this fix is that scanners do not need to call
ReleaseCompletedResources() in CommitRows() and means that the
ScannerContext only ever needs to hold one I/O buffer at a time.

This change also reimplements SkipBytes() to avoid it accumulating
memory in the boundary buffer for large skip sizes.

Also clarifies some of the invariants in ScannerContext. E.g. some
places assumed io_buffer_ != NULL, but that is no longer needed.

Testing:
Ran core tests with ASAN and exhaustive tests with DEBUG.

Change-Id: I74c5960a75f7d88b0e1de4199af731fb13e592f0
Reviewed-on: http://gerrit.cloudera.org:8080/8814
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins


> Simplify ScannerContext buffer management to only use one I/O buffer at a time.
> -------------------------------------------------------------------------------
>
>                 Key: IMPALA-6290
>                 URL: https://issues.apache.org/jira/browse/IMPALA-6290
>             Project: IMPALA
>          Issue Type: Sub-task
>          Components: Backend
>            Reporter: Tim Armstrong
>            Assignee: Tim Armstrong
>              Labels: resource-management
>             Fix For: Impala 2.12.0
>
>
> I'm doing this as part of the HDFS buffer management work but splitting it out as a subtask since it's a logically independent change.
> ScannerContext currently depends on the scanners calling ReleaseCompletedResources() repeatedly to free up buffers. Currently this works ok, but if we add a hard constraint to the number of I/O buffers, then we could hit resource exhaustion if we scan too far ahead without calling ReleaseCompletedResources(). E.g. if we have 3 * 8MB I/O buffers to use and try to scan 25MB before calling ReleaseCompletedResources(), we end up in a state where all I/O buffers are sitting in the ScannerContext.
> Certain ScannerContext operations also can exhaust the I/O buffers no matter how frequently ReleaseCompletedResources() is called. E.g. ReadBytes(25MB) or SkipBytes(25MB) would run into that problem with the current implementation.
> I spent some time looking at the ScannerContext API and the calling patterns of the scanners and came to the conclusion that there's no requirement for us to accumulate buffers in completed_io_buffers_ - after IMPALA-5307 we don't generally assume that the memory returned from previous calls remains valid when the read position from the stream is advanced.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)