[DS-108][DS-109] Cache object code on memory and enable memory tracking #6

augustoasilva · 2021-06-21T23:25:06Z

The task DS-108 and DS-109 was pushed together because they complement each other

cpp/src/gandiva/base_cache_key.h

cpp/src/gandiva/cache.cc

cpp/src/gandiva/tests/projector_test.cc

anthonylouisbsb · 2021-06-24T01:50:54Z

I noticed that you changed some asserts in existing tests, but I did not see any new tests. The existing ones already cover all changes you made?

anthonylouisbsb · 2021-06-24T01:51:45Z

Also, I noticed some formatting errors in your code, do not forget to run the cpp code formatted before open the PR to the main repository

projjal · 2021-09-08T08:11:57Z

ci/conda_env_cpp.txt

@@ -18,7 +18,7 @@
 # workaround for https://issues.apache.org/jira/browse/ARROW-13134
 aws-sdk-cpp<1.9
 benchmark>=1.5.4
-boost-cpp>=1.68.0
+boost-cpp>=1.68.0,<1.77.0


why is this change required?

Because in one of the builds, the version 1.77 will break as some of the boost libs on v1.77 will use C++17.

you should fix the build. This change won't be allowed in master i think. and there are plans to move to c++17 anyway

@projjal the error was:

