Skip to content
This repository was archived by the owner on Feb 20, 2023. It is now read-only.

Compilation Manager with Shared Pointer Approach #1402

Closed

Conversation

wuwenw
Copy link
Contributor

@wuwenw wuwenw commented Dec 18, 2020


Compilation Manager with Shared Pointer Approach

Description

This PR enables the adaptive exeuction mode and fixes the legacy issue that the TPL interpretation finishes before compilation is done, which leads to the aceess of invalid memory region. We have discussed several solutions, including copying modules, maintaing status map, transferring memory ownership, registering additional TBB task and using deferred event framework. The engineering/performance overhead of the above approaches is too much. At the end, we feel the necessity of using shared pointers though they're blacklisted.

The logic for shared pointers is pretty simple. The module of each frame and the context_region of an exetuable query are now shared_ptr (actually SanctionedSharedPtr) instead of unique_ptr. The main execution thread is responsible to create them, and copy the shared pointer to each compilation task. The shared pointers wil be destroyed only when the count == 0, meaning the interpretation and all compilation tasks have finished. The pointees are essentially owned by all referrers. This is a good situation for shared pointers because we believe multiple threads need the ownership of the same instance at some point. There are no cycles in our use case.

These two paragraphs mainly response to sanctions proposed in #1386. This PR has passed simple End-to-End tests.

Remaining tasks

  • Robust tests
  • Benchmark
  • Documentation
  • Git issues for shared pointer/further improvement

Further work

We need to collect the compilation metrics for the adaptive mode. We are not using the compilation class at the point, but we can modify it to have parameters of optimization options for each compilation task.

As Matt has proposed, eventually we need to introduce a consumer at the end of the pipeline - the cache framework. With this consumer, we may want to revisit the life-cycle of executbale query/modules.

@wuwenw wuwenw added the in-progress This PR is being actively worked on and not ready to be reviewed or merged. Mark PRs with this. label Dec 18, 2020
Copy link
Member

@apavlo apavlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix header.

@apavlo apavlo requested a review from kjobanputra December 22, 2020 20:07
Copy link
Contributor

@lmwnshn lmwnshn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the documentation, please do not approve this PR for merging until the following documentation has been addressed for all SanctionedSharedPtr usages:

See the class comment for https://github.com/cmu-db/noisepage/blob/master/src/include/common/sanctioned_shared_pointer.h

 * SanctionedSharedPtr exists to declare at a code-level that
 *   1. I have thought deeply about ownership:
 *      - Who owns the pointee?
 *      - When is the pointee created?
 *      - When should the pointee be freed?
 *   2. I have thought deeply about memory management:
 *      - I am not creating a shared pointer to "fix a memory leak" by hiding the leak under the carpet.
 *      - I am creating a shared pointer because I genuinely believe that two or more objects need ownership of the
 *        memory at the same instance in time.
 *      - I certify that there are no cycles of shared pointers.
 *   3. I am convinced that there is literally no other possible way to achieve my task except with a shared pointer.
 *

@lmwnshn lmwnshn added the do-not-merge This PR is either not intended to merge, or did not pass review. label Dec 22, 2020
Copy link
Contributor

@kjobanputra kjobanputra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spoke with Wuwen a while back, but it might be nice to put down what we discussed as the pipeline of actions:

  1. The current implementation doesn't use the compilation manager class. It is simply a fix within the framework that Prasanth built out. We'll run OLTP bench with the existing implementation, just to make sure that everything is working. We'll also make sure that the JUnit tests are working.
  2. We'll move the shared pointer approach to the compilation manager class, as it would be nice to have for the future when we are considering caching compiled queries / managing scheduling different compilation passes for queries.
  3. We'll again test and benchmark with JUnit and OLTP bench.

We're currently trying to figure out how to run OLTP bench.

wuwenw added 3 commits April 4, 2021 21:24
…shared-ptr

# Conflicts:
#	script/testing/meta/util/test_db_server.py
#	script/testing/util/db_server.py
@wuwenw wuwenw added ready-for-ci Indicate that this build should be run through CI. and removed do-not-merge This PR is either not intended to merge, or did not pass review. labels Apr 10, 2021
@lmwnshn lmwnshn removed the ready-for-ci Indicate that this build should be run through CI. label Apr 15, 2021
@apavlo
Copy link
Member

apavlo commented May 25, 2021

This is dead

@apavlo apavlo closed this May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
in-progress This PR is being actively worked on and not ready to be reviewed or merged. Mark PRs with this.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants