GH-40040: [C++][Gandiva] Make Gandiva's default cache size to be 5000 for object code cache#40041
Conversation
|
|
461e85f to
cc9cc6d
Compare
pitrou
left a comment
There was a problem hiding this comment.
Looks like an obvious fix, thank you
Taking a closer look at the size calculation, this may not be desirable.
pitrou
left a comment
There was a problem hiding this comment.
Sorry for the earlier review. It seems this PR misunderstands the cache size. 500 cache entries is actually not that bad.
cpp/src/gandiva/cache.cc
Outdated
There was a problem hiding this comment.
There's a problem here: what is the unit? The large number seems to imply this is expressed in bytes.
However, the size will actually be interpreted as the number of stored entries (it's passed to LruCache which in turn compares it to std::unordered_map::size).
It would probably be beneficial to cap the number of bytes and not the number of entries, but this is not what this PR does. 500000 cache entries may easily add up to hundreds of megabytes of RAM.
There was a problem hiding this comment.
Indeed, the unit in question pertains to the number of entries rather than bytes, but there's no misunderstanding of the cache size and the original value of 500 was also defined in terms of the number of entries. This PR aims to address an oversight by removing a previously missed flag.
This link [1] has more data about module cache vs. object code cache in Gandiva, and in the limited expressions tested, the memory is down to 0.8%~6% after using object code cache.
The current default value 500 is likely too small in production, and it will probably only use 8MB of memory using the stats in link [1], but you are right it seems 500000 is indeed too large (I have no idea how it was defined this way and I should do more calculation for it). According to the stats in link [1], when using module cache, it will probably take 750MB memory for 500 entries.
Other initiatives [2] have attempted to enhance the cache eviction algorithm, but such attempts were reversed due to other issues [3]. I've previously reviewed these efforts and believe I have distinct ideas for advancing Gandiva's cache. I plan to propose a PR if I fully understand the workflow. To avoid complicating this PR, my goal is solely to refine the default value. Would it be acceptable to adopt a more conservative default value, such as 5000/10000? Thanks.
[1] #11193 (comment)
[2] #10465
[3] #11957
|
cc @js8544 |
|
Happy Chinese New Year! @pitrou I think this PR is to fix the regression introduced in #12742 rather than a feature. #12742 sets GANDIVA_ENABLE_OBJECT_CODE_CACHE default to true but forget to change the default cache size. @niyue Is my understanding correct? However I do agree that we can further investigate the size limit of the cache. 500000 objects should require too much memory. Suppose one expression takes 1KB (my wild guess), that would be 500MB. |
|
Yes, 5000 would be much better here.
Le 13 février 2024 11:45:25 GMT+01:00, Yue ***@***.***> a écrit :
…
@niyue commented on this pull request.
> @@ -23,11 +23,7 @@
namespace gandiva {
-#ifdef GANDIVA_ENABLE_OBJECT_CODE_CACHE
static const size_t DEFAULT_CACHE_SIZE = 500000;
Indeed, the unit in question pertains to the number of entries rather than bytes, but there's no misunderstanding of the cache size and the original value of `500` was also defined in terms of the number of entries. This PR aims to address an oversight by removing a previously missed flag.
This link [[1]](#11193 (comment)) has more data about module cache vs. object code cache in Gandiva, and in the limited expressions tested, the memory is down to 0.8%~6% after using object code cache.
The current default value 500 is likely too small in production, and it will probably only use 8MB of memory using the stats in link [1], but you are right it seems 500000 is indeed too large (I have no idea how it was defined this way and I should do more calculation for it). According to the stats in link [1], when using module cache, it will probably take 750MB memory for 500 entries.
Other initiatives [2] have attempted to enhance the cache eviction algorithm, but such attempts were reversed due to other issues [3]. I've previously reviewed these efforts and believe I have distinct ideas for advancing Gandiva's cache. I plan to propose a PR if I fully understand the workflow. To avoid complicating this PR, my goal is solely to refine the default value. Would it be acceptable to adopt a more conservative default value, such as `5000`/`10000`?
[1] #11193 (comment)
[2] #10465
[3] #11957
--
Reply to this email directly or view it on GitHub:
#40041 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
cc9cc6d to
201dcf8
Compare
… address the previous oversight after switching to object code cache.
201dcf8 to
41eaf6a
Compare
|
Thank you @niyue ! |
|
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 967831b. There was 1 benchmark result with an error:
There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
…e 5000 for object code cache (apache#40041) ### Rationale for this change Gandiva's default cache is object code cache, however, the default cache size is still the old value for LLVM module based cache, which is too small. More details about the `GANDIVA_ENABLE_OBJECT_CODE_CACHE` flag can be found in apacheGH-40040 ### What changes are included in this PR? Remove the unused `GANDIVA_ENABLE_OBJECT_CODE_CACHE` flag and make the default cache size to be `500000` for object code cache. ### Are these changes tested? No ### Are there any user-facing changes? Yes, default cache size will be changed from 500 to 500000, and it may help the default deployment's performance. * Closes: apache#40040 Authored-by: Yue Ni <niyue.com@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
|
How would one go about configuring the cache size via something other than environment variable? |
Rationale for this change
Gandiva's default cache is object code cache, however, the default cache size is still the old value for LLVM module based cache, which is too small.
More details about the
GANDIVA_ENABLE_OBJECT_CODE_CACHEflag can be found in GH-40040What changes are included in this PR?
Remove the unused
GANDIVA_ENABLE_OBJECT_CODE_CACHEflag and make the default cache size to be500000for object code cache.Are these changes tested?
No
Are there any user-facing changes?
Yes, default cache size will be changed from 500 to 500000, and it may help the default deployment's performance.