Optimize redstone algorithm #1494

egg82 · 2018-09-27T08:32:10Z

Patch that implements theosib's redstone algorithms.

Tons faster, more stable, etc. Includes a config option to turn it on; off by default.

lucko · 2018-09-27T08:55:30Z

Spigot-Server-Patches/0381-Optimize-redstone-algorithm.patch

+
+    public static boolean useNewRedstone = false;
+    private static void useNewRedstone() {
+        useNewRedstone = config.getBoolean("settings.use-new-redstone", false);


IMO a better name for this setting would be use-theosib-redstone.

There are a number of alternate redstone algorithms, in particular the Panda4994 system - which is quite popular, and available as a Bukkit plugin, Forge mod & Sponge config option. Just using "new redstone" doesn't really describe the feature very clearly.

In fact, perhaps a string value would make more sense, in case another algorithm is added in the future.

e.g.

redstone-algorithm: vanilla redstone-algorithm: theosib redstone-algorithm: panda

Yup, latter is better and easier to implement as well

Better name definitely needed, but as discussed in discord we weren’t planning on implementing pandas algorithm

Yeah, the name can probably be better. I'm terrible at those. I suggested implementing something similar to what @lucko was saying because I had spent most of the day trying to port Sponge's version of Panda's algorithm but it's too buggy, touches too many files, and overall this one is better and likely faster.

Even if there's no current intention of porting Panda's, another system may pop up in the future that's worthwhile.

No harm in using a generic setting now (my second example above) just to ease that process down the line.

Reading the original author's comments there's some room for improvements. I omitted some commented-out code from the original and there's areas in which the author states one function would be preferred over another or caching various results would be a better option in some places. So there's definitely room for future patches to make things better, I just wanted to get the groundwork up and.. Well, working.

I could potentially possibly see a new thing coming up in the future that's better, but at that point would it be prudent to simply replace this patch with that one? I'm not sure, honestly.

The only harm is that it is less clear to users.
I'd rather see this remain a boolean, given that there are no intentions of adding more algorithms, and if we're wrong we can add a migration later.

Hi, everyone.

I'm very flattered that my redstone accelerator was chosen for inclusion in Paper. I was asked about naming, and my suggestion would be to call the setting "use-eigencraft-redstone". It is a higher priority for me to draw attention to the EigenCraft community than to make sure everyone sees my pseudonym or whatever.

Thanks.

kashike · 2018-09-27T21:27:49Z

Spigot-Server-Patches/0381-Optimize-redstone-algorithm.patch

+ * Ported class names, updated methods, added events, etc
+ * Original comments are (at least mostly) preserved
+ * 
+ * @author egg82


I'd drop this.

Was mostly following Aikar's previous examples. The author bit is a bit much, I agree.

kashike · 2018-09-27T21:27:52Z

Spigot-Server-Patches/0381-Optimize-redstone-algorithm.patch

@@ -0,0 +1,1185 @@
+From 6a5104193614fbe321dca74d32646a4ad33595fa Mon Sep 17 00:00:00 2001
+From: egg82 <phantom_zero@ymail.com>


I'd say it's worth using @theosib here - he wrote 99% of this.

I credited the original author multiple times. Although a LOT of the work is his I still had to learn how it worked and get it to play nicely with Paper.

You can always add yourself as co-author:
https://help.github.com/articles/creating-a-commit-with-multiple-authors/

Worked it out in Discord. My intent isn't to steal credit for work. There's no way I'd ever want to do that. The patch is basically theosib's, but it still took me a fair amount of hours, work, and understanding to implement it correctly and get it to play nicely with Paper and Bukkit's events. I just didn't want that to go by without having a small mention.

The solution we collectively came up with was to replace that header with a small "Ported to Paper and updated to 1.13 by egg82" and add GitHub's Co-authored-by header to the patch while attributing the real Author header to theosib

I appreciate the effort you put into adapting this code and you should not be shy about getting credit for that contribution. There are multiple ways to handle this. You can name me as the author and mention prominently in notes or other metadata that you did a lot of work to port it. Or you can say that you are the author of THIS VERSION of the accelerator, and mention in appropriate places that you adapted it from my code. Also, I would suggest adding comments to the code that explain the lineage and revision history. I don't feel strongly about any of this. I did this initially just to for an interesting challenge and to improve redstone performance. But when I showed it to Grum, he of course had to make up all sorts of reasons why it was inadequate, essentially giving me what he thought was an impossible combination of requirements. He didn't know who he was dealing with. :) Anyhow, I would be perfectly happy if Mojang were to absorb this code, and I wouldn't want any attribution requirements to be a stumbling block. Same goes for Paper. I'm leaving ethics of proper attribution to you; whatever you come up with, I'm sure I'll be fine with it.

Quick edit: It feels like I'm digging myself a hole, here. Basically what kashike was suggesting was to completely remove my name from the entire patch altogether. Since I follow a philosophy of "credit where credit is due" I felt like doing this was cheating me out of my contribution to this patch. While I will never try to take credit for things that aren't mine, I still felt a footnote somewhere saying "hey, egg ported this over" was earned.

Honestly I would feel bad if I tried to claim it as my own. Through reading and understanding your code it's obvious that the brilliance of the thing is in its simplicity, and the only major complexities presented are in keeping with vanilla behavior and actually linking it into the Mojang server. BUT there's still a ton of work that went into the thing so I would be very uncomfortable trying to take credit for it. I wouldn't even call it a new version, so I wouldn't try to claim that either.

I'm honestly more than fine with you taking the vast majority of the credit- I just didn't want my name to be completely lost when I spent a fair portion of my night (and half of my morning as well) actually getting it to work on Paper is all. For me, it's not about trying to take credit. It's just about appropriately crediting the people who worked on it- and since that includes a few tiny contributions from myself, my name as even just a footnote somewhere is more than okay. I kept the comments about other contributors in for exactly the same reason ;)

mibby · 2018-09-28T08:53:20Z

How does this compare performance wise to Panda's algorithm?
https://gist.github.com/Panda4994/70ed6d39c89396570e062e4404a8d518

As seen implemented in PandaWire.
https://www.spigotmc.org/resources/pandawire.41991/

egg82 · 2018-09-29T03:03:24Z

I went ahead and did a little profiling.

The setup

Paper 1.13.1 with Aikar's flags (<10GB version), async chunks & redstone patch
Three layers of 30x30 redstone wire powered by a lever directly in the center layer
JProfiler

Screenshots of the contraption:
http://prntscr.com/l01lyp
http://prntscr.com/l01lve
http://prntscr.com/l01lrm
http://prntscr.com/l01m24
http://prntscr.com/l01m53
http://prntscr.com/l01mee

The method

Start server up without new redstone enabled (vanilla). Log in, look around to ensure all chunks load/render. Start profiling. Wait about 30 seconds.
Start toggling the switch. Do this exactly 30 times. Stop profiling.
Profit?
Repeat the above steps but with new redstone enabled (theosib/eigencraft)
Repeat again with PandaWire plugin (if you have it)

The results

Vanilla: https://prnt.sc/l01jmr ~1136ms total
Theosib/Eigncraft: https://prnt.sc/l01hme ~355ms total
PandaWire plugin: http://prntscr.com/l01qmn ~202ms total

The conclusion

As you can see, the vanilla method is vastly outclassed by both algorithms. Either one would be a huge improvement over it. The PandaWire plugin is about a 43% improvement in speed over theosib's algorithm.

The problem with PandaWire is it breaks a lot of redstone machines. Panda4994 actually adresses these in a video here, whereas theosib's algorithm keeps vanilla behavior. Both of these algorithms do seem to fix bugs listed on Mojang's tracker, but Panda creates its own bugs in place.

If the goal is to have a patch that decreases time spent on redstone updates but still keeps with vanilla behavior, this is the patch to do it.

Cryptite · 2018-09-29T12:51:12Z

I'm glad you did a test on this specific contraption, since this exact contraption is actually a "Doomsday Device" and was recently used to bring down my server in the not-too-distant past. Without any modifications in vanilla 1.12, spamming this contraption on/off was able to lag out the server and eventually cause a restart.

stale

zachbr · 2018-10-28T22:36:31Z

Rebased onto master, moved global config to a world config.

This needs a final look over and probably one last testing session thing. Community help with testing appreciated.

Edit: Testing jar: See below

Set use-faster-eigencraft-redstone: true in paper.yml under world settings

zachbr · 2018-10-30T02:52:07Z

Updated jar compiled w/ Java 8 for those of you having problems with the last one. https://zachbr.keybase.pub/paper/github/pulls/1494/paperclip-5bb177f.jar

Set use-faster-eigencraft-redstone: true in paper.yml under world settings

Spigot-Server-Patches/0402-Optimize-redstone-algorithm.patch

Tohya1 · 2018-10-31T23:06:03Z

Setting use-faster-eigencraft-redstone: true in paper.yml under world settings prints theosib to the log.

[09:54:46 INFO]: Using new redstone algorithm by theosib.

The Log should say Using Eigencraft redstone.

I had some problems making a 2 headed piston, but that was just a timing issue. So far It seems to work quite well.

zachbr · 2018-11-15T03:11:20Z

Thanks for all the work porting this, and to theosib for all the work that went into creating it.
1.13 builds 446+

lucko reviewed Sep 27, 2018

View reviewed changes

kashike previously requested changes Sep 27, 2018

View reviewed changes

aikar added type: feature Request for a new Feature. type: performance labels Sep 28, 2018

aikar added this to In progress in Performance Improvements via automation Sep 28, 2018

zachbr force-pushed the feature/redstone branch 2 times, most recently from 37b229a to cd732d7 Compare October 28, 2018 22:34

zachbr requested review from zachbr, aikar, electronicboy and kashike October 28, 2018 22:35

zachbr force-pushed the feature/redstone branch from cd732d7 to 5bb177f Compare October 30, 2018 02:09

kashike requested changes Oct 31, 2018

View reviewed changes

Spigot-Server-Patches/0402-Optimize-redstone-algorithm.patch Outdated Show resolved Hide resolved

zachbr approved these changes Nov 14, 2018

View reviewed changes

Optimize redstone algorithm

1ee9cbf

zachbr force-pushed the feature/redstone branch from de031fa to 1ee9cbf Compare November 15, 2018 03:03

zachbr merged commit 0b9983d into PaperMC:master Nov 15, 2018

Performance Improvements automation moved this from In progress to Done Nov 15, 2018

NoahvdAa mentioned this pull request Jan 6, 2022

Add Alternate Current redstone implementation (and make Eigencraft default) #7291

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize redstone algorithm #1494

Optimize redstone algorithm #1494

egg82 commented Sep 27, 2018

lucko Sep 27, 2018

mikroskeem Sep 27, 2018

zachbr Sep 27, 2018

egg82 Sep 27, 2018 •

edited

lucko Sep 27, 2018 •

edited

egg82 Sep 27, 2018

zachbr Sep 27, 2018

theosib Sep 27, 2018

kashike Sep 27, 2018

egg82 Sep 27, 2018

kashike Sep 27, 2018

egg82 Sep 27, 2018

Mystiflow Sep 27, 2018

egg82 Sep 27, 2018

theosib Sep 28, 2018

egg82 Sep 28, 2018 •

edited

mibby commented Sep 28, 2018

egg82 commented Sep 29, 2018 •

edited

Cryptite commented Sep 29, 2018

zachbr commented Oct 28, 2018 •

edited

zachbr commented Oct 30, 2018

Tohya1 commented Oct 31, 2018

zachbr commented Nov 15, 2018

		@@ -0,0 +1,1185 @@
		From 6a5104193614fbe321dca74d32646a4ad33595fa Mon Sep 17 00:00:00 2001
		From: egg82 <phantom_zero@ymail.com>

Optimize redstone algorithm #1494

Optimize redstone algorithm #1494

Conversation

egg82 commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egg82 Sep 27, 2018 • edited

Choose a reason for hiding this comment

lucko Sep 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egg82 Sep 28, 2018 • edited

Choose a reason for hiding this comment

mibby commented Sep 28, 2018

egg82 commented Sep 29, 2018 • edited

The setup

The method

The results

The conclusion

Cryptite commented Sep 29, 2018

zachbr commented Oct 28, 2018 • edited

zachbr commented Oct 30, 2018

Tohya1 commented Oct 31, 2018

zachbr commented Nov 15, 2018

egg82 Sep 27, 2018 •

edited

lucko Sep 27, 2018 •

edited

egg82 Sep 28, 2018 •

edited

egg82 commented Sep 29, 2018 •

edited

zachbr commented Oct 28, 2018 •

edited