FWD » Core Development » Runtime Infrastructure

Added caching to various places that were showing up as bottlenecks in profiling. Changes committed to 3435a/11217. Made measurable improvements in ETF batch and developer testing.

The use of ExpiryCache.get is showing up in profiling from where I added compiled regex pattern caching to TextOps.matchesList, so the LRUCache use in a synchronized block there may not be helping (and is possibly hurting). From the test set I'm using, it doesn't seem to be any improvement over the original implementation.

It would be faster to use ConcurrentHashMap there instead, but the downside is that the map will grow indefinitely, unless we prune it from time to time. Greg, how likely do you think it is we could get a very large set (say, more than several 10 thousands) of unique regex expressions in that API? This would be a potential memory leak.

Code Review Task Branch 3435a Revisions 11217 through 11221 Inclusive

I'm fine with the changes. I looked pretty carefully and do not see any logic issues.

how likely do you think it is we could get a very large set (say, more than several 10 thousands) of unique regex expressions in that API? This would be a potential memory leak.

The cache is server-wide. Over the lifetime of the server process, this could certainly be called a great deal.

Calls to TextOps.matchesList() occur from 2 places:

• Converted CAN-DO built-in function calls. With static input, it might only be in the tens of thousands of different cases per application. If this is called with dynamic input, then there could be a wide range of cases, possibly far beyond the thousands.
• External procedure program name resolution for appserver calls. Most likely this is on the smaller side but if the application relies on this for rejecting invalid calls and some client is programmed badly, it could be much more.

I think the issue is real, even if it is most likely only going to occur in a badly implemented application.

Why not "solve" this by setting some hard limit (50,000 or 100,000) and checking the map size at access time? When the hard limit is reached, clear the map. It resets all the stored regexes (not optimal), but if the limit is high enough it only will affect the bad applications.

Greg Shah wrote:

Why not "solve" this by setting some hard limit (50,000 or 100,000) and checking the map size at access time? When the hard limit is reached, clear the map. It resets all the stored regexes (not optimal), but if the limit is high enough it only will affect the bad applications.

I implemented this in revision 11223, though ConcurrentHashMap.size looks to be pretty expensive to call with each put, so I created a separate counter with AtomicInteger which should do the job.

Code Review Task Branch 3435a Revisions 11223 through 11225 Inclusive

I'm good with the changes.

Hynek, please see task branch 3435a/11228. The thread-local access in ConfigSyncManager was showing up as a hot spot in CPU profiling the AW app, so I tried to reduce it as much as possible. I did some successful ad-hoc testing of this, but please do a quick code review to make sure I did not break your logic with my optimizations.

Code review 3435a/11228. The changes are good.

Hynek, please review the changes in 3436a/11215. Greg asked me to put this in a branch separate from 3435a, pending your review. I'll merge it to 3435a if all is well.

The point of the change is to eliminate the relatively expensive calls to ConfigManager.getInstance from ConfigFieldSetterAspect.beforeSetField, which profiling showed as a hot spot. Instead, I store a reference to the ConfigManager in each WidgetConfig instance, within a transient instance variable. This approach allows us to eliminate the calls in ConfigFieldSetterAspect.beforeSetField. It has the side benefit of making the ConfigManager instance available for the various places it is used directly from within WidgetConfig itself (though these are called much less often in the use case I profiled).

The GenericFrame case represented a smaller hot spot, but I decided to address it and MenuContainerWidget similarly.

Eric Faulhaber wrote:

Hynek, please review the changes in 3436a/11215. Greg asked me to put this in a branch separate from 3435a, pending your review. I'll merge it to 3435a if all is well.

Eric, the changes are good.

I am just wondering whether we would benefit further from initializing the WidgetConfig.configManager field lazily. There are several places where the created (short-lived) config instances do not require access to config manager (for example config instances that are held for further reference, like in CosnfigSyncManager.registerConfig() and the call to duplicate()).

Possibly. I'll do some more profiling and see if these cases pop up. Thanks.

In the mean time, I've committed the reviewed changes to 3435a/11230.

Hynek, profiling of the client confirmed that we were still calling ConfigManager.getInstance quite a lot from the places you suggested. So, I implemented your idea in 3435a/11232, which further reduces these calls significantly.

BigDecimal.valueOf(double) and BigDecimal.setScale(int,int) are still showing up as medium hot spots on the client in this use case, both called from CoordinatesConversion.toChar. Was it problematic to replace this use? Not the highest priority, just wanted to document.

BTW, it seems we're spending a lot less time on the client than the server overall. It's not obvious to me how to measure the time spent on the network between the two.

Eric Faulhaber wrote:

BigDecimal.valueOf(double) and BigDecimal.setScale(int,int) are still showing up as medium hot spots on the client in this use case, both called from CoordinatesConversion.toChar. Was it problematic to replace this use? Not the highest priority, just wanted to document.

This should be resolved in 3435a revision 11233.

Some additional ideas on performance to implement when there is time:

Read-only Tables. We need to identify tables which are effectively read-only and mark them as such in schema hints. This will eliminate or defer certain runtime processing in query handling and in certain record buffer operations. This feature already is implemented in the runtime, but we will need customer input to identify candidate tables. Tables used only for configuration or as templates usually are good candidates.

Denormalization of Extent Fields. Denormalization of smaller extent fields tends to result in better performance in general, because it is faster to fetch all fields at once than to have to go back for another round trip if the data are needed later. This one is a bit tricky, because it depends on how the extent fields are used and how much data there is. There can be cases (e.g, if the data is big and is only used in rare circumstances) where denormalization could actually lead to worse performance. We definitely don't want to denormalize all extent fields (some are up to 160 in size), but a conservative application of denormalization (say, fields of extent 7 or less?) may be helpful. We don't necessarily need customer input to try this one.

Hibernate Second Level and Query Cache. I had been avoiding turning this on, because it has implications for updating the database externally (cached data can become stale without us knowing about it). Plus, I was planning on making more drastic changes to our persistence layer to make this obsolete. However, this one was easy to enable for a quick test and it does seem to make a positive impact, so I think we should leave it on. When we get to the point where we're making more substantive changes to our use of Hibernate, it can always be turned off again.