-
Notifications
You must be signed in to change notification settings - Fork 11
Dynamic tick interval #521
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
Conversation
OhmV-IR
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we risk a race condition here if you are writing to the pylonBlock.tickInterval.ticks value on the main thread while the async thread is reading it to delay the thread? I think you need a mutex setup for read/writes to the tickInterval value
Yes, but if you know your code is running async to begin with you should already know how to handle such situations |
|
Ohm is right. The only way to 'account for it' is by only ever setting tick rate while the block is ticking which makes no sense. I would just make this an atomic int - the performance cost is pretty minimal iirc |
|
What? |
|
I was referring to the recent debate on the performance impact of an if statement. Atomics definitively have more perf impact, so might also be wise to have caution here. |
|
There was no debate on the impact of an if statement. Are you referring to the tick pausing debate? I was concerned with the impact of a map lookup every tick there, not the impact of an if statement lmao. I was under the impression that atomics use compare and set under the hood so the performance impact is negligible, am I wrong on that? |
https://discord.com/channels/1329177304857055273/1329177972631928915/1449122587694202951
Atomic CAS is about 10x slower than a nonatomic CAS due to it requiring taking over cache and possibly invalidating it, and in a contested environment a CAS spinloop is even worse (not saying we shouldn't do this, I was just half-sarcastically drawing comparison to a recent convo lol) |
I get that this is a small thing, but I'd ask that you please don't mischaracterise my words like saying 'debate on the impact of if statements'. It was about the map lookup and not about the if statement itself (as is easily evident in all the context surrounding the message) and I think you can appreciate it's a bit frustrating to say one thing and have someone else take it out of context and then argue that you said something entirely different.
Ok quite curious about this since I've been told that CAS has similar performance impact to regular memory access and I thought this was the whole appeal of lockless concurrency. Can you provide a source or more reading for this? Not finding anything from a quick search |
|
My apologies then, I was legitimately under the impression that we were discussing the performance impact of if statements |
|
Thanks and no worries |
|
https://joeduffyblog.com/2009/01/08/some-performance-implications-of-cas-operations/ Most of the performance impact of (uncontested) CAS is in managing cache coherency |
|
Ok, interesting I didn't know that. Agree 100% that it's fine to do though - the pausing situation was mostly just me being confused that coroutines don't have an inbuilt way to do this, and I wanted to wait until the custom scheduler thing to be on the safe side that I'm writing performant code. I think the conversation ended up blowing up a bit in proportion to the issue lol. It seems necessary here anyway |
|
comments to lines of code ratio on this pr is crazy 💀 |
|
To reiterate because that was a lot of talk about nothing - tick rate should become an atomic boolean to prevent race conditions |
Atomic int you mean |
|
O wait just read convo lol |
pylon-core/src/main/kotlin/io/github/pylonmc/pylon/core/block/base/PylonTickingBlock.kt
Outdated
Show resolved
Hide resolved
5eea681 to
04319ac
Compare
OhmV-IR
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs the volatile then its lgtm
|
Fixed by whatever PR/commit nuked the TickManager |
No description provided.