Cache stringConstant results on JITServer to reduce ResolvedMethod_stringConstant messages

[AbdallahAbd05]

Caches the result of stringConstant(cpIndex) on the JITServer side to avoid sending repeated ResolvedMethod_stringConstant messages to the client for the same (class, cpIndex) pair within a compilation.

The cache uses (TR_OpaqueClassBlock *, int32_t) as the key, mirroring the existing isUnresolvedString cache, since cpIndex refers to a constant pool which is associated with a class rather than an individual resolved method.

Issue: https://github.ibm.com/runtimes/rt-tr-common-repo/issues/30

[mpirvu]

The code looks ok but there is room for improvement.
The solution creates a secondary map with the same key <class, cpIndex>. Why not have the same map <key,value> with an extended value?

What is the hit rate of the implemented cache? How many messages were before and after this change?
Depending on the answer we could go one step further: the cache is per compilation, so it gets destroyed when the compilation is done, only to be recreated for the next compilation (if needed). We could have a cache per client (for all compilations stemming from that client) because classes don't become invalid when a compilation ends. The disadvantage of this solution is that the memory for the cache will not be freed at the end of the compilation.

[AbdallahAbd05]

The code looks ok but there is room for improvement. The solution creates a secondary map with the same key <class, cpIndex>. Why not have the same map <key,value> with an extended value?

What is the hit rate of the implemented cache? How many messages were before and after this change? Depending on the answer we could go one step further: the cache is per compilation, so it gets destroyed when the compilation is done, only to be recreated for the next compilation (if needed). We could have a cache per client (for all compilations stemming from that client) because classes don't become invalid when a compilation ends. The disadvantage of this solution is that the memory for the cache will not be freed at the end of the compilation.

In my latest push I combined the two maps into a single _stringConstantCache using a new TR_StringConstantData struct that holds both the void * and the two booleans. I created a benchmark with 10 string returning methods called 100,000 times and ran it with JITServer. On master, ResolvedMethod_stringConstant messages were around 2180 in hot runs, while on my branch they were consistently around 44-72. So what do you think we should do?

[mpirvu]

On master, ResolvedMethod_stringConstant messages were around 2180 in hot runs, while on my branch they were consistently around 44-72. So what do you think we should do?

This seems like a great reduction and based on this alone I would just proceed with this solution.
Is it possible to get the number of messages for something like AcmeAirEE8?

[AbdallahAbd05]

On master, ResolvedMethod_stringConstant messages were around 2180 in hot runs, while on my branch they were consistently around 44-72. So what do you think we should do?

This seems like a great reduction and based on this alone I would just proceed with this solution. Is it possible to get the number of messages for something like AcmeAirEE8?

I ran the AcmeAirEE8 benchmark with JITServer. The reduction is less dramatic than my simple benchmark but on AcmeAirEE8, Master sent 48 ResolvedMethod_stringConstant messages while my branch sent 23, so about a 50% reduction.

[mpirvu]

on AcmeAirEE8, Master sent 48 ResolvedMethod_stringConstant

Something is off. I remember this being in the thousands.

[AbdallahAbd05]

on AcmeAirEE8, Master sent 48 ResolvedMethod_stringConstant

Something is off. I remember this being in the thousands.

I ran AcmeAirEE8 with various configurations. Master still consistently gets around 29-48 ResolvedMethod_stringConstant messages while my branch gets around 23-24. The reduction is a lot less significant than on my simple benchmark. I'm not entirely sure why the numbers are so low on AcmeAirEE8 compared to what we expected. Maybe the per-client cache would be more effective? Should I go ahead and implement it?

[mpirvu]

I just had a run of AcmeAirEE8. The number of ResolvedMethod_stringConstant messages is around 16K, so significantly higher than what you are getting. The total number of messages is around 400K.
Are you applying load to AcmeAirEE8? What is the absolute throughput you are getting?

[AbdallahAbd05]

