You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/09/20 23:15:43 UTC
[GitHub] [arrow] augustoasilva opened a new pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache Object Code
augustoasilva opened a new pull request #11193:
URL: https://github.com/apache/arrow/pull/11193
Implement Gandiva to cache object code instead the entire llvm module, improving the memory consumption and LLVM time perfomance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r732332091
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -102,14 +46,30 @@ Status Filter::Make(SchemaPtr schema, ConditionPtr condition,
ARROW_RETURN_IF(configuration == nullptr,
Status::Invalid("Configuration cannot be null"));
- static Cache<FilterCacheKey, std::shared_ptr<Filter>> cache;
- FilterCacheKey cache_key(schema, configuration, *(condition.get()));
- auto cachedFilter = cache.GetModule(cache_key);
- if (cachedFilter != nullptr) {
- *filter = cachedFilter;
- return Status::OK();
+ std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ shared_cache = LLVMGenerator::GetCache();
+
+ Condition conditionToKey = *(condition.get());
+
+ ExpressionCacheKey cache_key(schema, configuration, conditionToKey, "filter");
+ std::unique_ptr<ExpressionCacheKey> base_cache_key =
+ std::make_unique<ExpressionCacheKey>(cache_key);
+ std::shared_ptr<ExpressionCacheKey> shared_base_cache_key = std::move(base_cache_key);
Review comment:
Changed GandivaObjectCache to take an object instead a shared_pointer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r731941082
##########
File path: cpp/src/gandiva/tests/filter_test.cc
##########
@@ -29,7 +29,11 @@ using arrow::int32;
class TestFilter : public ::testing::Test {
public:
- void SetUp() { pool_ = arrow::default_memory_pool(); }
+ void SetUp() {
+ pool_ = arrow::default_memory_pool();
+ // Setup arrow log severity threshold to debug level.
+ arrow::util::ArrowLog::StartArrowLog("", arrow::util::ArrowLogLevel::ARROW_DEBUG);
Review comment:
no. its fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714703047
##########
File path: cpp/src/gandiva/engine.h
##########
@@ -54,6 +54,13 @@ class GANDIVA_EXPORT Engine {
functions_to_compile_.push_back(fname);
}
+ /// Set BaseObjectCache.
+ template <class KeyType>
+ Status SetLLVMObjectCache(GandivaObjectCache<KeyType>& object_cache) {
+ execution_engine_->setObjectCache(&object_cache);
+ return Status::OK();
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714702340
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -78,6 +79,10 @@ std::string FilterCacheKey::ToString() const {
return ss.str();
}
+std::string FilterCacheKey::GetExpressionsAsString() const {
Review comment:
This function is to be used only in the base_cache_key so it also stores the expressions and compare them when comparing one base_cache_key against another.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713938227
##########
File path: cpp/src/gandiva/llvm_generator.h
##########
@@ -49,10 +51,48 @@ class GANDIVA_EXPORT LLVMGenerator {
static Status Make(std::shared_ptr<Configuration> config,
std::unique_ptr<LLVMGenerator>* llvm_generator);
+ static std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ GetCache();
+
/// \brief Build the code for the expression trees for default mode. Each
/// element in the vector represents an expression tree
Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode);
+ /// \brief Build the code for the expression trees for default mode with a LLVM
+ /// ObjectCache. Each element in the vector represents an expression tree
+ template <class KeyType>
+ Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode,
Review comment:
Ok. It looks like there is code duplication with the other build method. Should we remove the other build method if it is not used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713898020
##########
File path: cpp/src/gandiva/base_cache_key.h
##########
@@ -0,0 +1,79 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <arrow/util/hash_util.h>
+#include <stddef.h>
+
+#include "gandiva/expression.h"
+#include "gandiva/filter.h"
+#include "gandiva/projector.h"
+
+namespace gandiva {
+
+class BaseCacheKey {
+ public:
+ BaseCacheKey(ProjectorCacheKey& key, std::string type) : type_(type) {
+ static const int32_t kSeedValue = 4;
+ size_t key_hash = key.Hash();
+ size_t result_hash = kSeedValue;
+ arrow::internal::hash_combine(result_hash, type);
+ arrow::internal::hash_combine(result_hash, key_hash);
+ hash_code_ = result_hash;
+ schema_ = key.schema();
+ }
+
+ BaseCacheKey(FilterCacheKey& key, std::string type) : type_(type) {
+ static const size_t kSeedValue = 4;
+ size_t key_hash = key.Hash();
+ size_t result_hash = kSeedValue;
+ arrow::internal::hash_combine(result_hash, type);
+ arrow::internal::hash_combine(result_hash, key_hash);
+ hash_code_ = result_hash;
+ schema_ = key.schema();
+ }
+
+ size_t Hash() const { return hash_code_; }
+
+ std::string Type() const { return type_; }
+
+ bool operator==(const BaseCacheKey& other) const {
+ if (hash_code_ != other.hash_code_) {
Review comment:
Ok, I will also check the expressions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714693822
##########
File path: cpp/src/gandiva/base_cache_key.h
##########
@@ -0,0 +1,86 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <arrow/util/hash_util.h>
+#include <stddef.h>
+
+#include "gandiva/expression.h"
+#include "gandiva/filter.h"
+#include "gandiva/projector.h"
+
+namespace gandiva {
+
+class BaseCacheKey {
+ public:
+ BaseCacheKey(ProjectorCacheKey& key, std::string type) : type_(type) {
+ static const int32_t kSeedValue = 4;
+ size_t key_hash = key.Hash();
+ size_t result_hash = kSeedValue;
+ arrow::internal::hash_combine(result_hash, type);
+ arrow::internal::hash_combine(result_hash, key_hash);
+ hash_code_ = result_hash;
+ schema_ = key.schema();
+ expressions_as_string_ = key.GetExpressionsAsString();
+ }
+
+ BaseCacheKey(FilterCacheKey& key, std::string type) : type_(type) {
+ static const size_t kSeedValue = 4;
+ size_t key_hash = key.Hash();
+ size_t result_hash = kSeedValue;
+ arrow::internal::hash_combine(result_hash, type);
+ arrow::internal::hash_combine(result_hash, key_hash);
+ hash_code_ = result_hash;
+ schema_ = key.schema();
+ expressions_as_string_.push_back(key.GetExpressionsAsString());
+ }
+
+ size_t Hash() const { return hash_code_; }
+
+ std::string Type() const { return type_; }
+
+ bool operator==(const BaseCacheKey& other) const {
+ if (hash_code_ != other.hash_code_) {
+ return false;
+ }
+
+ if (!schema_->Equals(other.schema_)) {
+ return false;
+ }
+
+ if (type_ != other.type_) {
+ return false;
+ }
+
+ if (expressions_as_string_ != other.expressions_as_string_) {
+ return false;
+ }
+
+ return true;
+ }
+
+ bool operator!=(const BaseCacheKey& other) const { return !(*this == other); }
+
+ private:
+ uint64_t hash_code_;
Review comment:
> size_t
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713885595
##########
File path: cpp/src/gandiva/base_cache_key.h
##########
@@ -0,0 +1,79 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <arrow/util/hash_util.h>
+#include <stddef.h>
+
+#include "gandiva/expression.h"
+#include "gandiva/filter.h"
+#include "gandiva/projector.h"
+
+namespace gandiva {
+
+class BaseCacheKey {
+ public:
+ BaseCacheKey(ProjectorCacheKey& key, std::string type) : type_(type) {
+ static const int32_t kSeedValue = 4;
+ size_t key_hash = key.Hash();
+ size_t result_hash = kSeedValue;
+ arrow::internal::hash_combine(result_hash, type);
+ arrow::internal::hash_combine(result_hash, key_hash);
+ hash_code_ = result_hash;
+ schema_ = key.schema();
+ }
+
+ BaseCacheKey(FilterCacheKey& key, std::string type) : type_(type) {
+ static const size_t kSeedValue = 4;
+ size_t key_hash = key.Hash();
+ size_t result_hash = kSeedValue;
+ arrow::internal::hash_combine(result_hash, type);
+ arrow::internal::hash_combine(result_hash, key_hash);
+ hash_code_ = result_hash;
+ schema_ = key.schema();
+ }
+
+ size_t Hash() const { return hash_code_; }
+
+ std::string Type() const { return type_; }
+
+ bool operator==(const BaseCacheKey& other) const {
+ if (hash_code_ != other.hash_code_) {
Review comment:
There can still be collision on different expressions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r715206873
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -78,6 +79,10 @@ std::string FilterCacheKey::ToString() const {
return ss.str();
}
+std::string FilterCacheKey::GetExpressionsAsString() const {
Review comment:
There is no need for it anymore, as it were only used on the ExpressionCacheKey and now the ExpressionCacheKey is already built with the expressions from projector or filter, not the old ProjectorCacheKey or FilterCacheKey
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache Object Code
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713593711
##########
File path: cpp/src/gandiva/cache.h
##########
@@ -39,20 +39,30 @@ class Cache {
Cache() : Cache(GetCapacity()) {}
- ValueType GetModule(KeyType cache_key) {
+ ::std::shared_ptr<Cache> create(size_t capacity) {
+ return ::std::make_shared<Cache>(cache_(capacity));
+ }
+
+ ValueType GetObjectCode(KeyType cache_key) {
arrow::util::optional<ValueCacheObject<ValueType>> result;
mtx_.lock();
- result = cache_.get(cache_key);
+ result = cache_.GetObjectCode(cache_key);
mtx_.unlock();
return result != arrow::util::nullopt ? (*result).module : nullptr;
}
- void PutModule(KeyType cache_key, ValueCacheObject<ValueType> valueCacheObject) {
+ void PutObjectCode(KeyType& cache_key, ValueCacheObject<ValueType> object_code) {
mtx_.lock();
- cache_.insert(cache_key, valueCacheObject);
+ cache_.InsertObjectCode(cache_key, object_code);
mtx_.unlock();
}
+ ::std::shared_ptr<Cache> CreateSharedCachePtr() { return Cache::create(); }
Review comment:
This looks not needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713832526
##########
File path: cpp/src/gandiva/base_cache_key.h
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <arrow/util/hash_util.h>
+#include <stddef.h>
+
+#include "gandiva/expression.h"
+#include "gandiva/filter.h"
+#include "gandiva/projector.h"
+
+namespace gandiva {
+
+class BaseCacheKey {
+ public:
+ BaseCacheKey(ProjectorCacheKey& key, std::string type) : type_(type) {
+ static const int32_t kSeedValue = 4;
+ size_t key_hash = key.Hash();
+ size_t result_hash = kSeedValue;
+ arrow::internal::hash_combine(result_hash, type);
+ arrow::internal::hash_combine(result_hash, key_hash);
+ hash_code_ = result_hash;
+ schema_ = key.schema();
+ }
+
+ BaseCacheKey(FilterCacheKey& key, std::string type) : type_(type) {
+ static const size_t kSeedValue = 4;
+ size_t key_hash = key.Hash();
+ size_t result_hash = kSeedValue;
+ arrow::internal::hash_combine(result_hash, type);
+ arrow::internal::hash_combine(result_hash, key_hash);
+ hash_code_ = result_hash;
+ schema_ = key.schema();
+ }
+
+ size_t Hash() const { return hash_code_; }
+
+ std::string Type() const { return type_; }
+
+ bool operator==(const BaseCacheKey& other) const {
+ if (hash_code_ != other.hash_code_) {
Review comment:
Ok, I compared the other private variables to.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714684869
##########
File path: cpp/src/gandiva/greedy_dual_size_cache.h
##########
@@ -17,6 +17,8 @@
#pragma once
+#include <llvm/ADT/SmallString.h>
Review comment:
Ok, I've used the "Optmize imports" from Clion and it did not remove it before, so I removed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714357412
##########
File path: cpp/src/gandiva/llvm_generator.h
##########
@@ -49,10 +51,48 @@ class GANDIVA_EXPORT LLVMGenerator {
static Status Make(std::shared_ptr<Configuration> config,
std::unique_ptr<LLVMGenerator>* llvm_generator);
+ static std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ GetCache();
+
/// \brief Build the code for the expression trees for default mode. Each
/// element in the vector represents an expression tree
Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode);
+ /// \brief Build the code for the expression trees for default mode with a LLVM
+ /// ObjectCache. Each element in the vector represents an expression tree
+ template <class KeyType>
+ Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode,
+ GandivaObjectCache<KeyType>& obj_cache) {
+ selection_vector_mode_ = mode;
+
+ // Start measuring code gen time
+ auto begin = std::chrono::high_resolution_clock::now();
+ for (auto& expr : exprs) {
+ auto output = annotator_.AddOutputFieldDescriptor(expr->result());
+ ARROW_RETURN_NOT_OK(Add(expr, output));
+ }
+
+ // Stop measuring time, calculate the elapsed time and pass it to object cache
+ auto end = std::chrono::high_resolution_clock::now();
+ size_t elapsed =
Review comment:
Sorry, it seemed right. But I fixed with your suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713843095
##########
File path: cpp/src/gandiva/projector.h
##########
@@ -138,6 +167,7 @@ class GANDIVA_EXPORT Projector {
SchemaPtr schema_;
FieldVector output_fields_;
std::shared_ptr<Configuration> configuration_;
+ bool compiled_from_cache_;
Review comment:
built_from_cache sounds better, so changed to it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713888853
##########
File path: cpp/src/gandiva/engine.h
##########
@@ -54,6 +54,13 @@ class GANDIVA_EXPORT Engine {
functions_to_compile_.push_back(fname);
}
+ /// Set BaseObjectCache.
+ template <class KeyType>
+ Status SetLLVMObjectCache(GandivaObjectCache<KeyType>& object_cache) {
+ execution_engine_->setObjectCache(&object_cache);
+ return Status::OK();
Review comment:
why did you remove the error detection code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714351811
##########
File path: cpp/src/gandiva/engine.h
##########
@@ -54,6 +54,13 @@ class GANDIVA_EXPORT Engine {
functions_to_compile_.push_back(fname);
}
+ /// Set BaseObjectCache.
+ template <class KeyType>
+ Status SetLLVMObjectCache(GandivaObjectCache<KeyType>& object_cache) {
+ execution_engine_->setObjectCache(&object_cache);
+ return Status::OK();
Review comment:
Ok, so I will just leave it withouht the error checking for compability with older versions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r761163468
##########
File path: cpp/src/gandiva/cache.h
##########
@@ -39,17 +39,17 @@ class Cache {
Cache() : Cache(GetCapacity()) {}
- ValueType GetModule(KeyType cache_key) {
+ ValueType GetObjectCode(KeyType cache_key) {
arrow::util::optional<ValueCacheObject<ValueType>> result;
mtx_.lock();
result = cache_.get(cache_key);
mtx_.unlock();
return result != arrow::util::nullopt ? (*result).module : nullptr;
}
- void PutModule(KeyType cache_key, ValueCacheObject<ValueType> valueCacheObject) {
+ void PutObjectCode(KeyType& cache_key, ValueCacheObject<ValueType> object_code) {
Review comment:
pass the cacheobject also as reference
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714351975
##########
File path: cpp/src/gandiva/tests/projector_test.cc
##########
@@ -196,21 +202,23 @@ TEST_F(TestProjector, TestProjectCacheDecimalCast) {
auto expr0 = TreeExprBuilder::MakeExpression("castDECIMAL", {field_float64}, res_31_13);
std::shared_ptr<Projector> projector0;
ASSERT_OK(Projector::Make(schema, {expr0}, TestConfiguration(), &projector0));
+ EXPECT_FALSE(projector0->GetBuiltFromCache());
// if the output scale is different, the cache can't be used.
auto res_31_14 = field("result", arrow::decimal(31, 14));
auto expr1 = TreeExprBuilder::MakeExpression("castDECIMAL", {field_float64}, res_31_14);
std::shared_ptr<Projector> projector1;
ASSERT_OK(Projector::Make(schema, {expr1}, TestConfiguration(), &projector1));
- EXPECT_NE(projector0.get(), projector1.get());
+ // EXPECT_NE(projector0.get(), projector1.get()); -> old expect.
Review comment:
Removed it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713848639
##########
File path: cpp/src/gandiva/engine.cc
##########
@@ -167,7 +167,6 @@ Status Engine::Make(const std::shared_ptr<Configuration>& conf,
return Status::CodeGenError("Could not instantiate llvm::ExecutionEngine: ",
builder_error);
}
-
Review comment:
Removed it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713840682
##########
File path: cpp/src/gandiva/llvm_generator.cc
##########
@@ -50,6 +51,23 @@ Status LLVMGenerator::Make(std::shared_ptr<Configuration> config,
return Status::OK();
}
+std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+LLVMGenerator::GetCache() {
+ static Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>> cache;
+ // static std::unique_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ // cache_unique = std::make_unique<Cache<BaseCacheKey,
+ // std::shared_ptr<llvm::MemoryBuffer>>>();
+
+ // static std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ // shared_cache = std::move(cache_unique);
Review comment:
Removed it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r715204877
##########
File path: cpp/src/gandiva/projector.h
##########
@@ -34,6 +34,34 @@ namespace gandiva {
class LLVMGenerator;
+class ProjectorCacheKey {
Review comment:
Done, now there is only the ExpressionCacheKey
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714701296
##########
File path: cpp/src/gandiva/cache.cc
##########
@@ -20,10 +20,10 @@
namespace gandiva {
-static const int DEFAULT_CACHE_SIZE = 500;
+static const size_t DEFAULT_CACHE_SIZE = 128 * 1024 * 1024; // 256 MiB
-int GetCapacity() {
- int capacity;
+size_t GetCapacity() {
+ size_t capacity;
const char* env_cache_size = std::getenv("GANDIVA_CACHE_SIZE");
if (env_cache_size != nullptr) {
capacity = std::atoi(env_cache_size);
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713833611
##########
File path: cpp/src/gandiva/greedy_dual_size_cache.h
##########
@@ -146,9 +159,25 @@ class GreedyDualSizeCache {
priority_set_.erase(i);
}
+ void evictObject() {
Review comment:
Yeah, it was so and I should have removed it earlier, so I removed it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#issuecomment-985454352
Benchmark runs are scheduled for baseline = a6c81e621b637479a5828c4bdb95ec442ad94bb5 and contender = 188c94a9820ef3377573b835c9dc9dcb96ceafe4. 188c94a9820ef3377573b835c9dc9dcb96ceafe4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/f0dc01b023534d96b0bac4d36ea14c3e...5b059cee5d844774aae1f958873691eb/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/602e996a876949659442f3c145b84f06...f1aa0fd8f0a140058010e12f7a096f3f/)
[Finished :arrow_down:0.22% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/5c7ec03f40ac4f25a089b656edc6d4b8...721a52173d9848eb8dbecfa19fe61d4d/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714701520
##########
File path: cpp/src/gandiva/cache.cc
##########
@@ -20,10 +20,10 @@
namespace gandiva {
-static const int DEFAULT_CACHE_SIZE = 500;
+static const size_t DEFAULT_CACHE_SIZE = 128 * 1024 * 1024; // 256 MiB
-int GetCapacity() {
- int capacity;
+size_t GetCapacity() {
+ size_t capacity;
const char* env_cache_size = std::getenv("GANDIVA_CACHE_SIZE");
Review comment:
Makes sense, changed to it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714347477
##########
File path: cpp/src/gandiva/llvm_generator.h
##########
@@ -49,10 +51,48 @@ class GANDIVA_EXPORT LLVMGenerator {
static Status Make(std::shared_ptr<Configuration> config,
std::unique_ptr<LLVMGenerator>* llvm_generator);
+ static std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ GetCache();
+
/// \brief Build the code for the expression trees for default mode. Each
/// element in the vector represents an expression tree
Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode);
+ /// \brief Build the code for the expression trees for default mode with a LLVM
+ /// ObjectCache. Each element in the vector represents an expression tree
+ template <class KeyType>
+ Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode,
Review comment:
Yes, I safely removed the duplicated code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714681418
##########
File path: cpp/src/gandiva/greedy_dual_size_cache.h
##########
@@ -17,6 +17,8 @@
#pragma once
+#include <llvm/ADT/SmallString.h>
Review comment:
this is not required
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714681261
##########
File path: cpp/src/gandiva/greedy_dual_size_cache.h
##########
@@ -133,6 +138,14 @@ class GreedyDualSizeCache {
priority_set_.clear();
}
+ size_t GetCacheSize() { return cache_size_; }
+
+ std::string ToString() {
Review comment:
I see its called after buildProject/buildFilter. Its not really required. Lets remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r733758362
##########
File path: cpp/src/gandiva/projector.cc
##########
@@ -174,13 +93,11 @@ Status Projector::Make(SchemaPtr schema, const ExpressionVector& exprs,
ARROW_RETURN_NOT_OK(expr_validator.Validate(expr));
}
- // Start measuring build time
- auto begin = std::chrono::high_resolution_clock::now();
- ARROW_RETURN_NOT_OK(llvm_gen->Build(exprs, selection_vector_mode));
- // Stop measuring time and calculate the elapsed time
- auto end = std::chrono::high_resolution_clock::now();
- auto elapsed =
- std::chrono::duration_cast<std::chrono::milliseconds>(end - begin).count();
+ // Set the object cache for LLVM
+ llvm_gen->SetLLVMObjectCache(obj_cache);
Review comment:
Does this overall logic work in multithreaded scenario?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r732339817
##########
File path: cpp/src/gandiva/llvm_generator.h
##########
@@ -240,7 +269,7 @@ class GANDIVA_EXPORT LLVMGenerator {
void AddTrace(const std::string& msg, llvm::Value* value = NULLPTR);
std::unique_ptr<Engine> engine_;
- std::vector<std::unique_ptr<CompiledExpr>> compiled_exprs_;
+ std::vector<std::shared_ptr<CompiledExpr>> compiled_exprs_;
Review comment:
It was for a test for a while, so changed back to unique_ptr
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r732333013
##########
File path: cpp/src/gandiva/gandiva_object_cache.h
##########
@@ -0,0 +1,86 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#if defined(_MSC_VER)
+#pragma warning(push)
+#pragma warning(disable : 4244)
+#pragma warning(disable : 4141)
+#pragma warning(disable : 4146)
+#pragma warning(disable : 4267)
+#pragma warning(disable : 4624)
+#endif
+
+#include <llvm/ExecutionEngine/ObjectCache.h>
+#include <llvm/Support/MemoryBuffer.h>
+
+#include <chrono>
+
+#include "gandiva/cache.h"
+
+namespace gandiva {
+/// Class that enables the LLVM to use a custom rule to deal with the object code.
+template <class CacheKey>
+class GandivaObjectCache : public llvm::ObjectCache {
+ public:
+ GandivaObjectCache(
+ std::shared_ptr<Cache<CacheKey, std::shared_ptr<llvm::MemoryBuffer>>>& cache,
+ std::shared_ptr<CacheKey>& key) {
+ cache_ = cache;
+ cache_key_ = key;
+ // Start measuring code gen time
+ begin_time_ = std::chrono::high_resolution_clock::now();
+ }
+
+ ~GandivaObjectCache() {}
+
+ void notifyObjectCompiled(const llvm::Module* M, llvm::MemoryBufferRef Obj) {
+ // Stop measuring time and calculate the elapsed time to compile the object code
+ auto end_time = std::chrono::high_resolution_clock::now();
+ auto elapsed_time =
+ std::chrono::duration_cast<std::chrono::milliseconds>(end_time - begin_time_)
+ .count();
+
+ std::unique_ptr<llvm::MemoryBuffer> obj_buffer =
+ llvm::MemoryBuffer::getMemBufferCopy(Obj.getBuffer(), Obj.getBufferIdentifier());
+ std::shared_ptr<llvm::MemoryBuffer> obj_code = std::move(obj_buffer);
+
+ ValueCacheObject<std::shared_ptr<llvm::MemoryBuffer>> value_cache(
+ obj_code, elapsed_time, obj_code->getBufferSize());
+
+ cache_->PutObjectCode(*cache_key_.get(), value_cache);
+ }
+
+ std::unique_ptr<llvm::MemoryBuffer> getObject(const llvm::Module* M) {
+ std::shared_ptr<llvm::MemoryBuffer> cached_obj =
+ cache_->GetObjectCode(*cache_key_.get());
+ auto null = std::nullptr_t();
+ if (cached_obj != null) {
Review comment:
Ok, fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713854557
##########
File path: cpp/src/gandiva/llvm_generator.h
##########
@@ -49,10 +51,48 @@ class GANDIVA_EXPORT LLVMGenerator {
static Status Make(std::shared_ptr<Configuration> config,
std::unique_ptr<LLVMGenerator>* llvm_generator);
+ static std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ GetCache();
+
/// \brief Build the code for the expression trees for default mode. Each
/// element in the vector represents an expression tree
Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode);
+ /// \brief Build the code for the expression trees for default mode with a LLVM
+ /// ObjectCache. Each element in the vector represents an expression tree
+ template <class KeyType>
+ Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode,
+ GandivaObjectCache<KeyType>& obj_cache) {
+ selection_vector_mode_ = mode;
+
+ // Start measuring code gen time
+ auto begin = std::chrono::high_resolution_clock::now();
+ for (auto& expr : exprs) {
+ auto output = annotator_.AddOutputFieldDescriptor(expr->result());
+ ARROW_RETURN_NOT_OK(Add(expr, output));
+ }
+
+ // Stop measuring time, calculate the elapsed time and pass it to object cache
+ auto end = std::chrono::high_resolution_clock::now();
+ size_t elapsed =
Review comment:
I know, but when the FinalizeModule is called, the LLVM will access the GandivaObjectCache file to execute the logic for the object code manipulation (to cache it or to get it from cache) so I need to set GandivaObjectCache before the FinalizeModule execution, and before the code sets the GandivaObjectCache it needs to know the elapsed time so the greedy dual cache logic works.
Do you have any suggestions how to proceed with that case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713840353
##########
File path: cpp/src/gandiva/greedy_dual_size_cache.h
##########
@@ -146,9 +159,25 @@ class GreedyDualSizeCache {
priority_set_.erase(i);
}
+ void evictObject() {
+ // TODO: inflation overflow is unlikely to happen but needs to be handled
+ // for correctness.
+ // evict item from the beginning of the set. This set is ordered from the
+ // lower priority value to the higher priority value.
+ typename std::set<PriorityItem>::iterator i = priority_set_.begin();
+ // update the inflation cost related to the evicted item
+ inflation_ = (*i).actual_priority;
+ size_t size_to_decrease = map_.find((*i).cache_key)->second.first.size;
+ cache_size_ -= size_to_decrease;
+ map_.erase((*i).cache_key);
+ priority_set_.erase(i);
+ }
+
map_type map_;
std::set<PriorityItem> priority_set_;
uint64_t inflation_;
size_t capacity_;
+ size_t cache_size_ = 0;
+ llvm::SmallString<128> cache_dir_;
Review comment:
Removed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713939964
##########
File path: cpp/src/gandiva/tests/projector_test.cc
##########
@@ -196,21 +202,23 @@ TEST_F(TestProjector, TestProjectCacheDecimalCast) {
auto expr0 = TreeExprBuilder::MakeExpression("castDECIMAL", {field_float64}, res_31_13);
std::shared_ptr<Projector> projector0;
ASSERT_OK(Projector::Make(schema, {expr0}, TestConfiguration(), &projector0));
+ EXPECT_FALSE(projector0->GetBuiltFromCache());
// if the output scale is different, the cache can't be used.
auto res_31_14 = field("result", arrow::decimal(31, 14));
auto expr1 = TreeExprBuilder::MakeExpression("castDECIMAL", {field_float64}, res_31_14);
std::shared_ptr<Projector> projector1;
ASSERT_OK(Projector::Make(schema, {expr1}, TestConfiguration(), &projector1));
- EXPECT_NE(projector0.get(), projector1.get());
+ // EXPECT_NE(projector0.get(), projector1.get()); -> old expect.
Review comment:
can remove the commented code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#issuecomment-985454352
Benchmark runs are scheduled for baseline = a6c81e621b637479a5828c4bdb95ec442ad94bb5 and contender = 188c94a9820ef3377573b835c9dc9dcb96ceafe4. 188c94a9820ef3377573b835c9dc9dcb96ceafe4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/f0dc01b023534d96b0bac4d36ea14c3e...5b059cee5d844774aae1f958873691eb/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/602e996a876949659442f3c145b84f06...f1aa0fd8f0a140058010e12f7a096f3f/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/5c7ec03f40ac4f25a089b656edc6d4b8...721a52173d9848eb8dbecfa19fe61d4d/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714708734
##########
File path: cpp/src/gandiva/projector.h
##########
@@ -34,6 +34,34 @@ namespace gandiva {
class LLVMGenerator;
+class ProjectorCacheKey {
Review comment:
ProjectorCacheKey and FilterCacheKey are not really required now since we are now using a single cache and you are anyway creating basecachekey from them. Just create a single class named ExpressionCacheKey
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714700539
##########
File path: cpp/src/gandiva/base_cache_key.h
##########
@@ -0,0 +1,86 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <arrow/util/hash_util.h>
+#include <stddef.h>
+
+#include "gandiva/expression.h"
+#include "gandiva/filter.h"
+#include "gandiva/projector.h"
+
+namespace gandiva {
+
+class BaseCacheKey {
+ public:
+ BaseCacheKey(ProjectorCacheKey& key, std::string type) : type_(type) {
+ static const int32_t kSeedValue = 4;
+ size_t key_hash = key.Hash();
+ size_t result_hash = kSeedValue;
+ arrow::internal::hash_combine(result_hash, type);
+ arrow::internal::hash_combine(result_hash, key_hash);
+ hash_code_ = result_hash;
+ schema_ = key.schema();
+ expressions_as_string_ = key.GetExpressionsAsString();
+ }
+
+ BaseCacheKey(FilterCacheKey& key, std::string type) : type_(type) {
+ static const size_t kSeedValue = 4;
+ size_t key_hash = key.Hash();
+ size_t result_hash = kSeedValue;
+ arrow::internal::hash_combine(result_hash, type);
+ arrow::internal::hash_combine(result_hash, key_hash);
+ hash_code_ = result_hash;
+ schema_ = key.schema();
+ expressions_as_string_.push_back(key.GetExpressionsAsString());
+ }
+
+ size_t Hash() const { return hash_code_; }
+
+ std::string Type() const { return type_; }
+
+ bool operator==(const BaseCacheKey& other) const {
+ if (hash_code_ != other.hash_code_) {
+ return false;
+ }
+
+ if (!schema_->Equals(other.schema_)) {
+ return false;
+ }
+
+ if (type_ != other.type_) {
+ return false;
+ }
+
+ if (expressions_as_string_ != other.expressions_as_string_) {
+ return false;
+ }
+
+ return true;
+ }
+
+ bool operator!=(const BaseCacheKey& other) const { return !(*this == other); }
+
+ private:
+ uint64_t hash_code_;
Review comment:
Changed it to size_t type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r731798091
##########
File path: cpp/src/gandiva/cache.h
##########
@@ -39,17 +39,21 @@ class Cache {
Cache() : Cache(GetCapacity()) {}
- ValueType GetModule(KeyType cache_key) {
+ ::std::shared_ptr<Cache> create(size_t capacity) {
+ return ::std::make_shared<Cache>(cache_(capacity));
Review comment:
`make_shared<Cache>(capacity)`
##########
File path: cpp/src/gandiva/expression_cache_key.h
##########
@@ -0,0 +1,126 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <stddef.h>
+
+#include <thread>
+
+#include "arrow/util/hash_util.h"
+#include "gandiva/arrow.h"
+#include "gandiva/configuration.h"
+#include "gandiva/expression.h"
+#include "gandiva/selection_vector.h"
+#include "gandiva/visibility.h"
+
+namespace gandiva {
+
+class ExpressionCacheKey {
+ public:
+ ExpressionCacheKey(SchemaPtr schema, std::shared_ptr<Configuration> configuration,
+ ExpressionVector expression_vector, SelectionVector::Mode mode,
+ std::string type)
+ : type_(type),
+ schema_(schema),
+ mode_(mode),
+ uniqifier_(0),
+ configuration_(configuration) {
+ static const int kSeedValue = 4;
+ size_t result = kSeedValue;
+ for (auto& expr : expression_vector) {
+ std::string expr_as_string = expr->ToString();
+ expressions_as_strings_.push_back(expr_as_string);
+ arrow::internal::hash_combine(result, expr_as_string);
+ UpdateUniqifier(expr_as_string);
+ }
+ arrow::internal::hash_combine(result, static_cast<size_t>(mode));
+ arrow::internal::hash_combine(result, configuration->Hash());
+ arrow::internal::hash_combine(result, schema_->ToString());
+ arrow::internal::hash_combine(result, uniqifier_);
+ hash_code_ = result;
+ }
+
+ ExpressionCacheKey(SchemaPtr schema, std::shared_ptr<Configuration> configuration,
+ Expression& expression, std::string type)
+ : type_(type), schema_(schema), uniqifier_(0), configuration_(configuration) {
+ static const int kSeedValue = 4;
+ size_t result = kSeedValue;
+ expressions_as_strings_.push_back(expression.ToString());
+ UpdateUniqifier(expression.ToString());
+
+ arrow::internal::hash_combine(result, configuration->Hash());
+ arrow::internal::hash_combine(result, schema_->ToString());
+ arrow::internal::hash_combine(result, uniqifier_);
+ hash_code_ = result;
+ }
+
+ void UpdateUniqifier(const std::string& expr) {
+ if (uniqifier_ == 0) {
+ // caching of expressions with re2 patterns causes lock contention. So, use
+ // multiple instances to reduce contention.
+ if (expr.find(" like(") != std::string::npos) {
+ uniqifier_ = std::hash<std::thread::id>()(std::this_thread::get_id()) % 16;
+ }
+ }
+ }
+
+ size_t Hash() const { return hash_code_; }
+
+ std::string Type() const { return type_; }
+
+ bool operator==(const ExpressionCacheKey& other) const {
+ if (type_ != other.type_) {
+ return false;
+ }
+
+ if (hash_code_ != other.hash_code_) {
+ return false;
+ }
+
+ if (!(schema_->Equals(*other.schema_, true))) {
+ return false;
+ }
+
+ if (configuration_ != other.configuration_) {
+ return false;
+ }
+
+ if (expressions_as_strings_ != other.expressions_as_strings_) {
+ return false;
+ }
+
+ if (uniqifier_ != other.uniqifier_) {
+ return false;
+ }
+
+ return true;
+ }
+
+ bool operator!=(const ExpressionCacheKey& other) const { return !(*this == other); }
+
+ private:
+ size_t hash_code_;
+ std::string type_;
Review comment:
use enum for type
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -102,14 +46,30 @@ Status Filter::Make(SchemaPtr schema, ConditionPtr condition,
ARROW_RETURN_IF(configuration == nullptr,
Status::Invalid("Configuration cannot be null"));
- static Cache<FilterCacheKey, std::shared_ptr<Filter>> cache;
- FilterCacheKey cache_key(schema, configuration, *(condition.get()));
- auto cachedFilter = cache.GetModule(cache_key);
- if (cachedFilter != nullptr) {
- *filter = cachedFilter;
- return Status::OK();
+ std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ shared_cache = LLVMGenerator::GetCache();
+
+ Condition conditionToKey = *(condition.get());
+
+ ExpressionCacheKey cache_key(schema, configuration, conditionToKey, "filter");
+ std::unique_ptr<ExpressionCacheKey> base_cache_key =
+ std::make_unique<ExpressionCacheKey>(cache_key);
+ std::shared_ptr<ExpressionCacheKey> shared_base_cache_key = std::move(base_cache_key);
+
+ bool llvm_flag = false;
Review comment:
nit: use a better name like "is_cached" or something
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -102,14 +46,30 @@ Status Filter::Make(SchemaPtr schema, ConditionPtr condition,
ARROW_RETURN_IF(configuration == nullptr,
Status::Invalid("Configuration cannot be null"));
- static Cache<FilterCacheKey, std::shared_ptr<Filter>> cache;
- FilterCacheKey cache_key(schema, configuration, *(condition.get()));
- auto cachedFilter = cache.GetModule(cache_key);
- if (cachedFilter != nullptr) {
- *filter = cachedFilter;
- return Status::OK();
+ std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ shared_cache = LLVMGenerator::GetCache();
+
+ Condition conditionToKey = *(condition.get());
+
+ ExpressionCacheKey cache_key(schema, configuration, conditionToKey, "filter");
+ std::unique_ptr<ExpressionCacheKey> base_cache_key =
+ std::make_unique<ExpressionCacheKey>(cache_key);
+ std::shared_ptr<ExpressionCacheKey> shared_base_cache_key = std::move(base_cache_key);
Review comment:
directly create using std::make_shared instead of in three steps
##########
File path: cpp/src/gandiva/llvm_generator.h
##########
@@ -240,7 +269,7 @@ class GANDIVA_EXPORT LLVMGenerator {
void AddTrace(const std::string& msg, llvm::Value* value = NULLPTR);
std::unique_ptr<Engine> engine_;
- std::vector<std::unique_ptr<CompiledExpr>> compiled_exprs_;
+ std::vector<std::shared_ptr<CompiledExpr>> compiled_exprs_;
Review comment:
why this change?
##########
File path: cpp/src/gandiva/cache.h
##########
@@ -39,17 +39,21 @@ class Cache {
Cache() : Cache(GetCapacity()) {}
- ValueType GetModule(KeyType cache_key) {
+ ::std::shared_ptr<Cache> create(size_t capacity) {
+ return ::std::make_shared<Cache>(cache_(capacity));
Review comment:
Looks like this method is not getting used. You can remove it.
##########
File path: cpp/src/gandiva/expression_cache_key.h
##########
@@ -0,0 +1,126 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <stddef.h>
+
+#include <thread>
+
+#include "arrow/util/hash_util.h"
+#include "gandiva/arrow.h"
+#include "gandiva/configuration.h"
+#include "gandiva/expression.h"
+#include "gandiva/selection_vector.h"
+#include "gandiva/visibility.h"
+
+namespace gandiva {
+
+class ExpressionCacheKey {
+ public:
+ ExpressionCacheKey(SchemaPtr schema, std::shared_ptr<Configuration> configuration,
+ ExpressionVector expression_vector, SelectionVector::Mode mode,
+ std::string type)
+ : type_(type),
+ schema_(schema),
+ mode_(mode),
+ uniqifier_(0),
+ configuration_(configuration) {
+ static const int kSeedValue = 4;
+ size_t result = kSeedValue;
+ for (auto& expr : expression_vector) {
+ std::string expr_as_string = expr->ToString();
+ expressions_as_strings_.push_back(expr_as_string);
+ arrow::internal::hash_combine(result, expr_as_string);
+ UpdateUniqifier(expr_as_string);
+ }
+ arrow::internal::hash_combine(result, static_cast<size_t>(mode));
+ arrow::internal::hash_combine(result, configuration->Hash());
+ arrow::internal::hash_combine(result, schema_->ToString());
+ arrow::internal::hash_combine(result, uniqifier_);
+ hash_code_ = result;
+ }
+
+ ExpressionCacheKey(SchemaPtr schema, std::shared_ptr<Configuration> configuration,
+ Expression& expression, std::string type)
+ : type_(type), schema_(schema), uniqifier_(0), configuration_(configuration) {
+ static const int kSeedValue = 4;
+ size_t result = kSeedValue;
+ expressions_as_strings_.push_back(expression.ToString());
+ UpdateUniqifier(expression.ToString());
+
+ arrow::internal::hash_combine(result, configuration->Hash());
+ arrow::internal::hash_combine(result, schema_->ToString());
+ arrow::internal::hash_combine(result, uniqifier_);
+ hash_code_ = result;
+ }
+
+ void UpdateUniqifier(const std::string& expr) {
+ if (uniqifier_ == 0) {
+ // caching of expressions with re2 patterns causes lock contention. So, use
+ // multiple instances to reduce contention.
+ if (expr.find(" like(") != std::string::npos) {
+ uniqifier_ = std::hash<std::thread::id>()(std::this_thread::get_id()) % 16;
+ }
+ }
+ }
+
+ size_t Hash() const { return hash_code_; }
+
+ std::string Type() const { return type_; }
+
+ bool operator==(const ExpressionCacheKey& other) const {
+ if (type_ != other.type_) {
+ return false;
+ }
+
+ if (hash_code_ != other.hash_code_) {
+ return false;
+ }
+
+ if (!(schema_->Equals(*other.schema_, true))) {
+ return false;
+ }
+
+ if (configuration_ != other.configuration_) {
+ return false;
+ }
+
+ if (expressions_as_strings_ != other.expressions_as_strings_) {
+ return false;
+ }
+
+ if (uniqifier_ != other.uniqifier_) {
+ return false;
+ }
+
+ return true;
+ }
+
+ bool operator!=(const ExpressionCacheKey& other) const { return !(*this == other); }
+
+ private:
+ size_t hash_code_;
+ std::string type_;
Review comment:
on second thought, why is type required? Shouldn't the generated code be same for filter and project
##########
File path: cpp/src/gandiva/llvm_generator.h
##########
@@ -49,14 +51,41 @@ class GANDIVA_EXPORT LLVMGenerator {
static Status Make(std::shared_ptr<Configuration> config,
std::unique_ptr<LLVMGenerator>* llvm_generator);
- /// \brief Build the code for the expression trees for default mode. Each
- /// element in the vector represents an expression tree
- Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode);
+ static std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ GetCache();
+
+ /// \brief Build the code for the expression trees for default mode with a LLVM
+ /// ObjectCache. Each element in the vector represents an expression tree
+ template <class KeyType>
+ Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode,
+ GandivaObjectCache<KeyType>& obj_cache) {
+ selection_vector_mode_ = mode;
+
+ for (auto& expr : exprs) {
+ auto output = annotator_.AddOutputFieldDescriptor(expr->result());
+ ARROW_RETURN_NOT_OK(Add(expr, output));
+ }
+
+ engine_->SetLLVMObjectCache(obj_cache);
Review comment:
I see the only change is this line. How about keeping the earlier method as is and adding a new method to llvm_generator call SetLLVMObjectCache(obj_cache) which is called by projector/filter before Build()
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -102,14 +46,30 @@ Status Filter::Make(SchemaPtr schema, ConditionPtr condition,
ARROW_RETURN_IF(configuration == nullptr,
Status::Invalid("Configuration cannot be null"));
- static Cache<FilterCacheKey, std::shared_ptr<Filter>> cache;
- FilterCacheKey cache_key(schema, configuration, *(condition.get()));
- auto cachedFilter = cache.GetModule(cache_key);
- if (cachedFilter != nullptr) {
- *filter = cachedFilter;
- return Status::OK();
+ std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ shared_cache = LLVMGenerator::GetCache();
+
+ Condition conditionToKey = *(condition.get());
+
+ ExpressionCacheKey cache_key(schema, configuration, conditionToKey, "filter");
+ std::unique_ptr<ExpressionCacheKey> base_cache_key =
+ std::make_unique<ExpressionCacheKey>(cache_key);
+ std::shared_ptr<ExpressionCacheKey> shared_base_cache_key = std::move(base_cache_key);
+
+ bool llvm_flag = false;
+
+ std::shared_ptr<llvm::MemoryBuffer> prev_cached_obj;
+ prev_cached_obj = shared_cache->GetObjectCode(*shared_base_cache_key);
+
+ // Verify if previous filter obj code was cached
+ if (prev_cached_obj != nullptr) {
+ ARROW_LOG(DEBUG)
+ << "[DEBUG][CACHE-LOG][INFO]: Filter object code WAS already cached!";
Review comment:
nit: DEBUG and INFO in same message?
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -102,14 +46,30 @@ Status Filter::Make(SchemaPtr schema, ConditionPtr condition,
ARROW_RETURN_IF(configuration == nullptr,
Status::Invalid("Configuration cannot be null"));
- static Cache<FilterCacheKey, std::shared_ptr<Filter>> cache;
- FilterCacheKey cache_key(schema, configuration, *(condition.get()));
- auto cachedFilter = cache.GetModule(cache_key);
- if (cachedFilter != nullptr) {
- *filter = cachedFilter;
- return Status::OK();
+ std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ shared_cache = LLVMGenerator::GetCache();
+
+ Condition conditionToKey = *(condition.get());
+
+ ExpressionCacheKey cache_key(schema, configuration, conditionToKey, "filter");
+ std::unique_ptr<ExpressionCacheKey> base_cache_key =
+ std::make_unique<ExpressionCacheKey>(cache_key);
+ std::shared_ptr<ExpressionCacheKey> shared_base_cache_key = std::move(base_cache_key);
Review comment:
also you are unnecessary allocating. just use cache_key and make GandivaObjectCache can take an object instead of shared_pointer.
##########
File path: cpp/src/gandiva/gandiva_object_cache.h
##########
@@ -0,0 +1,86 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#if defined(_MSC_VER)
+#pragma warning(push)
+#pragma warning(disable : 4244)
+#pragma warning(disable : 4141)
+#pragma warning(disable : 4146)
+#pragma warning(disable : 4267)
+#pragma warning(disable : 4624)
+#endif
+
+#include <llvm/ExecutionEngine/ObjectCache.h>
+#include <llvm/Support/MemoryBuffer.h>
+
+#include <chrono>
+
+#include "gandiva/cache.h"
+
+namespace gandiva {
+/// Class that enables the LLVM to use a custom rule to deal with the object code.
+template <class CacheKey>
+class GandivaObjectCache : public llvm::ObjectCache {
+ public:
+ GandivaObjectCache(
+ std::shared_ptr<Cache<CacheKey, std::shared_ptr<llvm::MemoryBuffer>>>& cache,
+ std::shared_ptr<CacheKey>& key) {
+ cache_ = cache;
+ cache_key_ = key;
+ // Start measuring code gen time
+ begin_time_ = std::chrono::high_resolution_clock::now();
+ }
+
+ ~GandivaObjectCache() {}
+
+ void notifyObjectCompiled(const llvm::Module* M, llvm::MemoryBufferRef Obj) {
+ // Stop measuring time and calculate the elapsed time to compile the object code
+ auto end_time = std::chrono::high_resolution_clock::now();
+ auto elapsed_time =
+ std::chrono::duration_cast<std::chrono::milliseconds>(end_time - begin_time_)
+ .count();
+
+ std::unique_ptr<llvm::MemoryBuffer> obj_buffer =
+ llvm::MemoryBuffer::getMemBufferCopy(Obj.getBuffer(), Obj.getBufferIdentifier());
+ std::shared_ptr<llvm::MemoryBuffer> obj_code = std::move(obj_buffer);
+
+ ValueCacheObject<std::shared_ptr<llvm::MemoryBuffer>> value_cache(
+ obj_code, elapsed_time, obj_code->getBufferSize());
+
+ cache_->PutObjectCode(*cache_key_.get(), value_cache);
+ }
+
+ std::unique_ptr<llvm::MemoryBuffer> getObject(const llvm::Module* M) {
+ std::shared_ptr<llvm::MemoryBuffer> cached_obj =
+ cache_->GetObjectCode(*cache_key_.get());
+ auto null = std::nullptr_t();
+ if (cached_obj != null) {
Review comment:
just use nullptr
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r734102208
##########
File path: cpp/src/gandiva/projector.cc
##########
@@ -174,13 +93,11 @@ Status Projector::Make(SchemaPtr schema, const ExpressionVector& exprs,
ARROW_RETURN_NOT_OK(expr_validator.Validate(expr));
}
- // Start measuring build time
- auto begin = std::chrono::high_resolution_clock::now();
- ARROW_RETURN_NOT_OK(llvm_gen->Build(exprs, selection_vector_mode));
- // Stop measuring time and calculate the elapsed time
- auto end = std::chrono::high_resolution_clock::now();
- auto elapsed =
- std::chrono::duration_cast<std::chrono::milliseconds>(end - begin).count();
+ // Set the object cache for LLVM
+ llvm_gen->SetLLVMObjectCache(obj_cache);
Review comment:
Yes, it should work in multithreaded scenario.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r734100866
##########
File path: cpp/src/gandiva/projector.cc
##########
@@ -174,13 +93,11 @@ Status Projector::Make(SchemaPtr schema, const ExpressionVector& exprs,
ARROW_RETURN_NOT_OK(expr_validator.Validate(expr));
}
- // Start measuring build time
- auto begin = std::chrono::high_resolution_clock::now();
- ARROW_RETURN_NOT_OK(llvm_gen->Build(exprs, selection_vector_mode));
- // Stop measuring time and calculate the elapsed time
- auto end = std::chrono::high_resolution_clock::now();
- auto elapsed =
- std::chrono::duration_cast<std::chrono::milliseconds>(end - begin).count();
+ // Set the object cache for LLVM
+ llvm_gen->SetLLVMObjectCache(obj_cache);
+
+ ARROW_RETURN_NOT_OK(llvm_gen->Build(
+ exprs, selection_vector_mode)); // to use when caching only the obj code
Review comment:
Removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r733722827
##########
File path: cpp/src/gandiva/gandiva_object_cache.h
##########
@@ -0,0 +1,84 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#if defined(_MSC_VER)
+#pragma warning(push)
+#pragma warning(disable : 4244)
+#pragma warning(disable : 4141)
+#pragma warning(disable : 4146)
+#pragma warning(disable : 4267)
+#pragma warning(disable : 4624)
+#endif
+
+#include <llvm/ExecutionEngine/ObjectCache.h>
+#include <llvm/Support/MemoryBuffer.h>
+
+#include <chrono>
+
+#include "expression_cache_key.h"
+#include "gandiva/cache.h"
+
+namespace gandiva {
+/// Class that enables the LLVM to use a custom rule to deal with the object code.
+class GandivaObjectCache : public llvm::ObjectCache {
+ public:
+ explicit GandivaObjectCache(
+ std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>& cache,
+ ExpressionCacheKey key) : cache_key_(key) {
+ cache_ = cache;
+ // Start measuring code gen time
+ begin_time_ = std::chrono::high_resolution_clock::now();
+ }
+
+ ~GandivaObjectCache() {}
+
+ void notifyObjectCompiled(const llvm::Module* M, llvm::MemoryBufferRef Obj) {
+ // Stop measuring time and calculate the elapsed time to compile the object code
+ auto end_time = std::chrono::high_resolution_clock::now();
+ auto elapsed_time =
+ std::chrono::duration_cast<std::chrono::milliseconds>(end_time - begin_time_)
+ .count();
+
+ std::unique_ptr<llvm::MemoryBuffer> obj_buffer =
+ llvm::MemoryBuffer::getMemBufferCopy(Obj.getBuffer(), Obj.getBufferIdentifier());
+ std::shared_ptr<llvm::MemoryBuffer> obj_code = std::move(obj_buffer);
+
+ ValueCacheObject<std::shared_ptr<llvm::MemoryBuffer>> value_cache(
+ obj_code, elapsed_time, obj_code->getBufferSize());
+
+ cache_->PutObjectCode(cache_key_, value_cache);
+ }
+
+ std::unique_ptr<llvm::MemoryBuffer> getObject(const llvm::Module* M) {
+ std::shared_ptr<llvm::MemoryBuffer> cached_obj =
+ cache_->GetObjectCode(cache_key_);
Review comment:
I think we should log when there is a cache hit/miss (with INFO level). Also log during cache eviction
##########
File path: cpp/src/gandiva/projector.cc
##########
@@ -174,13 +93,11 @@ Status Projector::Make(SchemaPtr schema, const ExpressionVector& exprs,
ARROW_RETURN_NOT_OK(expr_validator.Validate(expr));
}
- // Start measuring build time
- auto begin = std::chrono::high_resolution_clock::now();
- ARROW_RETURN_NOT_OK(llvm_gen->Build(exprs, selection_vector_mode));
- // Stop measuring time and calculate the elapsed time
- auto end = std::chrono::high_resolution_clock::now();
- auto elapsed =
- std::chrono::duration_cast<std::chrono::milliseconds>(end - begin).count();
+ // Set the object cache for LLVM
+ llvm_gen->SetLLVMObjectCache(obj_cache);
+
+ ARROW_RETURN_NOT_OK(llvm_gen->Build(
+ exprs, selection_vector_mode)); // to use when caching only the obj code
Review comment:
nit: the comment isn't necessary now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713836840
##########
File path: cpp/src/gandiva/cache.h
##########
@@ -39,20 +39,30 @@ class Cache {
Cache() : Cache(GetCapacity()) {}
- ValueType GetModule(KeyType cache_key) {
+ ::std::shared_ptr<Cache> create(size_t capacity) {
+ return ::std::make_shared<Cache>(cache_(capacity));
+ }
+
+ ValueType GetObjectCode(KeyType cache_key) {
arrow::util::optional<ValueCacheObject<ValueType>> result;
mtx_.lock();
- result = cache_.get(cache_key);
+ result = cache_.GetObjectCode(cache_key);
mtx_.unlock();
return result != arrow::util::nullopt ? (*result).module : nullptr;
}
- void PutModule(KeyType cache_key, ValueCacheObject<ValueType> valueCacheObject) {
+ void PutObjectCode(KeyType& cache_key, ValueCacheObject<ValueType> object_code) {
mtx_.lock();
- cache_.insert(cache_key, valueCacheObject);
+ cache_.InsertObjectCode(cache_key, object_code);
mtx_.unlock();
}
+ ::std::shared_ptr<Cache> CreateSharedCachePtr() { return Cache::create(); }
Review comment:
Removed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713841531
##########
File path: cpp/src/gandiva/llvm_generator.cc
##########
@@ -50,6 +51,23 @@ Status LLVMGenerator::Make(std::shared_ptr<Configuration> config,
return Status::OK();
}
+std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+LLVMGenerator::GetCache() {
+ static Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>> cache;
Review comment:
Removed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713844650
##########
File path: cpp/src/gandiva/llvm_generator.h
##########
@@ -49,10 +51,48 @@ class GANDIVA_EXPORT LLVMGenerator {
static Status Make(std::shared_ptr<Configuration> config,
std::unique_ptr<LLVMGenerator>* llvm_generator);
+ static std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ GetCache();
+
/// \brief Build the code for the expression trees for default mode. Each
/// element in the vector represents an expression tree
Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode);
+ /// \brief Build the code for the expression trees for default mode with a LLVM
+ /// ObjectCache. Each element in the vector represents an expression tree
+ template <class KeyType>
+ Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode,
Review comment:
I know, but because this uses template, if it is on the cc file this function gets an undefined reference error for it, as just this function is using template.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713837690
##########
File path: cpp/src/gandiva/tests/filter_test.cc
##########
@@ -29,7 +29,11 @@ using arrow::int32;
class TestFilter : public ::testing::Test {
public:
- void SetUp() { pool_ = arrow::default_memory_pool(); }
+ void SetUp() {
+ pool_ = arrow::default_memory_pool();
+ // Setup arrow log severity threshold to debug level.
+ arrow::util::ArrowLog::StartArrowLog("", arrow::util::ArrowLogLevel::ARROW_DEBUG);
Review comment:
Not required, just useful to debug the cache system using the tests' files. Do you want me to remove it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713835246
##########
File path: cpp/src/gandiva/greedy_dual_size_cache.h
##########
@@ -92,25 +96,26 @@ class GreedyDualSizeCache {
bool contains(const Key& key) { return map_.find(key) != map_.end(); }
- void insert(const Key& key, const ValueCacheObject<Value>& value) {
+ void InsertObjectCode(const Key& key, const ValueCacheObject<Value>& value) {
Review comment:
Ok, fixed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714698769
##########
File path: cpp/src/gandiva/cache.cc
##########
@@ -20,10 +20,10 @@
namespace gandiva {
-static const int DEFAULT_CACHE_SIZE = 500;
+static const size_t DEFAULT_CACHE_SIZE = 128 * 1024 * 1024; // 256 MiB
-int GetCapacity() {
- int capacity;
+size_t GetCapacity() {
+ size_t capacity;
const char* env_cache_size = std::getenv("GANDIVA_CACHE_SIZE");
if (env_cache_size != nullptr) {
capacity = std::atoi(env_cache_size);
Review comment:
since it is calculating size it will be capped to int_max (2GB). Better to parse it as long.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r715205963
##########
File path: cpp/src/gandiva/greedy_dual_size_cache.h
##########
@@ -133,6 +138,14 @@ class GreedyDualSizeCache {
priority_set_.clear();
}
+ size_t GetCacheSize() { return cache_size_; }
+
+ std::string ToString() {
Review comment:
Removed then
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r737138263
##########
File path: cpp/src/gandiva/gandiva_object_cache.h
##########
@@ -71,8 +71,10 @@ class GandivaObjectCache : public llvm::ObjectCache {
if (cached_obj != nullptr) {
std::unique_ptr<llvm::MemoryBuffer> cached_buffer = cached_obj->getMemBufferCopy(
cached_obj->getBuffer(), cached_obj->getBufferIdentifier());
+ ARROW_LOG(INFO) << "[INFO][CACHE-LOG]: An object code was found on cache.";
Review comment:
@augustoasilva Also in the greedy dual size class. Also you can remove the log in the projector and filter since it is already logged here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r736413175
##########
File path: cpp/src/gandiva/gandiva_object_cache.h
##########
@@ -71,8 +71,10 @@ class GandivaObjectCache : public llvm::ObjectCache {
if (cached_obj != nullptr) {
std::unique_ptr<llvm::MemoryBuffer> cached_buffer = cached_obj->getMemBufferCopy(
cached_obj->getBuffer(), cached_obj->getBufferIdentifier());
+ ARROW_LOG(INFO) << "[INFO][CACHE-LOG]: An object code was found on cache.";
Review comment:
Ok, I fixed that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713936262
##########
File path: cpp/src/gandiva/llvm_generator.h
##########
@@ -49,10 +51,48 @@ class GANDIVA_EXPORT LLVMGenerator {
static Status Make(std::shared_ptr<Configuration> config,
std::unique_ptr<LLVMGenerator>* llvm_generator);
+ static std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ GetCache();
+
/// \brief Build the code for the expression trees for default mode. Each
/// element in the vector represents an expression tree
Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode);
+ /// \brief Build the code for the expression trees for default mode with a LLVM
+ /// ObjectCache. Each element in the vector represents an expression tree
+ template <class KeyType>
+ Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode,
+ GandivaObjectCache<KeyType>& obj_cache) {
+ selection_vector_mode_ = mode;
+
+ // Start measuring code gen time
+ auto begin = std::chrono::high_resolution_clock::now();
+ for (auto& expr : exprs) {
+ auto output = annotator_.AddOutputFieldDescriptor(expr->result());
+ ARROW_RETURN_NOT_OK(Add(expr, output));
+ }
+
+ // Stop measuring time, calculate the elapsed time and pass it to object cache
+ auto end = std::chrono::high_resolution_clock::now();
+ size_t elapsed =
Review comment:
You can track the time inside the gandivaObjectCache; start timer in constructor and stop it in notifyObject method?
Also, its best not to knowingly proceed with incorrect solution if you are stuck somewhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713846478
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -120,17 +138,27 @@ Status Filter::Make(SchemaPtr schema, ConditionPtr condition,
ARROW_RETURN_NOT_OK(expr_validator.Validate(condition));
// Start measuring build time
- auto begin = std::chrono::high_resolution_clock::now();
- ARROW_RETURN_NOT_OK(llvm_gen->Build({condition}, SelectionVector::Mode::MODE_NONE));
+ // auto begin = std::chrono::high_resolution_clock::now();
Review comment:
Removed it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r732332683
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -102,14 +46,30 @@ Status Filter::Make(SchemaPtr schema, ConditionPtr condition,
ARROW_RETURN_IF(configuration == nullptr,
Status::Invalid("Configuration cannot be null"));
- static Cache<FilterCacheKey, std::shared_ptr<Filter>> cache;
- FilterCacheKey cache_key(schema, configuration, *(condition.get()));
- auto cachedFilter = cache.GetModule(cache_key);
- if (cachedFilter != nullptr) {
- *filter = cachedFilter;
- return Status::OK();
+ std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ shared_cache = LLVMGenerator::GetCache();
+
+ Condition conditionToKey = *(condition.get());
+
+ ExpressionCacheKey cache_key(schema, configuration, conditionToKey, "filter");
+ std::unique_ptr<ExpressionCacheKey> base_cache_key =
+ std::make_unique<ExpressionCacheKey>(cache_key);
+ std::shared_ptr<ExpressionCacheKey> shared_base_cache_key = std::move(base_cache_key);
+
+ bool llvm_flag = false;
+
+ std::shared_ptr<llvm::MemoryBuffer> prev_cached_obj;
+ prev_cached_obj = shared_cache->GetObjectCode(*shared_base_cache_key);
+
+ // Verify if previous filter obj code was cached
+ if (prev_cached_obj != nullptr) {
+ ARROW_LOG(DEBUG)
+ << "[DEBUG][CACHE-LOG][INFO]: Filter object code WAS already cached!";
Review comment:
Just debug, fixed the message
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r733722827
##########
File path: cpp/src/gandiva/gandiva_object_cache.h
##########
@@ -0,0 +1,84 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#if defined(_MSC_VER)
+#pragma warning(push)
+#pragma warning(disable : 4244)
+#pragma warning(disable : 4141)
+#pragma warning(disable : 4146)
+#pragma warning(disable : 4267)
+#pragma warning(disable : 4624)
+#endif
+
+#include <llvm/ExecutionEngine/ObjectCache.h>
+#include <llvm/Support/MemoryBuffer.h>
+
+#include <chrono>
+
+#include "expression_cache_key.h"
+#include "gandiva/cache.h"
+
+namespace gandiva {
+/// Class that enables the LLVM to use a custom rule to deal with the object code.
+class GandivaObjectCache : public llvm::ObjectCache {
+ public:
+ explicit GandivaObjectCache(
+ std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>& cache,
+ ExpressionCacheKey key) : cache_key_(key) {
+ cache_ = cache;
+ // Start measuring code gen time
+ begin_time_ = std::chrono::high_resolution_clock::now();
+ }
+
+ ~GandivaObjectCache() {}
+
+ void notifyObjectCompiled(const llvm::Module* M, llvm::MemoryBufferRef Obj) {
+ // Stop measuring time and calculate the elapsed time to compile the object code
+ auto end_time = std::chrono::high_resolution_clock::now();
+ auto elapsed_time =
+ std::chrono::duration_cast<std::chrono::milliseconds>(end_time - begin_time_)
+ .count();
+
+ std::unique_ptr<llvm::MemoryBuffer> obj_buffer =
+ llvm::MemoryBuffer::getMemBufferCopy(Obj.getBuffer(), Obj.getBufferIdentifier());
+ std::shared_ptr<llvm::MemoryBuffer> obj_code = std::move(obj_buffer);
+
+ ValueCacheObject<std::shared_ptr<llvm::MemoryBuffer>> value_cache(
+ obj_code, elapsed_time, obj_code->getBufferSize());
+
+ cache_->PutObjectCode(cache_key_, value_cache);
+ }
+
+ std::unique_ptr<llvm::MemoryBuffer> getObject(const llvm::Module* M) {
+ std::shared_ptr<llvm::MemoryBuffer> cached_obj =
+ cache_->GetObjectCode(cache_key_);
Review comment:
I think we should log when there is a cache hit & miss (with INFO level) (i see there is already log during cache hit in projector.cc/filter.cc). Also log during cache eviction
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r761502021
##########
File path: cpp/src/gandiva/cache.h
##########
@@ -39,17 +39,17 @@ class Cache {
Cache() : Cache(GetCapacity()) {}
- ValueType GetModule(KeyType cache_key) {
+ ValueType GetObjectCode(KeyType cache_key) {
arrow::util::optional<ValueCacheObject<ValueType>> result;
mtx_.lock();
result = cache_.get(cache_key);
mtx_.unlock();
return result != arrow::util::nullopt ? (*result).module : nullptr;
}
- void PutModule(KeyType cache_key, ValueCacheObject<ValueType> valueCacheObject) {
+ void PutObjectCode(KeyType& cache_key, ValueCacheObject<ValueType> object_code) {
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pravindra closed pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
pravindra closed pull request #11193:
URL: https://github.com/apache/arrow/pull/11193
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713891180
##########
File path: cpp/src/gandiva/engine.h
##########
@@ -54,6 +54,13 @@ class GANDIVA_EXPORT Engine {
functions_to_compile_.push_back(fname);
}
+ /// Set BaseObjectCache.
+ template <class KeyType>
+ Status SetLLVMObjectCache(GandivaObjectCache<KeyType>& object_cache) {
+ execution_engine_->setObjectCache(&object_cache);
+ return Status::OK();
Review comment:
It was failling on appveyor with the message: `error C2039: 'hasError': is not a member of 'llvm::ExecutionEngine'`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713941321
##########
File path: cpp/src/gandiva/greedy_dual_size_cache.h
##########
@@ -133,6 +138,14 @@ class GreedyDualSizeCache {
priority_set_.clear();
}
+ size_t GetCacheSize() { return cache_size_; }
+
+ std::string ToString() {
Review comment:
Is this required?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714352692
##########
File path: cpp/src/gandiva/greedy_dual_size_cache.h
##########
@@ -133,6 +138,14 @@ class GreedyDualSizeCache {
priority_set_.clear();
}
+ size_t GetCacheSize() { return cache_size_; }
+
+ std::string ToString() {
Review comment:
Not required, only used for debugging. Should remove it and also remove the lines that call it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#issuecomment-985454352
Benchmark runs are scheduled for baseline = a6c81e621b637479a5828c4bdb95ec442ad94bb5 and contender = 188c94a9820ef3377573b835c9dc9dcb96ceafe4. 188c94a9820ef3377573b835c9dc9dcb96ceafe4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/f0dc01b023534d96b0bac4d36ea14c3e...5b059cee5d844774aae1f958873691eb/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/602e996a876949659442f3c145b84f06...f1aa0fd8f0a140058010e12f7a096f3f/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/5c7ec03f40ac4f25a089b656edc6d4b8...721a52173d9848eb8dbecfa19fe61d4d/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r731940395
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -42,7 +43,7 @@ FilterCacheKey::FilterCacheKey(SchemaPtr schema,
expression_as_string_ = expression.ToString();
UpdateUniqifier(expression_as_string_);
arrow::internal::hash_combine(result, expression_as_string_);
- arrow::internal::hash_combine(result, configuration);
+ arrow::internal::hash_combine(result, configuration->Hash());
Review comment:
looks fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r732323838
##########
File path: cpp/src/gandiva/expression_cache_key.h
##########
@@ -0,0 +1,126 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <stddef.h>
+
+#include <thread>
+
+#include "arrow/util/hash_util.h"
+#include "gandiva/arrow.h"
+#include "gandiva/configuration.h"
+#include "gandiva/expression.h"
+#include "gandiva/selection_vector.h"
+#include "gandiva/visibility.h"
+
+namespace gandiva {
+
+class ExpressionCacheKey {
+ public:
+ ExpressionCacheKey(SchemaPtr schema, std::shared_ptr<Configuration> configuration,
+ ExpressionVector expression_vector, SelectionVector::Mode mode,
+ std::string type)
+ : type_(type),
+ schema_(schema),
+ mode_(mode),
+ uniqifier_(0),
+ configuration_(configuration) {
+ static const int kSeedValue = 4;
+ size_t result = kSeedValue;
+ for (auto& expr : expression_vector) {
+ std::string expr_as_string = expr->ToString();
+ expressions_as_strings_.push_back(expr_as_string);
+ arrow::internal::hash_combine(result, expr_as_string);
+ UpdateUniqifier(expr_as_string);
+ }
+ arrow::internal::hash_combine(result, static_cast<size_t>(mode));
+ arrow::internal::hash_combine(result, configuration->Hash());
+ arrow::internal::hash_combine(result, schema_->ToString());
+ arrow::internal::hash_combine(result, uniqifier_);
+ hash_code_ = result;
+ }
+
+ ExpressionCacheKey(SchemaPtr schema, std::shared_ptr<Configuration> configuration,
+ Expression& expression, std::string type)
+ : type_(type), schema_(schema), uniqifier_(0), configuration_(configuration) {
+ static const int kSeedValue = 4;
+ size_t result = kSeedValue;
+ expressions_as_strings_.push_back(expression.ToString());
+ UpdateUniqifier(expression.ToString());
+
+ arrow::internal::hash_combine(result, configuration->Hash());
+ arrow::internal::hash_combine(result, schema_->ToString());
+ arrow::internal::hash_combine(result, uniqifier_);
+ hash_code_ = result;
+ }
+
+ void UpdateUniqifier(const std::string& expr) {
+ if (uniqifier_ == 0) {
+ // caching of expressions with re2 patterns causes lock contention. So, use
+ // multiple instances to reduce contention.
+ if (expr.find(" like(") != std::string::npos) {
+ uniqifier_ = std::hash<std::thread::id>()(std::this_thread::get_id()) % 16;
+ }
+ }
+ }
+
+ size_t Hash() const { return hash_code_; }
+
+ std::string Type() const { return type_; }
+
+ bool operator==(const ExpressionCacheKey& other) const {
+ if (type_ != other.type_) {
+ return false;
+ }
+
+ if (hash_code_ != other.hash_code_) {
+ return false;
+ }
+
+ if (!(schema_->Equals(*other.schema_, true))) {
+ return false;
+ }
+
+ if (configuration_ != other.configuration_) {
+ return false;
+ }
+
+ if (expressions_as_strings_ != other.expressions_as_strings_) {
+ return false;
+ }
+
+ if (uniqifier_ != other.uniqifier_) {
+ return false;
+ }
+
+ return true;
+ }
+
+ bool operator!=(const ExpressionCacheKey& other) const { return !(*this == other); }
+
+ private:
+ size_t hash_code_;
+ std::string type_;
Review comment:
Yes, my mistake, when I changed it to Expression Cache Key, I shoud have removed this type indicator. So I removed it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r736267605
##########
File path: cpp/src/gandiva/gandiva_object_cache.h
##########
@@ -71,8 +71,10 @@ class GandivaObjectCache : public llvm::ObjectCache {
if (cached_obj != nullptr) {
std::unique_ptr<llvm::MemoryBuffer> cached_buffer = cached_obj->getMemBufferCopy(
cached_obj->getBuffer(), cached_obj->getBufferIdentifier());
+ ARROW_LOG(INFO) << "[INFO][CACHE-LOG]: An object code was found on cache.";
Review comment:
You shouldn't LOG in header files. Better to move it out to .cc file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r732323264
##########
File path: cpp/src/gandiva/cache.h
##########
@@ -39,17 +39,21 @@ class Cache {
Cache() : Cache(GetCapacity()) {}
- ValueType GetModule(KeyType cache_key) {
+ ::std::shared_ptr<Cache> create(size_t capacity) {
+ return ::std::make_shared<Cache>(cache_(capacity));
Review comment:
Removed this unused function
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713970110
##########
File path: cpp/src/gandiva/engine.h
##########
@@ -54,6 +54,13 @@ class GANDIVA_EXPORT Engine {
functions_to_compile_.push_back(fname);
}
+ /// Set BaseObjectCache.
+ template <class KeyType>
+ Status SetLLVMObjectCache(GandivaObjectCache<KeyType>& object_cache) {
+ execution_engine_->setObjectCache(&object_cache);
+ return Status::OK();
Review comment:
Ok. looks like earlier versions didnot contain this method. Also looking at setObjectCache impl it just sets the member variable so no need to check for error
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714700665
##########
File path: cpp/src/gandiva/engine.h
##########
@@ -54,6 +54,13 @@ class GANDIVA_EXPORT Engine {
functions_to_compile_.push_back(fname);
}
+ /// Set BaseObjectCache.
+ template <class KeyType>
+ Status SetLLVMObjectCache(GandivaObjectCache<KeyType>& object_cache) {
+ execution_engine_->setObjectCache(&object_cache);
+ return Status::OK();
Review comment:
status is not required here. just make it void.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713832264
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -42,7 +43,7 @@ FilterCacheKey::FilterCacheKey(SchemaPtr schema,
expression_as_string_ = expression.ToString();
UpdateUniqifier(expression_as_string_);
arrow::internal::hash_combine(result, expression_as_string_);
- arrow::internal::hash_combine(result, configuration);
+ arrow::internal::hash_combine(result, configuration->Hash());
Review comment:
I thought that configuration being a shared_ptr when using it directly to hash it hashed the memory location value, so as it has a Hash() function, it looked better to use the hash_code that it generates to use on the hash_combine.
Do you think it is better to undo this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714705167
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -78,6 +79,10 @@ std::string FilterCacheKey::ToString() const {
return ss.str();
}
+std::string FilterCacheKey::GetExpressionsAsString() const {
Review comment:
yes I understood that. I meant the name here should be getExpression instead of getExpressions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714700972
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -78,6 +79,10 @@ std::string FilterCacheKey::ToString() const {
return ss.str();
}
+std::string FilterCacheKey::GetExpressionsAsString() const {
Review comment:
nit: GetExpressionAsString?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache only ObjectCode in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713938227
##########
File path: cpp/src/gandiva/llvm_generator.h
##########
@@ -49,10 +51,48 @@ class GANDIVA_EXPORT LLVMGenerator {
static Status Make(std::shared_ptr<Configuration> config,
std::unique_ptr<LLVMGenerator>* llvm_generator);
+ static std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ GetCache();
+
/// \brief Build the code for the expression trees for default mode. Each
/// element in the vector represents an expression tree
Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode);
+ /// \brief Build the code for the expression trees for default mode with a LLVM
+ /// ObjectCache. Each element in the vector represents an expression tree
+ template <class KeyType>
+ Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode,
Review comment:
Ok. It looks like there is code duplication with the other build. Should we remove the other build method if it is not used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache Object Code
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r712991445
##########
File path: cpp/src/gandiva/base_cache_key.h
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <arrow/util/hash_util.h>
+#include <stddef.h>
+
+#include "gandiva/expression.h"
+#include "gandiva/filter.h"
+#include "gandiva/projector.h"
+
+namespace gandiva {
+
+class BaseCacheKey {
+ public:
+ BaseCacheKey(ProjectorCacheKey& key, std::string type) : type_(type) {
+ static const int32_t kSeedValue = 4;
+ size_t key_hash = key.Hash();
+ size_t result_hash = kSeedValue;
+ arrow::internal::hash_combine(result_hash, type);
+ arrow::internal::hash_combine(result_hash, key_hash);
+ hash_code_ = result_hash;
+ schema_ = key.schema();
+ }
+
+ BaseCacheKey(FilterCacheKey& key, std::string type) : type_(type) {
+ static const size_t kSeedValue = 4;
+ size_t key_hash = key.Hash();
+ size_t result_hash = kSeedValue;
+ arrow::internal::hash_combine(result_hash, type);
+ arrow::internal::hash_combine(result_hash, key_hash);
+ hash_code_ = result_hash;
+ schema_ = key.schema();
+ }
+
+ size_t Hash() const { return hash_code_; }
+
+ std::string Type() const { return type_; }
+
+ bool operator==(const BaseCacheKey& other) const {
+ if (hash_code_ != other.hash_code_) {
Review comment:
This is incorrect. You can't just only compare the hashcode.
##########
File path: cpp/src/gandiva/llvm_generator.h
##########
@@ -49,10 +51,48 @@ class GANDIVA_EXPORT LLVMGenerator {
static Status Make(std::shared_ptr<Configuration> config,
std::unique_ptr<LLVMGenerator>* llvm_generator);
+ static std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ GetCache();
+
/// \brief Build the code for the expression trees for default mode. Each
/// element in the vector represents an expression tree
Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode);
+ /// \brief Build the code for the expression trees for default mode with a LLVM
+ /// ObjectCache. Each element in the vector represents an expression tree
+ template <class KeyType>
+ Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode,
Review comment:
Better to add the definition in the cc file.
##########
File path: cpp/src/gandiva/engine.cc
##########
@@ -167,7 +167,6 @@ Status Engine::Make(const std::shared_ptr<Configuration>& conf,
return Status::CodeGenError("Could not instantiate llvm::ExecutionEngine: ",
builder_error);
}
-
Review comment:
nit: better to remove these extraneous changes
##########
File path: cpp/src/gandiva/greedy_dual_size_cache.h
##########
@@ -146,9 +159,25 @@ class GreedyDualSizeCache {
priority_set_.erase(i);
}
+ void evictObject() {
Review comment:
This looks duplicate of evict()
##########
File path: cpp/src/gandiva/llvm_generator.cc
##########
@@ -50,6 +51,23 @@ Status LLVMGenerator::Make(std::shared_ptr<Configuration> config,
return Status::OK();
}
+std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+LLVMGenerator::GetCache() {
+ static Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>> cache;
Review comment:
this line seems is not required
##########
File path: cpp/src/gandiva/greedy_dual_size_cache.h
##########
@@ -146,9 +159,25 @@ class GreedyDualSizeCache {
priority_set_.erase(i);
}
+ void evictObject() {
+ // TODO: inflation overflow is unlikely to happen but needs to be handled
+ // for correctness.
+ // evict item from the beginning of the set. This set is ordered from the
+ // lower priority value to the higher priority value.
+ typename std::set<PriorityItem>::iterator i = priority_set_.begin();
+ // update the inflation cost related to the evicted item
+ inflation_ = (*i).actual_priority;
+ size_t size_to_decrease = map_.find((*i).cache_key)->second.first.size;
+ cache_size_ -= size_to_decrease;
+ map_.erase((*i).cache_key);
+ priority_set_.erase(i);
+ }
+
map_type map_;
std::set<PriorityItem> priority_set_;
uint64_t inflation_;
size_t capacity_;
+ size_t cache_size_ = 0;
+ llvm::SmallString<128> cache_dir_;
Review comment:
looks like this is not getting used.
##########
File path: cpp/src/gandiva/llvm_generator.cc
##########
@@ -50,6 +51,23 @@ Status LLVMGenerator::Make(std::shared_ptr<Configuration> config,
return Status::OK();
}
+std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+LLVMGenerator::GetCache() {
+ static Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>> cache;
+ // static std::unique_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ // cache_unique = std::make_unique<Cache<BaseCacheKey,
+ // std::shared_ptr<llvm::MemoryBuffer>>>();
+
+ // static std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ // shared_cache = std::move(cache_unique);
Review comment:
remove the commented out code?
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -120,17 +138,27 @@ Status Filter::Make(SchemaPtr schema, ConditionPtr condition,
ARROW_RETURN_NOT_OK(expr_validator.Validate(condition));
// Start measuring build time
- auto begin = std::chrono::high_resolution_clock::now();
- ARROW_RETURN_NOT_OK(llvm_gen->Build({condition}, SelectionVector::Mode::MODE_NONE));
+ // auto begin = std::chrono::high_resolution_clock::now();
Review comment:
remove the commented out code if no longer required
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -42,7 +43,7 @@ FilterCacheKey::FilterCacheKey(SchemaPtr schema,
expression_as_string_ = expression.ToString();
UpdateUniqifier(expression_as_string_);
arrow::internal::hash_combine(result, expression_as_string_);
- arrow::internal::hash_combine(result, configuration);
+ arrow::internal::hash_combine(result, configuration->Hash());
Review comment:
why this change?
##########
File path: cpp/src/gandiva/projector.h
##########
@@ -138,6 +167,7 @@ class GANDIVA_EXPORT Projector {
SchemaPtr schema_;
FieldVector output_fields_;
std::shared_ptr<Configuration> configuration_;
+ bool compiled_from_cache_;
Review comment:
nit: use a different name like built_from_cache since it is already compiled
##########
File path: cpp/src/gandiva/greedy_dual_size_cache.h
##########
@@ -92,25 +96,26 @@ class GreedyDualSizeCache {
bool contains(const Key& key) { return map_.find(key) != map_.end(); }
- void insert(const Key& key, const ValueCacheObject<Value>& value) {
+ void InsertObjectCode(const Key& key, const ValueCacheObject<Value>& value) {
Review comment:
nit: original name insert seems better since this is a generic class.
##########
File path: cpp/src/gandiva/llvm_generator.h
##########
@@ -49,10 +51,48 @@ class GANDIVA_EXPORT LLVMGenerator {
static Status Make(std::shared_ptr<Configuration> config,
std::unique_ptr<LLVMGenerator>* llvm_generator);
+ static std::shared_ptr<Cache<BaseCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ GetCache();
+
/// \brief Build the code for the expression trees for default mode. Each
/// element in the vector represents an expression tree
Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode);
+ /// \brief Build the code for the expression trees for default mode with a LLVM
+ /// ObjectCache. Each element in the vector represents an expression tree
+ template <class KeyType>
+ Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode,
+ GandivaObjectCache<KeyType>& obj_cache) {
+ selection_vector_mode_ = mode;
+
+ // Start measuring code gen time
+ auto begin = std::chrono::high_resolution_clock::now();
+ for (auto& expr : exprs) {
+ auto output = annotator_.AddOutputFieldDescriptor(expr->result());
+ ARROW_RETURN_NOT_OK(Add(expr, output));
+ }
+
+ // Stop measuring time, calculate the elapsed time and pass it to object cache
+ auto end = std::chrono::high_resolution_clock::now();
+ size_t elapsed =
Review comment:
This is wrong. the elapse time was supposed to track the compilation time which is mainly done in FinalizeModule
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache Object Code
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#issuecomment-923424831
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache Object Code
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r713594481
##########
File path: cpp/src/gandiva/tests/filter_test.cc
##########
@@ -29,7 +29,11 @@ using arrow::int32;
class TestFilter : public ::testing::Test {
public:
- void SetUp() { pool_ = arrow::default_memory_pool(); }
+ void SetUp() {
+ pool_ = arrow::default_memory_pool();
+ // Setup arrow log severity threshold to debug level.
+ arrow::util::ArrowLog::StartArrowLog("", arrow::util::ArrowLogLevel::ARROW_DEBUG);
Review comment:
is this change required?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on pull request #11193: ARROW-14048: [C++][Gandiva] Implement Gandiva to Cache Object Code
Posted by GitBox <gi...@apache.org>.
projjal commented on pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#issuecomment-924586629
> Implement Gandiva to cache object code instead the entire llvm module, improving the memory consumption and LLVM time perfomance.
This should not have any effect on llvm performance only the memory footprint. Can you also include the numbers in the description.
Also the wording in the description and pr title seems vague. change it to something like "Cache only ObjectCode in memory instead of entire module"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r734101093
##########
File path: cpp/src/gandiva/gandiva_object_cache.h
##########
@@ -0,0 +1,84 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#if defined(_MSC_VER)
+#pragma warning(push)
+#pragma warning(disable : 4244)
+#pragma warning(disable : 4141)
+#pragma warning(disable : 4146)
+#pragma warning(disable : 4267)
+#pragma warning(disable : 4624)
+#endif
+
+#include <llvm/ExecutionEngine/ObjectCache.h>
+#include <llvm/Support/MemoryBuffer.h>
+
+#include <chrono>
+
+#include "expression_cache_key.h"
+#include "gandiva/cache.h"
+
+namespace gandiva {
+/// Class that enables the LLVM to use a custom rule to deal with the object code.
+class GandivaObjectCache : public llvm::ObjectCache {
+ public:
+ explicit GandivaObjectCache(
+ std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>& cache,
+ ExpressionCacheKey key) : cache_key_(key) {
+ cache_ = cache;
+ // Start measuring code gen time
+ begin_time_ = std::chrono::high_resolution_clock::now();
+ }
+
+ ~GandivaObjectCache() {}
+
+ void notifyObjectCompiled(const llvm::Module* M, llvm::MemoryBufferRef Obj) {
+ // Stop measuring time and calculate the elapsed time to compile the object code
+ auto end_time = std::chrono::high_resolution_clock::now();
+ auto elapsed_time =
+ std::chrono::duration_cast<std::chrono::milliseconds>(end_time - begin_time_)
+ .count();
+
+ std::unique_ptr<llvm::MemoryBuffer> obj_buffer =
+ llvm::MemoryBuffer::getMemBufferCopy(Obj.getBuffer(), Obj.getBufferIdentifier());
+ std::shared_ptr<llvm::MemoryBuffer> obj_code = std::move(obj_buffer);
+
+ ValueCacheObject<std::shared_ptr<llvm::MemoryBuffer>> value_cache(
+ obj_code, elapsed_time, obj_code->getBufferSize());
+
+ cache_->PutObjectCode(cache_key_, value_cache);
+ }
+
+ std::unique_ptr<llvm::MemoryBuffer> getObject(const llvm::Module* M) {
+ std::shared_ptr<llvm::MemoryBuffer> cached_obj =
+ cache_->GetObjectCode(cache_key_);
Review comment:
Added the logs on the above scenarios that you asked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r761163468
##########
File path: cpp/src/gandiva/cache.h
##########
@@ -39,17 +39,17 @@ class Cache {
Cache() : Cache(GetCapacity()) {}
- ValueType GetModule(KeyType cache_key) {
+ ValueType GetObjectCode(KeyType cache_key) {
arrow::util::optional<ValueCacheObject<ValueType>> result;
mtx_.lock();
result = cache_.get(cache_key);
mtx_.unlock();
return result != arrow::util::nullopt ? (*result).module : nullptr;
}
- void PutModule(KeyType cache_key, ValueCacheObject<ValueType> valueCacheObject) {
+ void PutObjectCode(KeyType& cache_key, ValueCacheObject<ValueType> object_code) {
Review comment:
pass the cacheobject also as reference. same for GetObjectCode parameter
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#issuecomment-985454352
Benchmark runs are scheduled for baseline = a6c81e621b637479a5828c4bdb95ec442ad94bb5 and contender = 188c94a9820ef3377573b835c9dc9dcb96ceafe4. 188c94a9820ef3377573b835c9dc9dcb96ceafe4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/f0dc01b023534d96b0bac4d36ea14c3e...5b059cee5d844774aae1f958873691eb/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/602e996a876949659442f3c145b84f06...f1aa0fd8f0a140058010e12f7a096f3f/)
[Finished :arrow_down:0.22% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/5c7ec03f40ac4f25a089b656edc6d4b8...721a52173d9848eb8dbecfa19fe61d4d/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] projjal commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Make Gandiva to cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r714699670
##########
File path: cpp/src/gandiva/cache.cc
##########
@@ -20,10 +20,10 @@
namespace gandiva {
-static const int DEFAULT_CACHE_SIZE = 500;
+static const size_t DEFAULT_CACHE_SIZE = 128 * 1024 * 1024; // 256 MiB
-int GetCapacity() {
- int capacity;
+size_t GetCapacity() {
+ size_t capacity;
const char* env_cache_size = std::getenv("GANDIVA_CACHE_SIZE");
Review comment:
rename the constant as GANDIVA_CACHE_SIZE_BYTES (otherwise it will pick up the earlier set value on existing machines)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r732339275
##########
File path: cpp/src/gandiva/llvm_generator.h
##########
@@ -49,14 +51,41 @@ class GANDIVA_EXPORT LLVMGenerator {
static Status Make(std::shared_ptr<Configuration> config,
std::unique_ptr<LLVMGenerator>* llvm_generator);
- /// \brief Build the code for the expression trees for default mode. Each
- /// element in the vector represents an expression tree
- Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode);
+ static std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ GetCache();
+
+ /// \brief Build the code for the expression trees for default mode with a LLVM
+ /// ObjectCache. Each element in the vector represents an expression tree
+ template <class KeyType>
+ Status Build(const ExpressionVector& exprs, SelectionVector::Mode mode,
+ GandivaObjectCache<KeyType>& obj_cache) {
+ selection_vector_mode_ = mode;
+
+ for (auto& expr : exprs) {
+ auto output = annotator_.AddOutputFieldDescriptor(expr->result());
+ ARROW_RETURN_NOT_OK(Add(expr, output));
+ }
+
+ engine_->SetLLVMObjectCache(obj_cache);
Review comment:
Ok, applied your suggestion, by rolling back to previous implementation and adding SetLLVMObjectCache to llvm_generator
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #11193: ARROW-14048: [C++][Gandiva] Cache only object code in memory instead of entire module
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11193:
URL: https://github.com/apache/arrow/pull/11193#discussion_r732331940
##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -102,14 +46,30 @@ Status Filter::Make(SchemaPtr schema, ConditionPtr condition,
ARROW_RETURN_IF(configuration == nullptr,
Status::Invalid("Configuration cannot be null"));
- static Cache<FilterCacheKey, std::shared_ptr<Filter>> cache;
- FilterCacheKey cache_key(schema, configuration, *(condition.get()));
- auto cachedFilter = cache.GetModule(cache_key);
- if (cachedFilter != nullptr) {
- *filter = cachedFilter;
- return Status::OK();
+ std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>
+ shared_cache = LLVMGenerator::GetCache();
+
+ Condition conditionToKey = *(condition.get());
+
+ ExpressionCacheKey cache_key(schema, configuration, conditionToKey, "filter");
+ std::unique_ptr<ExpressionCacheKey> base_cache_key =
+ std::make_unique<ExpressionCacheKey>(cache_key);
+ std::shared_ptr<ExpressionCacheKey> shared_base_cache_key = std::move(base_cache_key);
+
+ bool llvm_flag = false;
Review comment:
Ok, changed to "is_cached"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org