search

Civcraft / Citadel

Do not open issues here; open them on the maintained fork @ DevotedMC
https://github.com/DevotedMC/Citadel
BSD 3-Clause "New" or "Revised" License
6 stars 23 forks source link

Decreasing location precision to make the cache work better (not tested) #164

Closed Sander-Kastelein closed 8 years ago

Sander-Kastelein commented 8 years ago

I think adding a function such I did in this pull request should make the caching more efficient, it removes the digits after the decimal point in order to make the .equal() function (and .hashCode()) to work for 2 different Location objects with the same integer XYZ coordinates. This should save some SQL queries on blockchange events preventing the Main thread from freezing.

Greets,

CivcraftBot commented 8 years ago

Can one of the admins verify this patch? Type 'ok to test' to test.

ProgrammerDan commented 8 years ago

I'll check this out tonight.

Did you do any speedtests using this change? Was there a difference?

Sander-Kastelein commented 8 years ago

Unfortunately I haven't the time to setup a testing environment right now (I might in the future), I stumbled upon the code you currently use (for learning purposes) and I thought the precision of the XYZ coordinates in the cache might be a problem.

If a location at: 5.34335,64.255,-24.654334 Has an Reinforcement object (RO1) it will be saved in the cache as: 5.34335,64.255,-24.654334 => RO1

If I lookup is done for: 5.12451,64.001,-24.12395 It will (I think) query the database for a Reinforcement object where (x = 5, y=64, z=-24) Which is already in the cache!

By stripping everything after the decimal point for the coordinates this can be prevented.

5.34335,64.255,-24.654334 => RO1 Will become 5,64,-24 => RO1

making it so that if a lookup is done for 5.12451,64.001,-24.12395 it will look in the cache for 5,64,-24 which will return RO1, without querying the database.

Again, (I can't stress this enough) I haven't had the time to measure if this is saving any CPU, or even if it doesn't break anything. It's just something I thought was odd while reading the source and try to bring up.

ProgrammerDan commented 8 years ago

Definitely appreciate the thought. My concern is if creating new Location objects on every cache request will ultimately be more expensive then potentially higher cache misses ... hence benchmarking / speedtesting might be in order.

This does give me some good food for thought though. I've been meaning to look closer at the cache b/c of some intermittent NPE's during runtime for Devoted (a server I admin that uses this plugin) ... thanks :).

Sander-Kastelein commented 8 years ago

I'm not (yet) familiar with the specifics of Java objects and their performance, but flooring the current values in the current object could also work (just have to check the object isn't reused further down the line where more specific coordinates are needed).

ProgrammerDan commented 8 years ago

The method to drop the decimal values isn't at question, allocating new memory for a new Location object is :)

Just reread your comment and realized I misunderstood. I'd have to check if the internal representation was immutable or not (within Location) ...

Ok, so yeah, you could just modify the passed-in Location object. That might be worth trying, no idea if any secondary effects.