Yeah I'm applying load with JMeter using 10 threads for 180 seconds. My total messages are around 2700-2800 and I'm getting about 31 ResolvedMethod_stringConstant messages on master. My throughput peaks around 2000-4000 req/s. I must be running a much lighter load because this is very different from your 400K total messages. What JMeter parameters are you using?

[mpirvu]

10 JMeter threads should be enough. Maybe run is too short, especially if you are using a single CPU for Liberty. I use 10 minutes of load. For a cold run (with empty SCC) there are about 40K messages. For warm run (SCC populated by the first run) I get 20K total messages.

[AbdallahAbd05]

I've tried various configurations (10 minute runs, empty SCC, with and without AOT cache) and consistently get my same results as before. All compilations seem to happen during Liberty startup (about 190 compilations), with no new compilations triggered during the 10 minute JMeter load. What Liberty image and configuration are you using for the benchmark?

[mpirvu]

All compilations seem to happen during Liberty startup (about 190 compilations), with no new compilations triggered during the 10 minute JMeter load.

That's a red flag right there. There should be thousands of compilations during JMeter load under any configuration. The version of Liberty should not matter that much. I would say that JMeter does not connect at all to Liberty, but I cannot reconcile the reasonable throughput value you see: "2000-4000 req/s". Is JMeter reporting errors?
I have a repo that shows how to install and drive AcmeAirEE8: https://github.com/mpirvu/LibertyApps/tree/main/AcmeAirEE8
I am not sure whether you followed that or some other instructions.

Regardless, could you please push your newest version of the code so that I can have a look? There could be some optimizations at the point of the call.

[AbdallahAbd05]

I found the issue, I followed your repo more closely and tried again, I get about 12K ResolvedMethod_stringConstant messages on master and about 7K on my branch, so about a 41% reduction. Total messages went from roughly 260K to 229K. Also, my branch is already pushed and updated.

[mpirvu]

While the solution works, it can be optimized further. TR_ResolvedJ9JITServerMethod::isUnresolvedString(I_32 cpIndex, bool optimizeForAOT) does not do any caching which forces TR_ResolvedJ9JITServerMethod::stringConstant() to send another message.
Sometimes isUnresolvedString() is used by itself (without stringConstant()) and therefore no caching will take place. Example:

                if (isUnresolved && node->getSymbol()->isConstString()) {
                    TR::ResolvedMethodSymbol *rms = comp()->getOwningMethodSymbol(node->getOwningMethod());
                    isUnresolved
                        = rms->getResolvedMethod()->isUnresolvedString(node->getSymbolReference()->getCPIndex(), true);
                }
                return isUnresolved;

In other cases we call stringConstant() followed by isUnresolvedString(). In these cases caching works fine:

  void *stringConst = owningMethod->stringConstant(cpIndex);
    TR::SymbolReference *symRef;
    bool isString = true;
    if (owningMethod->isUnresolvedString(cpIndex)) {
        symRef = findOrCreateCPSymbol(owningMethodSymbol, cpIndex, TR::Address, false, 0);
        symRef->setOffset((uintptr_t)stringConst);
    } else {

Yet, in other cases we first call isUnresolvedString() and only then stringConstant(). Again, we may send two messages because only stringConstant() caches the result sent by the client:

           if (method()->isStringConstant(cpIndex) && !method()->isUnresolvedString(cpIndex)) {
                uintptr_t *location = (uintptr_t *)method()->stringConstant(cpIndex);

I think we should also send the ResolvedMethod_stringConstant messages for isUnresolvedString() and cache the result. This way no matter the order of the two frontend calls, the second one will always find the desired data cached.

Furthermore, the calling points of these frontend APIs can be changed so that we have a single cache access. Maybe that can done in a separate PR.

[AbdallahAbd05]

I've updated the branch with the optimizations, but the average number of stringConstant messages I'm getting is still roughly 7K.