FWD » Core Development » Database

The code in PreselectQuery.assembleHQL appears in the profiler when testing very complex application APIs, in 6129a. In that case, it is called ~20k times, and it creates lots of objects. We need to make this more efficient both in terms of CPU speed and allocated objects.

For an initial check, I recommend implementing a very small suite of performance tests (~6) for PreselectQuery to test different aspects of HQL composition. Use ~10k preselect queries with very low iteration count (~2). Use temp-tables in:

• 1 test with high number of selected fields (one big table, small where, no index)
• 1 test with several joins (multiple small tables, small where, no indexes)
• 2 tests with a large WHERE clause
  • one small table, large where clause, no index
  • one small table, where clause with placeholders (?), no index
• 1 test with a large ORDER BY clause (some small tables, small where, multiple large indexes)
• 1 overall test (multiple tables, large where clause, multiple large indexes)

For each of these tests, record the QUERY_ASSEMBLE JMX average time output. This should work as a baseline for your further improvements. Note that VisualVM can also hint the hotspots.

• Phase 1: first, the program will check if the hql can be retrieved immediately from the cache (all at once). If so, then there is no need to move to phase 2.
• Phase 2: the program will check every function/method of the 5 (assembleOrderByClause, assembleSelectClause, etc.) and see if their result can be retrieved from the cache. The ones that cannot get their result from cache will move to phase 3.
• Phase 3: call the function/method normally and store the result in the cache.

Let me know if you think this is a good idea and/or if you would change some aspects in the thought process. Thank you!

Radu, can you port this patch to 6129b? I'd like to test the impact on a customer's application.

Unfortunately the patch shows a slowdown in the performance testing. I'm not sure from where it is. I wonder if is not related to many cache misses (too small cache size), I'll check a heap dump and see what the cache reports.

• in cache.Cache.computeIfAbsent, parameters need to be on the same line, if they fit in the max line limit
• in ExpiryCache.computeIfAbsent, you have lines like if ((v = get(key)) == null) - v is never even really used, and also including the assignment in the IF test just makes the code less readable. Same with Objects.requireNonNull(mappingFunction);, I don't think is needed.
• in SortCriterion, you have a Vector toStringCache - why not use instead directly a Java array? As the array from the look of it doesn't need to grow, is fixed size.
• in SortCriterionFactory:
  • the JPRM column from the header needs to be removed.
  • the cache field can be private

I have a mod count for the LRUCache.cache map (used by SortCrtierionFactory) of 11k for just 10 runs of the performance tests. So this just adds overhead, there are lots of cache misses - and it might not be solved just by increasing the cache size.

Please check how the cache behaves when you have (the same) temp-table created dynamically (with different temp-table names, just the fields are the same) and after that used in dynamic queries. Also, does SortCrtierionFactory$Key.text have the columns prefixed with the DMO alias?

Is the profiling done with multiple users? The cache is global across all users, so it is a synchronized data structure. Maybe this can lead to some overhead?

Also, there are two "somehow independent" optimizations: sort criterion factory and SortCriterion toString caching. Maybe we can integrate them separately. Clearly the "toString" optimization is more powerful along the factory, but anyways.

Alexandru Lungu wrote:

Is the profiling done with multiple users? The cache is global across all users, so it is a synchronized data structure. Maybe this can lead to some overhead?

No, the profiling is done with a single user, which executes the tests sequentially.

I've increased the cache limit to something big (2048000) and now for 100 runs the average is only a little less worse (11513ms vs 11435ms without this patch, when previously the patch had 11621ms). The actual number of entries in the cache is 12590.

Radu: please think of a way to stress-test this, and find what can be improved. Maybe doing some profiling when populating the cache with 10k entries (and using some 100s of queries which call assembleHQL 20k times) can help.

Also, the cache Key doesn't need the dialect - the DmoMeta already is distinct for temp-table and permanent DMOs. And instead of DmoMeta, the DMO class can be used safely at the key. From the heap dump, the Key.text does have the buffer alias as prefix, but I don't think this can be easily removed.

Constantin Asofiei wrote:

Also, the cache Key doesn't need the dialect - the DmoMeta already is distinct for temp-table and permanent DMOs.

The possible distinction of dialects is not only between temp-table and permanent table. It may be a rare or even just theoretical case, but an application could be configured with multiple, permanent databases of different dialects, using the same schema. So, I think the dialect is needed for the cache key, since the same generic sort criterion input taken from a converted query may be modified for a particular dialect when generating the final FQL sort expression (e.g., using a computed column property name instead of an upper/rtrim expression).

