Custom serialisers to improve the footprint of aldica caches #34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AFaust
force-pushed
the
feature/serialisation-improvements
branch
from
651ccf2
to
85cbcf9
Compare
andreaskring
previously approved these changes
Jul 3, 2020
| ${moduleId}.core.binary.optimisation.enabled=true | ||
| ${moduleId}.core.binary.optimisation.useRawSerial=\${${moduleId}.core.binary.optimisation.enabled} | ||
| ${moduleId}.core.binary.optimisation.useIdsWhenReasonable=\${${moduleId}.core.binary.optimisation.enabled} | ||
| ${moduleId}.core.binary.optimisation.useIdsWhenPossible=false |
There was a problem hiding this comment.
This could be set to true as the default value (as you suggested earlier), since the memory footprint / throughput numbers are very good in this case.
There was a problem hiding this comment.
In updated branch, useIdsWhenPossible now also inherites from enabled, meaning it is enabled by default.
- analysis of unexpected results for aspectsSharedCache - optional preloadQNames URL parameter for benchmark to verify limited impact of TransactionalCache flaw - enable useIdsWhenPossible by default - add missing specific cache types for various local/invalidating caches
AFaust
force-pushed
the
feature/serialisation-improvements
branch
from
a01a48f
to
374a3ec
Compare
|
Note: PR is not yet ready to be merged. I have been running the Repository-tier integration tests and when the second Repository instance tries to join the cache grid, there appears to be a bit of a partition exchange deadlock issue. I am not sure why / for what purpose, but some grid cache message handling is triggering a deseralisation of a node properties cache entries, which in turn performs a lookup on the content data cache for reconstituting the value map, and gets stuck there. It may just be an issue with our Ignite thread pool configuration, which was previously set to be very conservative, e.g. everything that was not clear as to how it was used / needed was reduced to a minimal value. |
|
So the problem - for some reason - does not occur when running the same state of Ignite in some external, Docker-Compose based deployment. Repeated integration tests also consistently show a slightly different stack trace. Looking at these additional details, it appears that the initialisation of Repository-tier web scripts is processing Data Dictionary deployed web scripts, loading the properties of a node therein with |
|
Disabling |
|
Alright - problem seems to be fixed now. Please verify as part of review by running integration test - I have actually added a new profile in the Repository-tier sub-module to run integration test but skip the regular unit tests, which now can take quite some time, using The main purpose of the fix is trying to avoid any deserialisation occuring on Ignite threads, since deserialisation with our serialisation improvements can mean access to another cache. And whenever a cache is accessed, Ignite internally uses a Future on a disconnected thread to load the actual value, which may require a remote call in case of a partitioned cache, and may also in turn involve another deserialsiation. By using withKeepBinary on Ignite caches during retrieval-like operations, we completely move all deserialisation handling to client code, meaning the original thread of the cache call, freeing up Ignite threads and avoiding lock ups. |
|
I have tried to run Inspecting the Docker logs for the repo gives: |
|
The problem above was due to some old Docker volumes... everything works now |
|
As per your last feedback and our web meeting this morning, I am merging this PR although you have not formally given your approval. This is to ensure we continue working on the remaining, outstanding PR to target a release before this week's TTL. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CHECKLIST
We will not consider a PR until the following items are checked off--thank you!
CONVINCING DESCRIPTION
This PR adds a basic set of custom binary serialisers to the Repository module of aldica in order to optimise the serialised state of cache keys and values, as well as in some cases avoid warnings emitted by Ignite due to Alfresco classes implementing
ExternalizableorSerializablehook methods. The various serialisers are all configurable in detail, and there are also simplified, high-level configuration properties provided to toggle all non-trivial optimisations on/off. In the default configuration, the serialisers will use a streamlined serial format without class field metadata and with potentially compressed value fields (e.g. writing primitives instead of their nullable wrapper objects), and will substitute both well known and dynamic values with placeholders that can be easily resolved back to the actual value during deserialisation. The dynamic value substitution is by default limited to entities / values that can be expected to be always stored in theimmutableEntitySharedCache, a cache that should be fully replicated among all grid members and thus would be extremely fast to lookup any required value.All custom serialisers come with their own test cases verifying correct functionality and a relative change in memory footprint for instances of the affected classes. Not all serialisers actually provide improved memory footprints in all situations, but the very few instances where the footprint actually grows can either be accepted because it affects rarely used value classes (
ModuleVersionNumber), be ignored because the difference is negligible, or be ignored because we do not intend the specific constellation of configuration properties resulting in the higher footprint to be used in real envrionments.The memory benchmark has been enhanced, redone and re-documented. The expansion of the test to also cover content data and properties of type
d:noderefandd:qnamehas partially compensated for the reduced footprint resulting from our custom serialisers, so the overall improvement compared to Alfresco default is still in the 20 - 25% range with regards to reduced memory requirements. Additionally, the bechnmark now also takes a very high-level look at the throughput of read operations on already initialised caches, and here we are able to show that aldica caches can generally provide better performance with less memory, despite the added overhead of serialisation / deserialisation.I still have further ideas for optimising our serialisation, but want to have the current state ready and merged in time for the 1.0.0 release and Tech Talk Live. Any future improvements will be done in new feature branches.
RELATED INFORMATION
N/A