-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix JSONPath cache inefficient issue #7409
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7409 +/- ##
============================================
- Coverage 71.54% 71.51% -0.03%
+ Complexity 4036 4033 -3
============================================
Files 1579 1580 +1
Lines 80390 80386 -4
Branches 11945 11944 -1
============================================
- Hits 57512 57489 -23
- Misses 18996 19012 +16
- Partials 3882 3885 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
public void testSimpleJsonPathMapCacheWorks() throws JsonProcessingException { | ||
String path = "$.contact.email"; | ||
assertEquals(JsonFunctions.jsonPathString(_jsonString, path), "test@example.com"); | ||
JsonPathMapCache cache = (JsonPathMapCache) CacheProvider.getCache(); | ||
|
||
// verify json path has been cached | ||
LinkedList<Predicate> filterStack = new LinkedList<>(Collections.emptyList()); | ||
String cacheKey = Utils.concat(path, filterStack.toString()); | ||
JsonPath jsonPath = cache.get(cacheKey); | ||
assertNotNull(jsonPath); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This depends on undocumented library behaviour, it assumes the format of the cache keys. I have actually submitted a PR to the library to change this behaviour, which would break this test: json-path/JsonPath#750
I suggest iterating over the map instead (requires exposing the keys on JsonPathMapCache
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or just use a Mockito spy and verify put
was called, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestions.
I always prefer to use Mockitio for unit testing, but all method calls related to JSONFunctions are static, so I try to check the map size now.
pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathMapCache.java
Outdated
Show resolved
Hide resolved
* of it. | ||
*/ | ||
public class JsonPathMapCache implements Cache { | ||
private final ConcurrentMap<String, JsonPath> _pathCache = new ConcurrentHashMap<>(128, 0.75f, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to explicitly set the concurrency level or load factor, these are the default values any way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change it. This is the habit of my previous team!
* the number of JSON paths is bounded by the size of the transformation config, | ||
* add this threshold for protection from the extreme cases. | ||
*/ | ||
public static final int MAX_JSON_PATH_CACHE_SIZE = 1024 * 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class should be removed and the constant inlined into JsonPathMapCache
where it is relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an approver but this looks good to me, and solves a big problem 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise
pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathMapCache.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathMapCache.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/function/JsonPathMapCache.java
Outdated
Show resolved
Hide resolved
* a lot of unnecessary lock waits during high concurrent data ingestion, | ||
* and LRU mechanism is inappropriate for Pinot bounded size of the | ||
* transformation config, so we should use this simple Map cache instead | ||
* of it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsonPath can also be used at query time (see JsonExtractScalarTransformFunction
). Let's add a TODO here to add some evict policy in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the future this can be handled better by just precompiling JsonPath
objects and bypassing the library's cache entirely. For transformConfig, it would require changes to the way ScalarFunction
is defined as well as changes to InbuiltFunctionEvaluator.planExecution
(this would also permit validation and cleansing of config, which is currently impossible). For JsonExtractScalarTransformFunction
the change is trivial.
I think in the meantime using a guava Cache
instead of a ConcurrentHashMap
as suggested on slack would alleviate this concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think guava Cache
is a more safe cache solution cause the JsonPath
cache will also be applied to the query.
For one of my table ingestion cases, reading data QPS close to 10w and configuring JsonPath
transformations with dozens of fields, this will put a certain pressure on GC and CPU because of the LRU algorithm needs to record every get
. Of course, this should not become a system bottleneck, but the implementation is a bit too heavy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guava Cache uses recencyQueue
to record cache accesses. A large number of CAS
enqueue operations generated by hotkeys will cause CPU consumption, and this synchronous recording will also slow down cache reading.
I think maybe Caffeine Cache is a good choice. Its Striped-RingBuffer design is more efficient for GC and CPU usage. And its API is very similar to Guava Cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether using Guava's cache would result in high CPU consumption needs measurement. I agree that Caffeine is a better cache implementation, but Guava is already on the classpath. Is this particular use case important enough to add a 1MB depedency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use the default value of the concurrencyLevel
of Guava cache
. In this scenario, there will be very few caches writes and no need to update the cache. My concern is the way Guava records cache reads, which is not very efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The performance of cache reads improves with the concurrency level due to reducing the likelihood of contending on the same CLQ instance. The benchmarks use a zipf to simulate a hotspot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the insights @ben-manes. FWIW I have been using Caffeine in commercial projects for years, the only question here is whether this particular use case is worth adding a relatively large (in bytes) dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a problem. I co-authored Guava's so either way my code is used. 😄
@Ferrari6 is correct in what your preference and default should be, though. Prior to my involvement, the Guava team bet on reference caching (MapMaker) and spent their complexity budget by forking the hash table as an optimization. This was a mistake because soft references can cause GC death spirals of stop the world events and unpredictable evictions, but looked fine in a naive benchmark. This blew their complexity budget, so porting size eviction from CLHM favored simplicity over performance.
The longer-term problem you'll face is that no one maintains CacheBuilder. The last big change was adding Map.compute, but it was riddled with major bugs and done inefficiently. I fixed some of those problems, but there are show stoppers. If you keep to the historic functionality then Guava's can be made acceptable in most cases. Caffeine does have jar bloat by code generating per-configuration entry classes to minimize the memory footprint. In those cases where disk is a premium some projects embed CLHM into their code base, e.g. msjdbc and groovy. You'll probably have many caches throughout Pinot making the Caffeine dependency worthwhile even if you can get by in a case-by-case basis.
yes, sure. But because the setCache of |
Both |
8dd48c4
to
3166fae
Compare
Some tests failed, rerunning to see if intermittent issue due to timeouts. |
3166fae
to
2be13d1
Compare
Thanks @mayankshriv. I have fixed the failed tests. |
w
@Ferrari6 seems like there are still failing tests. |
740572f
to
e78ac2c
Compare
Addressed comments to improve unit test Addressed review comments - Remove Constants class and configure inlined into JsonPathMapCache - Remove useless ConcurrentHashMap parameters Addressed review comments Co-authored-by: Xiaotian (Jackie) Jiang <17555551+Jackie-Jiang@users.noreply.github.com> Change cache maximum size to reduce memory usage use Guava cache for json path add json path cache integration test
e78ac2c
to
c03156c
Compare
Fix the issue of setting different default configurations Do not access the JsonPath cache in the transform function
c03156c
to
fd7aed0
Compare
This reverts commit 14c377d.
This reverts commit 14c377d.
Also fix the issue of setting different default configurations
Description
this commit fixes #7403
Background
stack trace
**flamegraphs can be found in the issue descriptions #7403 **
Fix
A new JSON path cache is implemented using ConcurrentHashMap, and the cache threshold is set at the same time. When the maximum is exceeded, the JSON path will not be cached anymore.
Pinot Server Flamegraphs when using ConcurrentHashMap cache (28vcpu)
jsonpath CPU usage is low and no lock contentions
-->
Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
backward-incompat
, and complete the section below on Release Notes)Does this PR fix a zero-downtime upgrade introduced earlier?
backward-incompat
, and complete the section below on Release Notes)Does this PR otherwise need attention when creating release notes? Things to consider:
release-notes
and complete the section on Release Notes)Release Notes
Documentation