You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by saketj <gi...@git.apache.org> on 2016/09/21 09:59:44 UTC
[GitHub] incubator-quickstep pull request #101: Optimize PackedRowStoreValueAccessor ...
GitHub user saketj opened a pull request:
https://github.com/apache/incubator-quickstep/pull/101
Optimize PackedRowStoreValueAccessor & BasicColumnStoreValueAccessor by removing redundant computations and clearly exposing a strided memory access pattern
This PR proposes to optimize the way value accessors are currently used to read values from a storage block. Code profiling revealed that almost 10%-15% of the execution time was being spent in making redundant method calls to CatalogRelationSchema to find out properties that are anyway always constant when iterating over a given column. A simple caching of these values could give significant improvements. Moreover, during code analysis it was found that even more opportunities for compiler induced optimization and vectorization exist, if a simple semantics of a strided memory access pattern is exposed to the compiler & the runtime. After all, most iterations over a column in a relation can be expressed by as simple equation as: `base_address + tuple_id * offset`, where base_address and offset are constant. Exposing this semantics opens a whole lot of interesting opportunities, which otherwise have been obscured in the existing design of ValueAccessors. Rewriting the entire set o
f ValueAccessors is a mammoth task. Therefore, this PR and few subsequent PRs will propose to do this incrementally, always making sure that each PR addresses end-to-end functionality for some subset of features.
This PR specifically optimizes PackedRowStoreValueAccessor & BasicColumnStoreValueAccessor only. To demonstrate the end-to-end benefit of this optimization, predicate evaluation involving relational operators, regular expressions & literal comparison have been refactored to be cognizant of such optimizations.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/saketj/incubator-quickstep optimize-value-accessor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/101.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #101
----
commit 88f810c0eac4a248a81a7cca614389cfd94aa1dd
Author: Saket Saurabh <ss...@cs.wisc.edu>
Date: 2016-09-21T08:17:19Z
Optimize PackedRowStoreValueAccessor & BasicColumnStoreValueAccessor by removing redundant computations and clearly exposing a strided memory access pattern
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #101: Optimize PackedRowStoreValueAccessor & Basic...
Posted by saketj <gi...@git.apache.org>.
Github user saketj commented on the issue:
https://github.com/apache/incubator-quickstep/pull/101
@navsan Could you please review?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #101: Optimize PackedRowStoreValueAccessor & Basic...
Posted by pateljm <gi...@git.apache.org>.
Github user pateljm commented on the issue:
https://github.com/apache/incubator-quickstep/pull/101
LGTM. Merging. Thanks @saketj.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #101: Optimize PackedRowStoreValueAccessor & Basic...
Posted by saketj <gi...@git.apache.org>.
Github user saketj commented on the issue:
https://github.com/apache/incubator-quickstep/pull/101
It is strange that only one of the Travis build tasks for [clang++ compiler, build=release and vector_copy_ellision_level=selection] is repeatedly failing due to a compiler error. (Reproduced below) .
The same code compiles perfectly for clang++ release (vector_copy_ellision_level=joinwithbinaryexpression) and for all configurations of g++.
I don't think it is related to changes of current PR. I tried running it multiple times, but still the same error. Does anyone else has also faced the same issue before, or might know what is causing it?
> Building CXX object types/operations/comparisons/CMakeFiles/quickstep_types_operations_comparisons_GreaterComparison.dir/GreaterComparison.cpp.o
error: unable to execute command: Killed
clang: error: clang frontend command failed due to signal (use -v to see invocation)
Ubuntu clang version 3.7.1-svn253571-1~exp1 (branches/release_37) (based on LLVM 3.7.1)
Target: x86_64-pc-linux-gnu
Thread model: posix
clang: note: diagnostic msg: PLEASE submit a bug report to http://bugs.debian.org/ and include the crash backtrace, preprocessed source, and associated run script.
clang: note: diagnostic msg:
PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang: note: diagnostic msg: /tmp/EqualComparison-835ea0.cpp
clang: note: diagnostic msg: /tmp/EqualComparison-835ea0.sh
clang: note: diagnostic msg:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #101: Optimize PackedRowStoreValueAccessor & Basic...
Posted by pateljm <gi...@git.apache.org>.
Github user pateljm commented on the issue:
https://github.com/apache/incubator-quickstep/pull/101
@saketj Can you restart Travis to make sure the tests come back green.
@navsan If this looks good to you, can we close this?
Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #101: Optimize PackedRowStoreValueAccessor & Basic...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/101
@saketj Next time, please squash all the commits into one before merging. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #101: Optimize PackedRowStoreValueAccessor & Basic...
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on the issue:
https://github.com/apache/incubator-quickstep/pull/101
The compiler is probably running out of memory. Can you try turning down
the level of parallelism?
On Mon, Oct 3, 2016, 08:48 Saket Saurabh <no...@github.com> wrote:
> It is strange that only one of the Travis build tasks for [clang++
> compiler, build=release and vector_copy_ellision_level=selection] is
> repeatedly failing due to a compiler error. (Reproduced below) .
>
> The same code compiles perfectly for clang++ release
> (vector_copy_ellision_level=joinwithbinaryexpression) and for all
> configurations of g++.
>
> I don't think it is related to changes of current PR. I tried running it
> multiple times, but still the same error. Does anyone else has also faced
> the same issue before, or might know what is causing it?
>
> Building CXX object
> types/operations/comparisons/CMakeFiles/quickstep_types_operations_comparisons_GreaterComparison.dir/GreaterComparison.cpp.o
> error: unable to execute command: Killed
> clang: error: clang frontend command failed due to signal (use -v to see
> invocation)
> Ubuntu clang version 3.7.1-svn253571-1~exp1 (branches/release_37) (based
> on LLVM 3.7.1)
> Target: x86_64-pc-linux-gnu
> Thread model: posix
> clang: note: diagnostic msg: PLEASE submit a bug report to
> http://bugs.debian.org/ and include the crash backtrace, preprocessed
> source, and associated run script.
> clang: note: diagnostic msg:
> PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
> Preprocessed source(s) and associated run script(s) are located at:
> clang: note: diagnostic msg: /tmp/EqualComparison-835ea0.cpp
> clang: note: diagnostic msg: /tmp/EqualComparison-835ea0.sh
> clang: note: diagnostic msg:
>
> \u2014
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/incubator-quickstep/pull/101#issuecomment-251143502>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ACZB64ofKH1SUORDDVv1CF96p_rYXoTWks5qwSPBgaJpZM4KCnYW>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #101: Optimize PackedRowStoreValueAccessor & Basic...
Posted by saketj <gi...@git.apache.org>.
Github user saketj commented on the issue:
https://github.com/apache/incubator-quickstep/pull/101
@zuyu Thanks, sure will do that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #101: Optimize PackedRowStoreValueAccessor ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/101
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #101: Optimize PackedRowStoreValueAccessor & Basic...
Posted by saketj <gi...@git.apache.org>.
Github user saketj commented on the issue:
https://github.com/apache/incubator-quickstep/pull/101
@pateljm Thanks Jignesh. Have updated the patch and also added the performance numbers in the PR description.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---