FWD » Core Development » Runtime Infrastructure

After discussing with Constantin, mostly in regard to #7398, there are several places in FWD where the directory is read from static blocks. This reading is done in a quite non-deterministic fashion to us. In the backstage, the read is done when the class is loaded, but this is not quite easy to pinpoint and certainly not easy to manage.

A solution here is to refactor static blocks into static initializing methods. These methods will be called at server bootstrap, such that the directory read is done at a deterministic point. An initial effort in this sense is done in #7388, where cache sizes will be configured only once at the server bootstrap based on directory.xml values.

This work is not trivial, as some class initialization don't make sense (i.e. TableHints.initialize, TempTableDataSourceProvider.initialize - these are not a static helper classes, .initialize doesn't seem to make sense). For caches, we will intentionally create a separate static helper class whose .initialize makes sense. I would expect to see the same for other classes (maybe move the directory.xml reads in other helper classes which already have .initialize). We need to avoid calling tens of .initialize at server bootstrap, but to group them logically.

• TemporaryAccountPool
• WebHandler: from the main package
• DirtyShareSupport
• P2JQueryLogger
• Session: from the persist.orm package
• SQLStatementLogger
• TempTableDataSourceProvider: Note that the static block was removed in 7388a.
• DatabaseManager
• DatabaseStatistics
• MetafileHelper
• XprHelper
• JavaTypeSerializer
• TableHints
• EnvironmentOps

I will double check after replacing the current static blocks with initialize methods to make sure that none are missed. If there are places where the current directory.xml access should not be removed, tell me.

EDIT: SQLStatementLogger doesn't have a static initializer block.

Danut, try to make the initialization calls hierarchical. That is, do not list all initialization in StandardServer.
For example, I think P2JQueryLogger, DatabaseStatistics and SQLStatementLogger can be grouped. In fact P2JQueryLogger and SQLStatementLogger can be initialized from DatabaseStatistics.

• P2JQueryLogger can be initialized from DatabaseStatistics, as previously mentioned by Alexandru
• Any class from the persist package can be initialized in Persistence.initialize
• There are already server hooks defined for TemporaryAccountPool and supposedly WebHandler (correct me if I am wrong)
• Created new server hooks for the rest of the bootstrap calls, grouped by package.

All the changes that I am going make here should be committed to 7388a instead of 7425a because the server configuration for cache initialization was changed.

I committed 7388a/rev.14620 where static initializers were replaced by methods called at server bootstrap. I tested the changes on small programs, but will continue with testing a customer application.

Constantin, could you review these changes?

Rebased 7388a from trunk rev. 14633. 7388a is now at rev. 14636.

Review of 7388a

