Okay, great. If it's consistent I should be able to get to the bottom of it.

protip:



This bit of code isn't actually needed, as for loops copy the list, but do it much faster then Copy() can do it because it's all internal.

The question I have for you, is if this was added in attempt to find/stop the bug or if you've always been copying the contents, as if its the latter, that might be a key to the bug.

Well, crap. I just opened up the file and it's tg after all. I have to be honest here: Having been burned by tg before, I'm not even willing to open this in the debugger unless I'm out of options. The codebase is too freaking monstrous and it uses way, way, way too much memory. It makes the debugger very angry.

Is there any way you can get this to happen if you pare down the maps or something? That might reduce the footprint enough to make it possible to work with, but I don't want to even try this as-is except as a last resort.

Argh. This is not a bug.

I decided to try debugging this on another computer. It took forever just to get myself properly recognized as an admin, because inexplicably my guest ckey appears to be different from what shows up in another project. Weird, but not your fault; it's just something that was an anoyance, exacerbated by the ridiculous startup time.

Before I explain the issue, please pass on to the main tg team to fix these immediately, if these instances come from the main tg branch:

Line 13 in objs.dm:

This line of code is causing ALL objs to execute an init proc. You should never ever do something like this in a generic type like atom, turf, obj, etc., especially in cases where startup time is an issue. Do not initialize this list until you need it, and anything that uses attack_verb should be prepared to deal with a null case.

Here's another one of those gems, in items.dm on line 33:

Do all items need this line? I really doubt it.

The _DynamicAreaLighting_TG.dm file is a straight-up mess as well. Why call a proc on every turf/New() when you can simply have a single proc handle them in a list? That'd be way faster, avoiding craploads of overhead. (It wouldn't handle dynamic cases, but you could handle those other ways, like adding updated turfs to a list.) atom/movable/New() is also yet another proc always being called when objs start up.

code\modules\maploader\reader.dm, line 222 defines atom/New(). Why? This is something you really need to avoid at all costs in a game with a high startup time.

Some of these things may require some retooling to get away from, but the benefits of doing so are huge. Others, especially the objs.dm and items.dm cases, should be super easy to fix.

You're doing a similar thing in /obj/structure/railroad_track, creating the next_track list in an init proc when New() should use it instead. You're calling two procs when you could be calling one. Although it's not at all clear why you have a next_track list, since you don't even need such a list for the train to operate properly.

---

Now, the reason this isn't a bug: The value in the list is actually the correct value. It's the variable editor that's lying to you.

Items added in the map editor are often given a tag for that instance, when you generate the instance by creating an instance for each dir or icon state. Tags are supposed to be unique, but the map editor really doesn't care, so everything you lay down with that instance has that same tag.

Enter the \ref macro. When an object has a tag, \ref spits out the tag instead of a reference like [0x20abcde]. The variable editor is simply spitting out the ref as given, and when you click on it, it's dutifully looking that up by locate(). Although the tag is supposed to be unique, it's not, so it simply finds the first matching item in its list. That happens to be a specific one of the railroad tracks.

If you go into the variable editor and edit the tags of all railroad_track items to be empty, the next_track list displays the correct tracks that were there all along. Removing all tag=... entries from the map (when editing it as a text file) will make this permanent.

So to recap: several hours wasted chasing a non-issue, and 510.1345 is delayed another couple of days. Tg's unreasonably high startup time is why we can't have nice things.

Hi Lummox thanks for the report. :star:

We'll get onto this now although note we're mostly optimising for our specific server, and so far the roundstart time has not got to a noticeable problem for us on the live servers.

and we're using that list of items in a obj definition pattern in a lot of places and touch them all over the place, so it's not as trivial to refactor as we would like.

I'm a little bit confused here

Is it better to do in New() to avoid the init() or do it at runtime when it's first required?

Also is there anyway to get statistics on which type path inits are taking the longest?

We had an overhaul recently that seems to increased our startup time by about 3x and I'm not even sure where to start looking.

Optimumtact wrote:

We have this list of attack verbs, and one is picked in teh generic attack proc of the item type and used to display some message

var/list/attack_verb = list() //Used in attackby() to say how something was attacked "[x] has been [z.attack_verb] by [y] with [z]"

How can we init this at runtime when the parent proc doesn't know what values the child type might want?

I'm not sure I follow. The generic /obj type certainly does not need this list, or you'd have put something in it. So you can leave it null until you fill it with something, and when you actually use it you can check for the null case.

The actual example code you gave shows a non-list usage anyway. In that string, z.attack_verb is obviously not going to be a list.

The only solution I see is to create another proc that returns a list, like get_attack_verbs() and then call that when the attack_verb list is null

You don't need a proc to do that; that would be counterproductive. You can simply put a null check into wherever you use the attack_verb list. In fact you could probably use a #define macro for something like that.

I'm actually not sure, based on your description, why attack_verb is defined as a list at all. But in any case it definitely does not need to be initialized for every obj, and having that initializer in /obj doesn't make any difference as far as initializing child types is concerned.



The trick is making sure that the list is only initialized when you need it to be. I know there are objs that have attached datums and built-in contents and whatnot.

A good way to deal with this pattern and refactor is to take it one step at a time. Now that regex searches are possible, you can find cases like var/.../myvar = list(...) and then you can look for every place that var is used. Mostly only the top-level types, like atom and obj, are where you want to worry about this. I saw a lot of calls for init procs for /obj and /obj/item, and both of those init procs were completely unnecessary.

Is it better to do in New() to avoid the init() or do it at runtime when it's first required?

To expand on my earlier answer, this is a bad idea:


This is a better one:


This avoids the need for an init proc and therefore saves you overhead. But best of all is not to create the list at all until you need it, and if you can avoid having to call New() or init procs for as many atoms as possible, that's the very best.

Right now you're taking multiple hits: You have New() all over the place, including some really questionable spots I pointed out. The lighting is acting on turf/New() but you're better off only making changes in one big loop at startup and on an as-needed basis for any turfs that change later. atom/movable has a New() proc in the lighting also, and it's the same deal. In fact if all those procs are doing is a calculation, then you're better off giving all atoms a var that says their lighting info hasn't been calculated yet (or is dirty), and when you go to actually use that info, calculate it if needed.

The big takeaway is that procs have call overhead; even the simplest proc has to be setup and torn down. Init procs and New() procs are therefore the enemy of startup time, and what you need to do for best practices is to avoid calling such procs for atom types that you have a lot of. When you have both an init proc and a New(), throw everything that would need an init proc (like a list) into New() if feasible because then you're calling only one proc instead of two.

Yeah I've been getting startup times of 2m40s for startup and that's not even talking getting all the tickers running.

This is on a 10 dollar vm however, haven't had a chance to time my home desktop which is much more powerful.

But we had a recent code refactor and I checked out a version from just before then and startup was 0.40s on the previously mentioned vm so we've definitely regressed a lot

@Lummox

Apologies, I wasn't quite clear with how the attack_verbs list is used (that comment is actually incorrect and I didn't mean to copy it)

when an object hit's something it uses pick(attack_verbs) and then uses the picked result in the final string.

My point was, I agree we can wait until we need to actually pick from the list to actually create it and leave it null the rest of the time, that makes a lot of sense in terms of offputting list creation time until we need it and avoiding unnecessary init calls.

However we want each sub type of item to be able to define it's own unique list of attack_verbs, and current sub types do it by simply overriding what the attack_verb list is defined as and set it to their own values.

The only way I can see to do to handle this at runtime instead of during startup is to have a proc get_unique_attack_verbs that it overrides.

So the final actual runtime use becomes


Now I don't mind this approach, it seems reasonably sensible. I'm just wondering am I missing some byond language feature that would allow another way to do this?

As to the new/init case that makes perfect sense now and we can easily look to move all list instantiations to new and then we can profile startup and see where our hotspots are and start refactoring

Thanks

Optimumtact wrote:

when an object hit's something it uses pick(attack_verbs) and then uses the picked result in the final string.

That makes more sense to me; I was thinking it would have to be a pick().

My point was, I agree we can wait until we need to actually pick from the list to actually create it and leave it null the rest of the time, that makes a lot of sense in terms of offputting list creation time until we need it and avoiding unnecessary init calls.

However we want each sub type of item to be able to define it's own unique list of attack_verbs, and current sub types do it by simply overriding what the attack_verb list is defined as and set it to their own values.

Right, but you can override without needing the regular one in /obj to be initialized.

The only way I can see to do to handle this at runtime instead of during startup is to have a proc get_unique_attack_verbs that it overrides.

So the final actual runtime use becomes

   if(attack_verbs==null)
      attack_verbs = get_unique_attack_verbs()
      attack_msg = "blah blah [pick(attack_verbs)]"

am I missing some byond language feature that would allow another way to do this?

I can think of several. First, if you want this to be a list you can still use lists for most of these, but leave the default /obj with null for the var.


That method lets you use null for just "attack", or a string if you have only one verb, or a list.

You can go even further and use strings entirely without making this a list, by just giving every object a string with separators.


Or, you can go with a hybrid method that lets you slowly transition to using strings:


Attack verbs may be called often, but they're not called with such frequency that the small hit from splittext() would be significant at all.