Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement optional caching #19

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Implement optional caching #19

wants to merge 7 commits into from

Conversation

PillageDev
Copy link

This PR implements a caching system that users can enable, which caches the objects before entering the database. This improves the querying performance of the library. To enable the cache, users have to:

  1. Annotate any objects they want to cache with @UseCache
@NTable(name = "player_statistics", schema = "", catalog = "")
@UseCache // This enables the cache
public class PlayerStatsDTO extends NEntity<UUID> {

    @JsonProperty("killCount")
    private int kills;

    @JsonProperty("deathCount")
    private int deaths;

    @JsonProperty("lastLocation")
    private SerializedLocation lastLocation;

    public PlayerStatsDTO() { }
}
  1. Add one line to the onDisable to flush the cache
@Override
public void onDisable() {
    NDatabase.api().shutdown();
}
flowchart TD
    A[Client] --> B[CachedRepositoryImpl]
    B --> C{Cache Hit?}
    C -->|Yes| D[Return from Caffeine Cache]
    C -->|No| E[Load from DAO]
    E --> F[Store in Cache]
    F --> G[Return Value]
    
    B --> H[Write Operations]
    H -->|Insert/Update| I[Update Cache]
    I --> J[Cache Eviction]
    J -->|Removal Listener| K[Flush to DAO]

    subgraph Cache Configuration
    L[Expire After Access]
    M[Maximum Size]
    N[Soft Values Option]
    O[Refresh After Write]
    end
Loading

The cache is powered by caffeine, a high-performance java caching library (https://github.com/ben-manes/caffeine)
It makes use of their AsyncLoadingCache to run most operations on the async executor.

Due to how Bukkit unloads classes on shutdown, the user must call the api shutdown, as their plugin will be unloaded before NDatabase is unloaded.

Introduced caching functionality using Caffeine for entity repositories, with configuration options for expiration, size, and other settings. Added a `@UseCache` annotation to enable caching for specific entities and ensured proper cache flushing during plugin shutdown.
@NivixX NivixX added the request New feature or request label Dec 26, 2024
@NivixX
Copy link
Owner

NivixX commented Dec 26, 2024

Hello, thank you very much for your PR !
And I really appreciate how you detailed the content of the PR.
I think it's a good idea to add caching, I will have a closer look at the PR when I'll have more time.

Here are some random thought and stuff we can improve concerning this implementation:

  • a shutdown() method has been added in order to set free the memory allocated. But the current implementation will shutdown every single CachedRepository, so if we have 2+ plugins using cache, it may shutdown cache of other's plugins.
    I don't know if I really want to require developer to supply plugin instance when creating NDatabase repository, but at the same time, it would be necessary if we want to handle plugin unload properly.
    I think one compromise solution we can use without having to supply plugin instance, is to provide a shutdown method, but need to specify the class of the repository, for example shutdown(MyPluginAEntity.class).
    Also, with reflection, when we create a repository we may be able to scan the stacktrace to try to find the plugin instance, it will work in most case and we can still use the manual shutdown(Entity.class) as a fallback solution

  • We can also re-arrange the code so that if we have a repository with @UseCache, we can still use the NQuery. In the current implementation, every repository with UseCache won't be able to do value based query

  • Add parameters to UseCache if we want different config

  • Unit tests

  • I think the decision of not creating other threads when calling getAsync() on cached repository is a good idea as it's real time. Both Delete and Insert/Update should stay async as it require database writing

If I have time in the future, I can also help to address the above points

@PillageDev
Copy link
Author

Thank you for your reply! I will address these comments shortly.

@PillageDev
Copy link
Author

Hey, I am working on point 1 right now. I have a question about point 3; what parameters should be added to the use cache? Should the optional parameters be reflective of the config values? (Or something else)

…me. Associate plugin name with entity class for dynamic unloading when calling the shutdown.
@PillageDev
Copy link
Author

This scans the stack trace during the shutdown call and identifies entities associated with the plugin's name. I tested it with two different plugins, and it successfully removes the entities from the repository cache as each plugin invokes the shutdown() method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants