ID:1810538
 
Welcome back to another Sunday. Today I wanted to talk to you about optimizing code. Chances are, if you are asking yourself the titular question, you've got something going on in your project that involves a loop. We're going to look at some common patterns that I see in code snippets that get posted around here, and how you can improve them. Let's dive right in.

Why loops?

for(var/sometype/s in somelist)
//do something


The for...in loop is a very common and useful pattern for a lot of approaches in game design. What it does, is repeat a section of code for every instance of an object in the supplied list. This pattern is actually where I see a lot of attempts to write systems in DM go wrong. I'm not saying there's anything wrong with loops. They are useful. Use them. But you have to understand how DM works behind the scenes to really use this pattern to its fullest.

The for...in loop has three main components:

for(specifier in list)
action


1) The specifier. The specifier tells DM's interpreter what you are looking for inside of the supplied list.

2) The list. The list tells DM's interpreter where to look for the specifier.

3) The action. The action tells DM's interpreter what to do when something matching the specifier is found in the list.

You should be aware that behind the scenes, DM has to search every element of the supplied list in order to find every item in the list that matches the specifier. This means that the list actually does a bit more than you tell it to.

The larger the list that you supply to the for...in loop, the more items it has to check against the specifier. So even if the item doesn't match the specifier, the VM checking to see if it matches takes a very small amount of CPU time. This is all done for you automatically in DM's backing C code, so it's going to be a lot faster than doing it manually. However, there will be a point where a very immensely large list will really start to bog down the world when you iterate through it.


for...in world

world.contents is generally the largest list in any project. This means that supplying the world's contents to a for..in loop is almost always going to be the slowest thing you can do.

If you ever have a piece of code that has to search through the world to find something specific, odds are that you have used the least-optimal approach possible. Also, if you ever write a piece of code that has to search through any list looking for anything specific, odds are you are using the for..in pattern wrong in the first place. There are correct uses of this approach, but generally if you aren't calling the action block of the for...in pattern on every element of the supplied list, you are not taking advantage of proper design patterns. Again, this is not always the case, but it almost always is.

Let's look at some common examples and how to improve them.

mob
verb
GlobalChat(msg as text)
src << "You say: [msg]"
for(var/mob/M in world)
if(M!=src)
M << "[src]: [msg]"


This is a common Global Chat approach that you see in a lot of games. Unfortunately, it's not very efficient. Let's take a look at a better approach:

var
list/players = list()

mob
Login()
..()
players += src

Logout()
players -= src
src.loc = null

verb
WorldChat(msg as text)
var/list/receivers = players-src
src << "You say: [msg]"
receivers << "[src]: [msg]"


This approach doesn't make a major speed difference in small worlds, but the bigger your world gets, the faster this approach gets compared to the old one.

Now, you might be asking yourself: "How would I have known to do that?". The answer is simple. You should always store data that you are going to need again later somewhere. Searching through a list to find a series of items that match a particular specifier should be avoided, and specific items that need to be hung onto should be stored in list variables somewhere in your code for later use. It does increase the size of your code a little bit, and it's a little harder to design this pattern than just depending on for...in, but if you rely on for...in, your games will get progressively slower and slower until finally you are convinced that BYOND itself is slow.

This is not an avanced technique. This is a very basic design pattern that you should be learning very early in your programming career. BYOND is unique in that for...in abuse is rampant here. It's been spread as a bad habit by a lot of popular programming examples around here, and it's even made its way into Space Station 13, and just about every rip source that exists. It's not just bad programmers that do this. I see some of the very best DM developers misuse these patterns frequently.


And then there was one:

Let's look at another common bit of code that I see a lot. People use loops to update single objects in DM. For instance:

mob
Login()
client.screen += new/obj/screen_obj/something()
proc
UpdateSomething()
for(var/obj/screen_obj/something/s in client.screen)
s.icon_state = "updated"


This approach is very wasteful. It's immensely slow, and only gets slower the more stuff you pack into the client's screen. Let's fix it.

mob
player
var/tmp
obj/screen_obj/something/some_name
Login()
some_name = new()
client.screen += some_name
proc
UpdateSomething()
some_name.icon_state = "updated"


See how easy that is? Plus, no-loop, no CPU issues later. You can store the screen object in a variable and just use that variable handle rather than having to search through the screen list every time you want to use it. Hanging on to information that you are going to need to use later is not an advanced technique either. This is introductory stuff. As a rule of thumb: If you need the data again later, it should be stored in a variable in a sensible place. I catch self-described "DM pros" failing to store data properly and using loops to retrieve it all the time. Beware this type of design, because it's actually more work to do it that way, and it's slower by a huge margin.


Find a friend

Here's another common example of where people go wrong. When you need to find a specific object of a specific type. Here's an example of what you see done a lot:

mob
verb
Whisper(player as text,msg as text)
for(var/mob/M in world)
if(M.client && M.name==player)
src << "to [M]: [msg]"
M << "[src] whispers: [msg]"
return
src << "[player] not found."


We can improve this by storing the players in an associative list by their name. You might not think this will make a significant difference in speed, but you'd be surprised. Associative list indexing gives you constant O(1) time, while searching a list gives you O(n) linear time. Constant time is the absolute minimum threshold for optimization for a pattern you can get to. Grabbing an associative index does not require the underlying engine to search the whole list, thus it will be faster than searching the list in every instance.

var
list/player_names = list()
mob
Login()
..()
player_names[src.name] = src
Logout()
player_names.Remove(src.name)
verb
Whisper(player as text,msg as text)
var/mob/M = player_names[player]
if(M)
src << "to [M]: [msg]"
M << "[src] whispers: [msg]"
else
src << "[player] not found."


Associative lists are very useful for storing data, and they are extremely fast. You just have to remember to clean up unused references and keep in mind that any referenced object cannot be garbage collected, so always, always, always remember to do your cleaning.


Conclusion:

Loops and avoiding storing data you are going to need for later is why your code is slow 90% of the time. You need to look at what you are doing, and figure out when you NEED to use loops, and when you don't. There are a lot of ways to avoid excess processing of lists during iteration, and there are a lot of patterns to fix up problematic logic. I can't cover them all here. Just keep in mind that if there's a way to do it without the loop, do that. However, don't avoid loops without reason. If find yourself writing repetitive code, odds are that you want to use a loop instead. There is a time and a place for a loop. The trick is knowing when and how.
I just thought of an addendum to Snippet Sunday #6 that I should have mentioned:

The datum.tag variable.

Tag is a useful way to store references to objects without interfering with the garbage collector. A tag is a unique string that you can use to identify an object in DM. You can retrieve the object by its tag later:

var/datum/d = locate("sometag")


Internally, DM stores a Map (fancy word that means associative list) of all objects that have been given a unique tag. Storage in this list doesn't increment the reference count of the object. This is important, because in DM when an object is referenced (stored in a variable), the reference counter is increased for that object. When a reference to that object is cleared (either the variable being undefined or the variable being reassigned to another value), the reference counter is decreased. When the reference counter reaches zero for an object, the garbage collector catches it and frees up the object's memory. This can cause other objects referenced by the garbage object's variables to also drop into the garbage collector.

This is why I caution that you need to keep a good eye on your object references, because explicitly deleting objects using the del keyword is immensely slow. It's better to keep clean practices and clean up object references when they are no longer needed. Otherwise, you risk memory leaks.

The tag allows you to identify that you may need to get an object in the future, but also not bother hanging on to that object in memory if it's not needed anymore. The tag acts like a "weak reference", whereas putting an object into a variable is a "strong reference".

locate(tag) is actually really fast, because it doesn't need to search the entire world for the object. It just does a Map lookup on the internal tag table, which is basically the same thing as an associative list lookup. As I mentioned before, association is fast.

If there's a situation where you have an item that can be uniquely identified by a string easily, you can bypass maintaining extra global or instance associative lists by simply using the tag variable and the locate(tag) instruction.
These are some perfect examples. It bothers me when people do a "Who's Online" system and messaging using for(var/mob/M in world) as opposed to defining a list and adding the users to the list upon connection, then removing them when they disconnect.
Plus its easier to find the player count by simply calling list.len
Rather than
var/count = 0; for(var/mob/M in world) {if(M.client) { count++ } }; src<<"[count] players online";
Thanks for your seal of approval, Dale. Not storing enough information in variables and storing the wrong information in variables is the number one reason why most BYOND games have "lag" problems.

Optimizing isn't just about having a ton of knowledge about DM. It's about understanding common design patterns and understanding the logic flow of your game.

Unfortunately a lot of bad habits get passed around because people learn from ripped codebases. Learning to program is a slow, incremental process and trying to run before you can walk leads to big headaches down the road. It's best to learn the standard patterns and the logic of development before just diving into a leaked codebase and performing a long game of "monkey see, monkey do".
In response to Ter13
The problem isn't the poor quality of codebase around, it's that people, for whatever reason, think that that's how it's done and the best way to do it. I started with a ripped source, which you can find in my creations. Zeta rip run by a friend that I took over for, I went in knowing almost nothing about programming, as you could probably tell based on the early parts of the source I touched and never retouched. As I progressed, through trial and error, I got better and garnished my own habits, since I didn't use the forums or guide to learn off of. After a while, I started helping on the forums and reading posts from others, recently yours are the only ones to share knowledge I care to learn. As I found out about new ways to approach the code I wanted to make, my style changed. You can pretty well track my progress through that rip, as there are parts I touched at very different times over the last three or four years. The stuff from my last string of active development(since most of the last year has just been five minutes here, twenty there) was a retouch on the scoreboard aspect.

Rundlement, a personal project I started about two years ago, has received much more work from me than anything else programming wise in the last year and would be the best example of where I am now, but I started from a really poor zeta rip. \life story
In response to Ter13
Ter13 wrote:
Learning to program is a slow, incremental process and trying to run before you can walk leads to big headaches down the road. It's best to learn the standard patterns and the logic of development before just diving into a leaked codebase and performing a long game of "monkey see, monkey do".

I really wish I'd listened to this advice 20 years ago when I started playing around with programming, minus the 'leaked codebase'. I've developed a ton of bad habits over the years, mostly from trying to do 'cool stuff' instead of just learning the basics, and it is really hard to break them. I am pretty happy about the fact that I'm not really guilty of these particular abuses, although I'm sure I'm guilty of plenty of others :P

I, and I'm sure many others, do appreciate the tutorials/snippets/demos that you put out here and throughout the forum. I try and learn from them, though I am pretty old and set in my ways...

Thanks.
The problem isn't the poor quality of codebase around, it's that people, for whatever reason, think that that's how it's done and the best way to do it.

It's cyclical. Most people on BYOND learn from codebases, or by hiring people who learned from codebases.

Next to nobody learns from proper programming tutorials. DM has been taught by pass-along knowledge for the last twelve years, since most of my generation's gurus moved on.

And cheers, Flick.
Thanks for writing this one up. This is a great series you've got going on. I actually haven't messed with on-screen objects and such much until recently. Just a little on and off. I've researched a lot of different libraries, and other material on the subject, but I think I would of made some mistakes had I not seen this post.

I'm pleased to say I learned programming in DM primarily from the DM guide, Zilals tutorials, discussion with friends, and general trial and error. That being said, it still hasn't stopped me from acquiring at least a few bad habits and going through Snippet Sundays has already eliminated at least two for me.
I am struggling putting the knowledge you posted here into practice onto a sealing verb in my game.

I ain't no seasoned coder, and I already posted a post in developer help, and read this guide thoroughly several times. I just don't seem to understand the following:

I am trying to set a tag when sealing away a Mob or Player to a location which procs Sealingproc() that uses spawn to time when they should be teleported out of that place to the player that used the verb on them. I tried that by doing this:

That's part of the sealing verb:
B.tag="[usr]"
B.sealingproc()


This is the sealingproc()
sealingproc()
for(var/mob/M in world)
if(M.sealed==1&&M.con>80)
sleep(200)
new/obj/immortality(M.loc)
sleep(10)
new/obj/immortalityfire(M.loc)
sleep(1)
M.alpha=0
sleep(1)
new/obj/immortality(locate(M.sealer))
sleep(10)
new/obj/immortalityfire(locate(M.sealer))
sleep(1)
M.alpha=initial(alpha)
M.sealed=0
M.loc=(locate(M.sealer))


All that does is make the Mob or Player teleport to 0,0,0 so I am pretty sure I am messing up quite a bit.
A few pointers:

I wouldn't recommend setting the tag to the player's name. I'd recommend using a special format:

B.tag = "@player:[usr.name]"


^I'd suggest doing this after the player logs into their character, or after they successfully name their character. If you want to easily get a specific player by name, you should just set the tag for the player one time. There's no need to set it inside of a specific proc later on after they've already joined the game.

The second thing: Make sure you set the sealer variable to the player's name that sealed this mob.

B.sealer = usr.name


Now, when you want to locate a player using their unique tag:

var/mob/m = locate("@player:[M.sealer]")


You should know that locate(tag) returns the object itself, not the object's location. The code you are using will put the sealed player inside the contents of the sealing player. If the sealing player logs off, you need to make sure that the mob is removed from their contents, otherwise you may end up saving other peoples' players inside savefiles you don't mean to.

Lastly: My tutorial talked explicitly about never using for(var/mob/M) in world. During our PM session you claimed to have read it and had no real problems understanding it. You also claimed to have read it and had problems understanding it. You also insulted the length and the quality, and you also said you weren't coming here just looking for me to fix your code for you... But do you remember during our PM conversation when I told you that you hadn't actually learned from those rip sources you said were a better guide than my tutorials? You are making a horrible mistake here because you don't understand something very basic. This is page 1 material in the DM guide. This function should not be looping through every mob in the world to find the current person that's been sealed. You already know who that is, because it's always src.

    sealingproc()
for(var/mob/M in world) //Are you sure you read this tutorial?


^This is bad.

    sealingproc()
if(!sealed) return
if(con>80)
sleep(200)
new/obj/immortality(loc)
sleep(10)
new/obj/immortalityfire(loc)
sleep(1)
alpha = 0
sleep(1)
var/mob/M = locate("@player:[sealer]")
new/obj/immortality(M.loc)
sleep(10)
new/obj/immortalityfire(M.loc)
sleep(1)
alpha = initial(alpha)
sealed = 0
loc = M //you might change this.


Of course, there's a lot more that can be improved, but because I have talked to you for two hours, and have been unable to get a specific question out of you other than "fix my code please" (despite the fact that you keep claiming that you aren't just trying to get me to fix your code), I can't really help you.

In fact, this entire post isn't going to help you until you realize that just asking for code snippets is only going to make you a worse programmer in the long run.
Thanks for explaining I understand it a bit more now, but it doesn't teleport the mob to the player after 20 seconds.

So I am still doing quite a bit wrong here, I guess it can't be helped and my skills aren't adequate enough to be even attempting this :)

Thanks for helping though. Also you say I was disrespectful, but you take PM's to the forums, and that's a dick move. Thanks for that, I will be pager banning you now, cause I can't deal with people this disrespectful to people.
I'm sorry you've taken offense. In the future, starting out a conversation by calling a tutorial that was written for your benefit a "wall of text" is not a good first step to asking for help.

I'm still open to input on this particular article, and how to improve it, but I can't read minds. I need your help to understand what you don't understand, and I've asked for specifics on what's wrong with the article a half a dozen times now.

I'm trying to teach you to fish. If you are just looking for fish, hire a programmer.
You made me take offense, I wouldn't if you didn't bring private messages into this. You claim not to try and treat people like thrash then proceed and do this, not cool.

I understand you're willing to help, but by putting people down like this, isn't going to help them.

On-topic: I tried your suggestions, they compile nicely, they don't give any warnings nor errors.

It just doesn't do anything, I need to add thought that
B.tag = "@player:[usr.name]"


Was supposed to be the tag for sealer, and is obsolete, just forgot to remove that.

so it's just
B.sealer = usr.name


sealingproc()
if(!sealed) return
if(con>80)
sleep(200)
new/obj/immortality(loc)
sleep(10)
new/obj/immortalityfire(loc)
sleep(1)
alpha = 0
sleep(1)
var/mob/M = locate("@player:[sealer]")
new/obj/immortality(M.loc)
sleep(10)
new/obj/immortalityfire(M.loc)
sleep(1)
alpha = initial(alpha)
sealed = 0
loc = M //you might change this.

and

sealing()
if(sealingcd||swim||dead||resting||seals)return
var/list/choose=list()
for(var/mob/B in view(8))choose.Add(B)
var/cancel="Cancel"
choose+=cancel
var/mob/B=input("Who do you want to seal?") as null|anything in choose
if(!B||B==cancel)return
if(B.village=="[usr.village]")
src<<"<b><font color=white>You can't seal your fellow village members!"
return
usr.handseals(200,50)
if(!sealpass)return
sealingcd=1
for(var/obj/jutsu/uchiha/Sealing/M in src)timerjutsu(900,M)
spawn(900)sealingcd=0
B.freeze=1
new/obj/immortality(B.loc)
sleep(10)
new/obj/immortalityfire(B.loc)
sleep(1)
B.alpha=0
sleep(1)
new/obj/immortality(locate(20,10,6))
sleep(10)
new/obj/immortalityfire(locate(20,10,6))
sleep(1)
B.alpha=initial(alpha)
sleep(1)
B.loc=(locate(20,10,6))
B.freeze=0
B.sealed=1
B.sealer = usr.name
sealingproc()

Are the full proc and verb, and I am sure there's way better ways of going around that, ofcourse. Reason behind how I did this, was because I tried to keep the code the same as the rest of the verbs, as to not make it more messy and confusing.

Now, once more, I will try and say this: Sorry if I came across as a huge dick, it wasn't my intention. We started on the wrong foot, so I hope we can rewind and do it all over if you want.
We started on the wrong foot, so I hope we can rewind and do it all over if you want.

That's fine. I'm game for that. I will be forced to keep bringing things up from our prior conversation, though, because some of the stuff you are taking as insults really aren't insults. They are observations that I'm making based on what you are showing me in your code. I really hope you can realize that a few of these observations aren't meant to be put-downs, but just statements of fact that you need to take to heart in order to improve your knowledge of programming. It's hard. I understand, to take criticism. It's just unfortunately that programming takes time and a lot of effort to learn, and taking shortcuts just makes it take longer.

The reason why your sealing procs don't do anything right now is because you don't understand the concept of "usr and src". If I can, I'd really recommend reading Xirre's tutorial on the difference between usr and src:

http://www.byond.com/forum/?post=1789464

Also, you seem to be having difficulty understanding what variables are, and what values they contain.

I'd really recommend a good read of The DM Guide's chapter 5:

http://www.byond.com/docs/guide/chap05.html


Anyway, let's examine why your problem is what it is:

sealing() is a proc (or verb, judging from usr usage, but you never know with rip source code) that's called on the person doing the sealing.

Inside of sealing, "both src and usr" will be the person doing the sealing.

Inside of sealingproc(), however, src should be the person who was sealed by the usr of sealing(). Inside of sealingproc(), src will be the person usr chose to seal.

You actually don't even have to use tag at all in your example unless you want to locate the sealing player elsewhere, which I assumed you would want to anyway.

Let's do some more fixing of your code to account for this:

        sealing()
if(sealingcd||swim||dead||resting||seals)return
var/mob/B=input("Who do you want to seal?") as null|anything in view(8)
if(!B)return
if(B.village==usr.village)
src<<"<b><font color=white>You can't seal your fellow village members!"
return
usr.handseals(200,50)
if(!sealpass)return
sealingcd=1
for(var/obj/jutsu/uchiha/Sealing/M in src)timerjutsu(900,M)
spawn(900)sealingcd=0
B.freeze=1
new/obj/immortality(B.loc)
sleep(10)
new/obj/immortalityfire(B.loc)
sleep(1)
B.alpha=0
sleep(1)
new/obj/immortality(locate(20,10,6))
sleep(10)
new/obj/immortalityfire(locate(20,10,6))
sleep(1)
B.alpha=initial(alpha)
sleep(1)
B.loc=(locate(20,10,6))
B.freeze=0
B.sealed=1
B.sealer = usr.name
B.sealingproc()


And to clean up the sealingproc a bit:

    sealingproc()
if(!sealed) return
if(con>80)
sleep(200)
new/obj/immortality(loc)
sleep(10)
new/obj/immortalityfire(loc)
sleep(1)
alpha = 0
sleep(1)
new/obj/immortality(usr.loc)
sleep(10)
new/obj/immortalityfire(usr.loc)
sleep(1)
alpha = initial(alpha)
sealed = 0
loc = usr //you might change this.


It's okay to use usr in sealingproc(), because it's being called from a verb.

The only time you'll want to use locate(tag) is when you wouldn't otherwise already have access to the usr. I didn't realize that what you were doing was being called from a verb until I took a look back a few minutes ago, so don't worry about using locate(tag) in the sealingproc() function --it's not necessary. Only use it when you don't have access to the sealer already.

Enjoy your fish.
It compiles just fine every single time, in various ways, it "seals" the thing I chose to the place I specified.

It just doesn't proc the sealingproc() and returns the mob to my location. It's just stuck there, not doing anything, even with your specified adaptations.

Is this because both the proc and sealing verb are in different dm files(procs and uchiha jutsus.dm respectively)

This is just so odd... I don't know what's wrong with the code...

Show me what you changed it to?
sealing()
if(sealingcd||swim||dead||resting||seals)return
var/mob/B=input("Who do you want to seal?") as null|anything in view(8)
if(!B)return
if(B.village==usr.village)
src<<"<b><font color=white>You can't seal your fellow village members!"
return
usr.handseals(200,50)
if(!sealpass)return
sealingcd=1
for(var/obj/jutsu/uchiha/Sealing/M in src)timerjutsu(900,M)
spawn(900)sealingcd=0
B.freeze=1
new/obj/immortality(B.loc)
sleep(10)
new/obj/immortalityfire(B.loc)
sleep(1)
B.alpha=0
sleep(1)
new/obj/immortality(locate(20,10,6))
sleep(10)
new/obj/immortalityfire(locate(20,10,6))
sleep(1)
B.alpha=initial(alpha)
sleep(1)
B.loc=(locate(20,10,6))
B.freeze = 0
B.sealed = 1
B.sealer = usr.name
B.sealingproc()

and
sealingproc()
if(!sealed) return
if(con>80)
sleep(200)
new/obj/immortality(loc)
sleep(10)
new/obj/immortalityfire(loc)
sleep(1)
alpha = 0
sleep(1)
new/obj/immortality(usr.loc)
sleep(10)
new/obj/immortalityfire(usr.loc)
sleep(1)
alpha = initial(alpha)
sealed = 0
loc = usr //you might change this.


Just like you said. It works fine up until the point sealingproc() is called I guess, as it does teleport to 20,10,6 but doesn't return to me.

Edit: might be any use perhaps:

B in sealing is used for the mob, not the usr.
When you run into errors that you can't explain, you can often work your way out of them by yourself by doing some simple debugging:

Just add some temporary debug code to the function and check to make certain that the values are what you expect them to be:

    sealingproc()
world << "DEBUG: sealingproc(usr = [usr] ([usr.type]), src = [src] ([src.type])"
world << "DEBUG: sealed = [sealed]"
if(!sealed) return
world << "DEBUG: con = [con]"
if(con>80)
sleep(200)
new/obj/immortality(loc)
sleep(10)
new/obj/immortalityfire(loc)
sleep(1)
alpha = 0
sleep(1)
new/obj/immortality(usr.loc)
sleep(10)
new/obj/immortalityfire(usr.loc)
sleep(1)
alpha = initial(alpha)
sealed = 0
loc = usr //you might change this.


Report back with the debug messages. They'll tell you what's wrong.
Page: 1 2