In retrospect, this processing seems better placed in the SQL generation step, instead of producing dialect-specific FQL. However, we used to rely on Hibernate for the HQL -> SQL generation, so we had less flexibility at that time and pushed this processing back to SortCriterion.

Eric, I was assuming that the SortCriterion is being used at the FQL level, before the FQL translation (dependent on the dialect) is performed. Am I mistaken?

Constantin Asofiei wrote:

Alexandru Lungu wrote:

Is the profiling done with multiple users? The cache is global across all users, so it is a synchronized data structure. Maybe this can lead to some overhead?

No, the profiling is done with a single user, which executes the tests sequentially.

Executing a synchronized block still can incur significant overhead, even for a single user. I noticed this while profiling SourceNameMapper APIs some time ago. Are we seeing any bottleneck around the cache use? We can temporarily disable the synchronization for comparison, since this testing is single-user.

In looking over the SortCriterion code, it seems to me that there are two things that are calling out for improvement in this code, but we should only take on this effort if we are seeing a clear bottleneck in these places:

• Do a much better job with the sort criterion/criteria parsing. That is, implement single-pass parsing in the methods/c'tors which parse. The aim would be to avoid all the extra string scanning we do by using redundant invocations of indexOf, substring, equalsIgnoreCase, etc. I'm not suggesting a full-blown grammar-based parser; hopefully, a hand-written character scanning loop for these relatively simple strings will suffice. BTW, I'm guilty of most of this, as I originally wrote this class.
• Assuming the above does not obviate the need for the LRU caches, we should consider using better synchronization for the caching than the simple synchronized block, leveraging the java.util.concurrent.locks package.

Considering that the overhead of the LRUCache is not insignificant, I wonder whether improving the parsing makes the LRU caching less attractive.

Again, these ideas currently are based on gut instinct, not profiling. Let's confirm that these are real bottlenecks with profiling before considering these improvements.

Constantin Asofiei wrote:

Eric, I was assuming that the SortCriterion is being used at the FQL level, before the FQL translation (dependent on the dialect) is performed. Am I mistaken?

That would be better, and I was (wishfully) remembering it this way, too. But the dialect does get stored in the SortCriterion instances and it is used to make decisions about how the FQL order by clause is generated, before the translation to SQL. Note the use of dialect in the SortCriterion c'tor and toSortExpression method.

This was the bit I mentioned at the end of my #6816-14 post. IIRC, I wrote it this way originally because we were handing off HQL to Hibernate that already referenced DMO properties either as computed columns or wrapped by upper[/rtrim], to allow Hibernate to generate the SQL correctly. This technical debt was not resolved when we pulled out Hibernate, so it still works that way now.

I've tried the patch with a customer application and there is an abend:

Caused by: java.lang.StringIndexOutOfBoundsException: String index out of range: -1
        at java.lang.String.<init>(String.java:196)
        at com.goldencode.p2j.persist.SortCriterion.<init>(SortCriterion.java:391)
        at com.goldencode.p2j.persist.SortCriterion.<init>(SortCriterion.java:319)
        at com.goldencode.p2j.persist.SortCriterion.parse(SortCriterion.java:568)
        at com.goldencode.p2j.persist.SortCriterion.parse(SortCriterion.java:676)
        at com.goldencode.p2j.persist.SortCriterion.parse(SortCriterion.java:652)
        at com.goldencode.p2j.persist.AdaptiveComponent.initialize(AdaptiveComponent.java:327)
        at com.goldencode.p2j.persist.AdaptiveQuery.execute(AdaptiveQuery.java:2870)
        at com.goldencode.p2j.persist.PreselectQuery.getResults(PreselectQuery.java:6034)
        at com.goldencode.p2j.persist.AdaptiveQuery.next(AdaptiveQuery.java:1660)
        at com.goldencode.p2j.persist.PreselectQuery.next(PreselectQuery.java:2617)

I think this can be committed to 6129b. Alexandru, any concerns?

Looks good, no concerns.

Alexandru Lungu wrote:

Looks good, no concerns.

Done. Committed to 6129b, rev. 14379.

The changes implemented for this task have long since made their way into trunk. It is still marked 40% done. Is there an expectation that further improvements can be made or is this task essentially finished?

Give me a bit of time to check PreselectQuery.assembleFQL, maybe I can notice something else. I'm not sure if Alex had in mind other optimizations, though.

I am dumping some statistics with LTW right now. I will let you know if I continue seeing something obvious here.