/opt/conda/envs/arrow/include/boost/process/detail/traits/wchar_t.hpp: In static member function 'static std::vector<std::__cxx11::basic_string<wchar_t> > boost::process::detail::char_converter<wchar_t, std::vector<std::__cxx11::basic_string<char> > >::conv(const std::vector<std::__cxx11::basic_string<char> >&)': /opt/conda/envs/arrow/include/boost/process/detail/traits/wchar_t.hpp:150:14: error: 'transform' is not a member of 'std' 150 | std::transform(in.begin(), in.end(), ret.begin(),

You can see it here: https://github.com/s1mbi0se/arrow/runs/3500727130

doesn't the failure look like something to do with arrow-flight?

I don't know, but when I was searching about the error itself and the transform in the STL, I discovered that the cause of this issue was about the C++17 version, and looking at the Cmake files I saw that arrow fully support by default the C++11.

Also, limiting the maximum boost version to up to 1.76 version does not appear to me to be a big issue.

So, what approach do you recomed to do?

You can't change these settings without a proper reasoning. The run you linked runs with c++17 so i believe its expected to pass with c++17. For now first find the root cause why its failing. If it is not related to this pr then it should fail in master too

@projjal I updated again with master, and this error was gone, so I changed back this file to the master version

projjal · 2021-09-08T08:58:01Z

cpp/src/gandiva/projector.h

@@ -138,6 +168,8 @@ class GANDIVA_EXPORT Projector {
  SchemaPtr schema_;
  FieldVector output_fields_;
  std::shared_ptr<Configuration> configuration_;
+  bool compiled_from_cache_;
+  static size_t used_cache_size_;


why is it static?

This is static so all the projector will use the same var to keep track of all projectors' memory consumption. But now, I'm thinking to remove it and only keep tracking of the global used memory by the cache, instead of both tracking.

Removed it as it will no longer be used.

projjal · 2021-09-08T09:25:46Z

cpp/src/gandiva/greedy_dual_size_cache.h

@@ -110,6 +114,25 @@ class GreedyDualSizeCache {
    }
  }

+  void InsertObjectCode(const Key& key, const ValueCacheObject<Value>& value) {


why are you adding these new methods?

These methods was added just to preserve the old ones so I can benchmak switching between them instead of switching the project branchs, I plan to change the old ones when moving the poc to the arrow PR.

Can you remove them now since you already benchmarked?

projjal · 2021-09-08T09:27:39Z

cpp/src/gandiva/cache.cc

  const char* env_cache_size = std::getenv("GANDIVA_CACHE_SIZE");
  if (env_cache_size != nullptr) {
-    capacity = std::atoi(env_cache_size);
+    capacity = std::stoul(env_cache_size);


why is this change required?

In atoi, if no conversion happens, because of an invalid argument or it falls out of range, the value will assume the zero value, in stoul if there is a problem it will throw an excpetion leading to a better error management.

atoi was specifically added because it doesn't throw exception. The zero case is handled in next line.
Also in this change you are not catching the exception so the change is incorrect. Better just undo this change.

Ok, I changed it back.

projjal · 2021-09-08T09:48:15Z

cpp/src/gandiva/projector.cc

 #include "gandiva/llvm_generator.h"

 namespace gandiva {

-class ProjectorCacheKey {


any reason why moved this class to projector.h?

Because it was giving me some circular dependency error, so to avoid it I moved it to the projector.h. The FilterCacheKey also is in the filter.h, so I thought it was no issue to move ProjectorCacheKey to projector.h

The circular error was, when using the ProjectorCacheKey into BaseCacheKey, and them using the BaseCacheKey into Projector, as ProjectorCacheKey was in the projector.cc file it became a circular dependency when I called to import projector.cc into base_cache_key, and the in projector.cc import the base_cache_key file

projjal · 2021-09-08T09:48:39Z

cpp/src/gandiva/engine.cc

+    ARROW_LOG(WARNING) << "[OBJ-CACHE-LOG][ERROR]: "
+                       << execution_engine_->getErrorMessage();
+    module_finalized_ = false;
+    return Status::OK();


why return status::OK in case of error?

My mistake, sorry, I will fix that.

projjal · 2021-09-08T11:11:57Z

cpp/src/gandiva/base_cache_key.h

+#include <arrow/util/hash_util.h>
+#include <stddef.h>
+
+#include <boost/lexical_cast.hpp>


why is it needed here

It is not, I was trying an approach to use boost::any for the cache, and forgot to clean this one to. I already fixed.

…ue cache instance

This was referenced Jun 21, 2021

[DS-110] Cache individual expressions instead of all expressions in projector #7

Open

[DS-146] Enabling caching to disk #8

Open

augustoasilva requested a review from anthonylouisbsb June 23, 2021 10:34

augustoasilva self-assigned this Jun 23, 2021

anthonylouisbsb suggested changes Jun 24, 2021

View reviewed changes

augustoasilva force-pushed the feature/cache-object-code-on-memory-and-enable-memory-tracking branch from e9505c3 to 6474465 Compare September 1, 2021 22:48

github-actions bot added gandiva lang-c++ labels Sep 1, 2021

augustoasilva requested a review from anthonylouisbsb September 1, 2021 22:50

augustoasilva force-pushed the feature/cache-object-code-on-memory-and-enable-memory-tracking branch from 7a65334 to c6ed351 Compare September 2, 2021 23:53

projjal reviewed Sep 8, 2021

View reviewed changes

augustoasilva added 13 commits September 8, 2021 20:47

Add SetLlvmObjectcache function

6fd426e

Add the GandivaObjectCache class

f3f48e7

Create new builder to cache the object code on memory and set an uniq…

148be9a

…ue cache instance

Fix conflicts from master

337b6ef

Fix conflicts from master

06827e4

Remove unused debug logs

24a5a10

Fix conflicts from master on filter

c559bf6

Comment test env var configuration

ff01008

Remove unecessary comments

b54e3f1

Remove more unnecessary comments

7522bc8

Fix conflicts from master on cache and projector

c12b7c3

Add comment to GandivaObjectCache class

9c29e8d

Fix conflicts from master on obj_cache and filter

780a164

augustoasilva added 18 commits September 8, 2021 20:47

Fix conflicts from master on filter

604acbf

Update base_cache_key.h as per reviewed

b1c5fa0

Fix conflicts from master on filter and projector

b998c89

Update engine.h and engine.cc as per reviewed

730582b

Fix conflicts from master on filter projector and cache

3101f04

Update obj-cache to new cache policy

76491f3

Fix base cache key build errors

ae09512

Remove boost::any from dependencies

86a08e9

Remove unused imports and functions from base cache key

f75cf07

Fix linting errors manually

7126b65

Fix linting errors manually on gandiva_object_cache.h

6242b31

Fix nullptr usage on gandiva_object_cache.h

548d3e7

Change conda env cpp to limit boost-cpp max version

8324575

Fix finalize module returning ok when execution engine has error

dba8d70

Remove static used_cache_size var from projector and filter

c57f98a

Change back the stoul to atoi

170f381

Fix linx error

17d5e3c

Remove unnused imports on base cache key

553e86c

augustoasilva force-pushed the feature/cache-object-code-on-memory-and-enable-memory-tracking branch from 61aadbc to 553e86c Compare September 8, 2021 23:48

Undo conda_env_cpp.txt boost-cpp lib modification

3fa6594

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DS-108][DS-109] Cache object code on memory and enable memory tracking #6

[DS-108][DS-109] Cache object code on memory and enable memory tracking #6

augustoasilva commented Jun 21, 2021

anthonylouisbsb commented Jun 24, 2021

anthonylouisbsb commented Jun 24, 2021

projjal Sep 8, 2021

augustoasilva Sep 8, 2021

projjal Sep 8, 2021

augustoasilva Sep 8, 2021 •

edited

Loading

projjal Sep 8, 2021

augustoasilva Sep 8, 2021

projjal Sep 8, 2021

augustoasilva Sep 9, 2021

projjal Sep 8, 2021

augustoasilva Sep 8, 2021

augustoasilva Sep 8, 2021

projjal Sep 8, 2021

augustoasilva Sep 8, 2021 •

edited

Loading

projjal Sep 21, 2021

projjal Sep 8, 2021

augustoasilva Sep 8, 2021 •

edited

Loading

projjal Sep 8, 2021

augustoasilva Sep 8, 2021

projjal Sep 8, 2021

augustoasilva Sep 8, 2021 •

edited

Loading

projjal Sep 8, 2021

augustoasilva Sep 8, 2021

augustoasilva Sep 8, 2021

projjal Sep 8, 2021

augustoasilva Sep 8, 2021

[DS-108][DS-109] Cache object code on memory and enable memory tracking #6

Are you sure you want to change the base?

[DS-108][DS-109] Cache object code on memory and enable memory tracking #6

Conversation

augustoasilva commented Jun 21, 2021

anthonylouisbsb commented Jun 24, 2021

anthonylouisbsb commented Jun 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustoasilva Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustoasilva Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustoasilva Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustoasilva Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustoasilva Sep 8, 2021 •

edited

Loading

augustoasilva Sep 8, 2021 •

edited

Loading

augustoasilva Sep 8, 2021 •

edited

Loading

augustoasilva Sep 8, 2021 •

edited

Loading