• In StandardServer, you no longer need DatabaseStatistics.get as the bootstrap is done explicitly already. You can remove the get call. Move the comment before .bootstrap
• I think we should have a more uniform way of handling directory binding. Please decide only one single pattern for all
  • there are places where Utils.getDirectoryNodeInt(null, ...); is used straight-away (this is OK only when the target instance can be null)
  • there are places where if (!dir.bind()) throw new RuntimeException("Directory bind failed"); is used to check if there is a bind directory. (this should be used)
  • there are places where finally{ ds.unbind(); } is used after usage: sometimes this doesn't happen; sometimes the unbind is not in a finally clause (this should be used)
  • there are places where ds.bind() happens in the try clause with ds.unbind() finally clause; some have the bind outside try (It doesn't make sense to attempt unbinding if binding failed)
  • there are places where ds == null is used (I think this is decent to be used)
• Private constructor in private P2JQueryLogger() was removed (?)
• Adding public in public class Persister is something dangerous from my POV. Maybe we can bootstrap it from another class in the same package without exposing Persister.
• CacheManager has several issues:
  • initializeCache calls may be omitted if we return early or an exception is thrown (the directory is not bound). The initialize should be always done, no matter if we return early / an exception is thrown.
  • The logs are not very descriptive. Please include the name of the parameters and values to identify what is wrong. If a class could not be found, log the name of it. If a cache is misconfigured, log its name.
  • I don't think it is safe to use initialize in a multi-session environment. Please bootstrap CacheManager only once at server start-up to ensure it is initialized only once. Remove any if (!initialized) initialize(); code.
  • Please make ClassKey.equals return false if o is not an instance of ClassKey
• Move Session.PK after all final members, as it is no longer final. There are other cases of such mismatch (e.g. DatabaseManager)
• Session.createCache no longer includes the database name in the cache name. Consider using database.toString() as discriminator. Change your cache size look-up to the following. This allows setting different cache sizes for each database sessions.
  • If you require class and discriminator: If there is a (class, discriminator) entry use it. Otherwise if there is a (class, null) entry use it. Lastly, use the default.
  • If you require class: If there is a (class, null) entry use it. Lastly, use the default.
• Please fix history entry numbers: everything that comes from 7388 should share the same history number / per file (e.g. BufferManager).
• Anything that you made non-final should be private and retrieved through getters. Otherwise, we may change it from outside of the class.
• Please do a check with a large customer application. Debug a bit to check if the values are right considering the directory.

Danut, please fix this ASAP.

• Removed the DatabaseStatistics.get call in StandardServer
• This will be the standard for handling directory binding:
  

  if (ds != null)
  {
      if (!ds.bind)
      {
          throw new RuntimeException("Directory bind failed");
      }
  
      try
      {
  
      }
      finally
      {
          ds.unbind();
      }
  }
  

• I instantiated the Persister cache in Session.bootstrap() and removed the public modifier
• Added better log messager in CacheManager
• Fixed initializeCache calls that could be omitted if binding fails.
• Removed intialized and it's usage in CacheManager, even though caches can be instantiated before CacheManager.initialize() is called, a default value is used for the cache size
• Added an additional check for ClassKey.equals method
• Reordered class members in some cases
• Fixed history entries
• Added CacheManager.getCacheSize that will be used to look for the cache size of a cache. If a discriminator is provided but there is no value found, it will use the default key (discriminator is null) to look again for the cache size.

Alexandru Lungu wrote:

• Private constructor in private P2JQueryLogger() was removed (?)

It was not removed.

• Anything that you made non-final should be private and retrieved through getters. Otherwise, we may change it from outside of the class.

It is really necessary to use getters when the cache is not used outside of the class? I think you are talking about DatabaseManager.forceNoUndoTempTables. In this case I added getters for this member. Same goes for DirtyShareSupport class members.
Note that if that's the case, should I still make these changes to Session.PK and JavaTypeSerializer members? I didn't do it yet because they are used in a lot of places and I would like to do a separate commit.

I tested a large customer application and everything worked well, but I did make a few more changes and need to retest to make sure.
Here is an example from directory.xml on how to set up a cache size:

            <node class="container" name="08">
              <node class="string" name="class-name">
                <node-attribute name="value" value="com.goldencode.p2j.persist.FastFindCache"/>
              </node>
              <node class="integer" name="size">
                <node-attribute name="value" value="10"/>
              </node>
              <node class="string" name="discriminator">
                <node-attribute name="value" value="L2"/>
              </node>
            </node>

I retested the customer application and found out that the psCache is initialized before the CacheManager, which means that the cache will not use the configured size. Only after the context is closed and a new one is opened, the psCache will use the configured size. I've committed 7388a/rev.14638 which solves this issue by initializing CacheManager before DatabaseManager. Except for this problem, I had no other issues.

Radu, please check P2JQueryLogger. I feel like I've mismatched something in it while rebasing. More specific, I am not sure if loggingEnabled is still used to rule out logging. Please double-check 7388a, more specifically your logging classes.

Alexandru Lungu wrote:

Radu, please check P2JQueryLogger. I feel like I've mismatched something in it while rebasing. More specific, I am not sure if loggingEnabled is still used to rule out logging. Please double-check 7388a, more specifically your logging classes.

A short answer to this would be that loggingEnabled is still used to mark whether P2JQuery logging is enabled or not. I don't see anything being out of place.

• You can enable/disable P2J query logging in directory.xml, under the persistence tag, by having:
  

        <node class="container" name="p2j-query-logging">
              <node class="boolean" name="enabled">
                <node-attribute name="value" value="TRUE"/>
              </node>
              ...
        </node>

• P2JQueryLogger does not support yet an output file to be set, meaning that logs will be placed in the server log, as a WARNING. An example of a RandomAccessQuery (without stack trace) log is:
  23/06/28 15:37:50.557+0300 | WARNING | com.goldencode.p2j.persist.orm.P2JQueryLogger | ThreadName:Conversation [00000002:bogus], Session:00000002, Thread:00000013, User:bogus | com.goldencode.p2j.persist.RandomAccessQuery: Records processed: 1;
• Although there is a field called loggingEnabled in P2JQueryLogger, it is not actually used, because P2JQueryExecutor is the one that controls the situation. The loggingEnabled field that should be used is the one from P2JQueryExecutor and not from P2JQueryLogger. The latter one is redundant, I forgot to delete it.

I'm not sure if this answers your question, but the point is that I don't see anything wrong.

Although there is a field called loggingEnabled in P2JQueryLogger, it is not actually used, because P2JQueryExecutor is the one that controls the situation. The loggingEnabled field that should be used is the one from P2JQueryExecutor and not from P2JQueryLogger. The latter one is redundant, I forgot to delete it.

This is what confused me in the rebase process. I thought that maybe I missed the usage of loggingEnabled in P2JQueryLogger. Thank you for the clarification. Please delete it here in 7388a.

Alexandru Lungu wrote:

Although there is a field called loggingEnabled in P2JQueryLogger, it is not actually used, because P2JQueryExecutor is the one that controls the situation. The loggingEnabled field that should be used is the one from P2JQueryExecutor and not from P2JQueryLogger. The latter one is redundant, I forgot to delete it.

This is what confused me in the rebase process. I thought that maybe I missed the usage of loggingEnabled in P2JQueryLogger. Thank you for the clarification. Please delete it here in 7388a.

Done, committed to 7388a, rev. 14639. Sorry for the confusion!

Danut, please let me know if there is something else to be addressed here. I will like to do a review and testing round. If everything is right, we shall commit this asap.

Please proceed with the reviewing/testing since I've yet to receive a review for #7425-11. The last change I've done was mentioned in #7425-12 and currently there is nothing else that needs to be done.

Review of 7388a/rev. 14639

• TemporaryAccountPool still has a static block that does all kind of initialization. Should we consider moving all that in bootstrap instead of leaving them in the static block? I mean, ServerStandard was forcing this static block anyway with getInstance, so from my POV, these can be safely moved in bootstrap. I am not aware if this class should be initialized outside the server. Maybe somebody with more TemporaryAccountPool background can advice.
  • This is a place where the directory.xml pattern is not honored: look-up for ds, check for null and bind, etc.
• ds and bind are not used for WebHandler - I missed checking the other files to for this pattern, please do a second check here

I am OK with other changes.

Constantin, can you do a second review here, please? I think this was supposed to reach trunk already (some time ago), but even so, I guess it will be OK if we get it now asap.

Alexandru, you previously mentioned that "there are places where Utils.getDirectoryNodeInt(null, ...); is used straight-away (this is OK only when the target instance can be null)" so I considered that the directory service change in this case is not required. In both cases, TemporaryAccountPool and WebHandler use null and the directory service is called by Utils.doWithBoundsDs. From my point of view, it looks a lot cleaner.

You are right Danut, my bad on this one. It is fine to keep null for ds and let the directory service do the work. I am just obsessed of keeping bind/unbind pattern balanced. For null, the balance is ok behind the scenes. For other places, I want to stress out how important is to keep bind/unbind ref count OK.

Alexandru, I guess the branch is 7388a, with latest rev 14639. I'm reviewing this now.

Right, 7388a was the one I meant to write. Edited now.

• TemporaryDatabaseManager.initialize - unbalanced ds.bind, this is an existing issue, please fix it.
• other cases which have Utils.getDirectoryNode in static constructor and should be initialized at server startup, if not already done implicitly by loading the class:
  • P2JQueryExecutor - static c'tor
  • Validation.P4GL_MAX_INDEX_SIZE - static field
  • Persistence.foreignKeysEnabled - static field initialized, but I think this is OK, as Persistence class gets initialized at FWD server startup AFAIK.
  • HtmlBrowserWidget - has static fields, but this is used only in GUI, so is OK
  • FileSystemOps.pkgRootPath - static field
  • SecurityOps.compatible - static field
• FQLPreprocessor - please leave only 096 number and remove 097 text.
• orm.Session - same, remove 007 text from history entry

Otherwise, nice work, Danut.

• Fields were made non-final and renamed if necessary (uppercase to camel case), the only exception being P4GL_MAX_INDEX_SIZE;
• As suggested, I skipped Persistence.foreignKeysEnabled and HtmlBrowserWidget;
• Directory handling was brought up to the standard in TemporaryDatabaseManager and P2JQueryExecutor as mentioned in #7425-11.

I will do a second review asap. I am intending to do a second rebase + test + profile just to check that the performance isn't degraded. If everything is ok, can we merge this by tomorrow EOD?

If everything is ok, can we merge this by tomorrow EOD?

Yes

Review:

• I am still a bit concerned of TemporaryAccountPool.getInstance(); TemporaryAccountPool.bootstrap();. I feel like bootstrap should be enough, right? That implies moving the static block in bootstrap. Constantin, any feedback here for TemporaryAccountPool?
• I am a bit concerned of the createLRUCache interface that always requires a discriminator. This is usually a particularity of some caches; the common case is "no discriminator". Danut, please create a second method only with class and size, which calls this 3 parameter method under the hood with null as discriminator. Refactor the places where null is used as second parameter.
• Be more consistent with cache sizes. This is not related with your changes, but can be fixed now: Persistence.Context.cache has size 1000. Can we make this 1024 for the sake of consistency with other cache sizes?

I tested the changes and got no regression. Performance is consistent with my baseline - this is expected as most changes are related to server start-up anyway.

Alexandru Lungu wrote:

Danut, please create a second method only with class and size, which calls this 3 parameter method under the hood with null as discriminator. Refactor the places where null is used as second parameter.
Be more consistent with cache sizes. This is not related with your changes, but can be fixed now: Persistence.Context.cache has size 1000. Can we make this 1024 for the sake of consistency with other cache sizes?

I added the an additional createLRUCache and increased the cache size of the case in Persistence.Context to 1024. These changes were committed to 7388a/rev.14641. Here's a list of known classes with inconsistent cache sizes: BufferManager (10000), FastFindCache (L2 - 10 and L3 - 100).

Alexandru, TemporaryAccountPool is OK, it gets fully initialized at server startup. I don't think even bootstrap is needed, but you can leave it to be consistent.

Danut, please do the same changes to BufferManager and FastFindCache. Find the smallest power of 2 bigger than the existent sizes. I don't think these are going to break something (hopefully) - I will just do a very quick check before merge.

So, after you do the changes to BufferManager and FastFindCache, I will rebase and commit.

• BufferManager: from 10000 to 16384.
• FastFindCache L2: from 10 to 16.
• FastFindCache L3: from 100 to 128.

Hope that BufferManager won't cause any problems, it's cache size went up by more than 50%.

Well, BufferManager is per-session so it may be a problem. We can go the other way around sticking to 8192 which is closer to the original value, but may cause performance draw-backs. For FFC, from my POV, they are quite right. I guess we can go with 16384, as long as we allow decreasing its size through server options, right? On the other way, I want to get 7388a in trunk in the next hour, so I may no be able to do the proper testing. Danut, I reverted the changes from #7425-33 as they may be too bold and may require extra testing. I will consider creating another task for cache size tuning.

I will update the Database Configuration wiki with the proper configuration and explanation for cache sizes as suggested in #7388-5.

I updated Database Configuration, this is subjected to changes since I am still working on it, but it should provide a basic explanation on how cache sizes should be configured.

Danut, please make sure you get this done correctly asap. I intend to experiment with some fine-tuning using this configuration.

Alexandru Lungu wrote:

Danut, please make sure you get this done correctly asap. I intend to experiment with some fine-tuning using this configuration.

I finished writing the wiki, please let me know what you think and if I missed something.

I am OK with the documentation. Looks clear to me. However, I feel you can do just a bit more feedback on the "list of caches". It is useful if you can provide their full class name (including package). One can simply check the "cache key" and configure it, without the need to actually search for that class inside the FWD code. Instead of FastFindCache, use com.goldencode.p2j.persist.FastFindCache. Also, please let the documentation hold the default cache sizes.

Alexandru Lungu wrote:

I am OK with the documentation. Looks clear to me. However, I feel you can do just a bit more feedback on the "list of caches". It is useful if you can provide their full class name (including package). One can simply check the "cache key" and configure it, without the need to actually search for that class inside the FWD code. Instead of FastFindCache, use com.goldencode.p2j.persist.FastFindCache. Also, please let the documentation hold the default cache sizes.

Done. Thank you for the feedback.

The order of the server bootstrap initialization was changed in trunk/rev.14654 and the resources affected need to be investigated further and decide if the current order is correct. For further information on this investigation, please refer to #7534-7.

• There were some instances where cache instantiation was moved to other initialization methods, such as Persister.initializeCache which is called by Session.bootstrap. In this case, CacheManager.initialize should be called before Session.bootstrap to make sure that the cache is created with the configured size.
• From the list mentioned above, the following only read from the directory configuration: Session, Validation, P2JQueryExecutor, DirtyShareSupport.
• DataTypeHelper.initialize initializes static resources which don't make use of directory access / cache configurations / previously modified resources.
• TemporaryDatabaseManager.initialize creates an instance of PrivateTempDbManager/SharedTempDbManager, which doesn't require previously modified resources.
• The order between TemporaryDatabaseManager, DatabaseManager and P2OLookup should not be changed. DatabaseManager will work with the private/shared manager that was created and then P2OLookup requires the managed databases that are created by it.

The conclusion is that CacheManager can be placed first, followed by Session, DirtyShareSupport, P2JQueryExecutor Validation, and DataTypeHelper. CacheManager also reads the configuration (with the exception of the 2 caches that are initialized by it).

I did not mention anything about SessionFactory.SessionCloseThread.initialize since it starts a thread before any SessionFactory is created and it worked properly.