Skip to content
This repository was archived by the owner on Feb 17, 2024. It is now read-only.

Refactor ActivityMap#39

Merged
wingzero54 merged 4 commits intoCivMC:masterfrom
Aleksey-Terzi:activity_map
Jul 22, 2022
Merged

Refactor ActivityMap#39
wingzero54 merged 4 commits intoCivMC:masterfrom
Aleksey-Terzi:activity_map

Conversation

@Aleksey-Terzi
Copy link
Copy Markdown

The goal of this change is to improve performance of the server and decrease amount of used resources (first of all load on the database)

  1. Added support for load/unload functionality (in a similar way CMC ChunkMeta does)
  2. It loads into the memory only those regions that belong to loaded chunks and unload those that are not used by loaded chunks
  3. activity-map-resolution should be multiple of 16 i.e. aligned with chunk size
  4. Only one thread is loading regions and only one thread is saving regions
  5. Introduced a new command am stat (permission citadel.admin) that shows ActivityMap statistic:

image

Comment thread paper/src/main/java/vg/civcraft/mc/citadel/activity/XZKey.java Outdated
Comment thread paper/src/main/java/vg/civcraft/mc/citadel/model/ActivityItem.java Outdated
Comment thread paper/src/main/java/vg/civcraft/mc/citadel/command/Activity.java Outdated
Comment thread paper/src/main/java/vg/civcraft/mc/citadel/activity/RegionCoord.java Outdated
@Diet-Cola
Copy link
Copy Markdown

Very cool and clear as always


record ChunkCoord (short worldId, int x, int z) {
@Override
public boolean equals(Object obj) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need for .equals() or .hashCode() on records unless you're doing something very custom

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

equals - agree
hashCode - I want to have here my own hashing function

sender.sendMessage("In-memory activities: " + longToStr(stat.loadedActivities));

sender.sendMessage(ChatColor.YELLOW + "[ActivityMap] region load statistics:");
sender.sendMessage(Component.text("[ActivityMap] region load statistics:").color(NamedTextColor.YELLOW));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to do Component.text("Message", colour);

What you're doing here is creating a new component with the text "[ActivityMap] region load statistics:", then creating another component that has that text but also the colour YELLOW. So you're creating two components here, not one.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function will be called like 0 - 4 times per day so I think doubled components are not a problem here.
I like a separate color more :)

Copy link
Copy Markdown

@Protonull Protonull Jul 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, it's not the end of the world. Since you aren't fussed and prefer the separation, you could also do

Component.text()
    .color(NamedTextColor.YELLOW)
    .content("[ActivityMap] region load statistics:")
    .build()

as Component.text() returns a builder which you can modify as much as you like.

@okx-code
Copy link
Copy Markdown

okx-code commented Jul 6, 2022

This adds a significant amount of complexity for what is essentially optimistically loading chunks from the database, why is this necessary?

@Aleksey-Terzi
Copy link
Copy Markdown
Author

This adds a significant amount of complexity for what is essentially optimistically loading chunks from the database, why is this necessary?

We have at the moment 3 main problems:

  1. ActivityMap creates too many async tasks which use too many connections. This makes problems not only for ActivityMap by itself but also for other functionality in Citadel that uses connections like reading reinforcements from DB.
  2. ActivityMap creates too many updates, we don't need to update group regions if they have been updated recently. Imagine a bunch of players with similar groups running together. Also, it is enough to update ActivityMap once per 1 - 3 hours, in the scope of 2 months - this is nothing.
  3. ActivityMap very often doesn't contain the required information and it is needed to load data from DB in "main thread"

For example, the recent RB problems were related indirectly to ActivityMap - ActivityMap occupied all connections, Citadel was not able to load reinforcements, this slowed down loading ChunkMeta and RB was not able to get data in time.

@wingzero54 wingzero54 merged commit d495d84 into CivMC:master Jul 22, 2022
wingzero54 added a commit to CivMC/CivDocker that referenced this pull request Jul 22, 2022
@Aleksey-Terzi Aleksey-Terzi deleted the activity_map branch August 13, 2022 22:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants