Uploaded image for project: 'IMPALA'
  1. IMPALA
  2. IMPALA-2399

Cleanup/rethink QueryMaintenance() calls in the BE.

Details

    • Improvement
    • Status: Open
    • Minor
    • Resolution: Unresolved
    • Impala 2.3.0
    • None
    • Backend

    Description

      In our BE execution, we currently call QueryMaintenance() and check for cancellation very generously, e.g., at the beginning of every GetNext() or in tight loops that could run for a long time (e.g. due to selective predicates).

      Our current pattern of usage has several problems:
      1. QueryMaintenance() is expensive, but the function name is rather opaque to this fact. It traverses the mem tracker hierarchy checking for the mem limit, and also checks the query status under a spin lock. It is not clear that these checks need to be coupled and called so frequently. We should place these checks judiciously/appropriately because QueryMaintenance() can become a performance bottleneck especially in subplan execution.

      2. We always couple QueryMaintenance() with checking for cancellation. There is an existing TODO in the code to address this (see runtime-state.cc RuntimeState::CheckQueryState()).

      See these code reviews for additional discussion:
      http://gerrit.cloudera.org:8080/894
      http://gerrit.cloudera.org:8080/901

      Attachments

        Issue Links

          Activity

            dhecht Daniel Hecht added a comment -

            We should be able to get rid of the spin lock on this path altogether – Status is really just a single pointer. First we need to improve our Atomic implementation though because it's conservative on its used of locked instructions.

            dhecht Daniel Hecht added a comment - We should be able to get rid of the spin lock on this path altogether – Status is really just a single pointer. First we need to improve our Atomic implementation though because it's conservative on its used of locked instructions.
            kwho Michael Ho added a comment -

            This JIRA will be tackled via the following steps:

            1. Replace all call sites of MemPool::Allocate() with MemPool::TryAllocate() so memory limit checks will be synchronous.
            2. Get rid of all callers of RuntimeState::SetMemLimitExceeded(). All errors should be propagated.
            3. Get rid of memory limit check in QueryMaintenance()
            4. Renamed query_status_ to udf_status_ to be used by UDF to propagate errors

            The current callers of MemPool::Allocate() include:

            1. All scanners
            2. All table writers
            3. Compressor / Decompressor (mostly called from table writers and scanners).
            4. Exec nodes: top-n nodes, analytic eval nodes
            5. Old Hash tables, RawValue
            6. TupleRow, Tuple, RowBatch (to be tackled by IMPALA-2384)
            7. UDF, UDA (tackled by IMPALA-2620)

            The items in the list above will be converted one-by-one.

            kwho Michael Ho added a comment - This JIRA will be tackled via the following steps: Replace all call sites of MemPool::Allocate() with MemPool::TryAllocate() so memory limit checks will be synchronous. Get rid of all callers of RuntimeState::SetMemLimitExceeded(). All errors should be propagated. Get rid of memory limit check in QueryMaintenance() Renamed query_status_ to udf_status_ to be used by UDF to propagate errors The current callers of MemPool::Allocate() include: All scanners All table writers Compressor / Decompressor (mostly called from table writers and scanners). Exec nodes: top-n nodes, analytic eval nodes Old Hash tables, RawValue TupleRow, Tuple, RowBatch (to be tackled by IMPALA-2384 ) UDF, UDA (tackled by IMPALA-2620 ) The items in the list above will be converted one-by-one.
            kwho Michael Ho added a comment -

            Also need to convert most if not all callers of Consume() to TryConsume().

            kwho Michael Ho added a comment - Also need to convert most if not all callers of Consume() to TryConsume().
            tarmstrong Tim Armstrong added a comment -

            The commit "IMPALA-5844: use a MemPool for expr result allocations" reduced QueryMaintenance() to two lines of code and removed all overrides:

            Status ExecNode::QueryMaintenance(RuntimeState* state) {
              expr_results_pool_->Clear();
              return state->CheckQueryState();
            }
            

            I don't think this fundamentally changes the remaining steps in this JIRA.

            tarmstrong Tim Armstrong added a comment - The commit " IMPALA-5844 : use a MemPool for expr result allocations" reduced QueryMaintenance() to two lines of code and removed all overrides: Status ExecNode::QueryMaintenance(RuntimeState* state) { expr_results_pool_->Clear(); return state->CheckQueryState(); } I don't think this fundamentally changes the remaining steps in this JIRA.

            People

              Unassigned Unassigned
              alex.behm Alexander Behm
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

                Created:
                Updated: