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

contents.Copy()


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.
Yeah this was the last thing I tried, I know it's not necessary. I tried locate at first, then iterating over it normally then using locate specifically on x.contents, then using contents.Copy.
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.
I can make a quick test map.
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:
var/list/attack_verb = list() //Used in attackby() to say how something was attacked "[x] has been [z.attack_verb] by [y] with [z]"

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:
var/armor = list(melee = 0, bullet = 0, laser = 0,energy = 0, bomb = 0, bio = 0, rad = 0)

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.
Lummox JR resolved issue (Not a bug)
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.
How would you suggest handling something like this by the way?

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?

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
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?
The latter is far better, but when you already have New(), you should stick stuff there that would ordinarily be done in a hidden init.
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.
Ya, this just hasn't been something noticeable for me, so i haven't looked into optimizing, but its been in the back of my mind. I have overclocked ddr3 memory on my pc, and the server has ddr4 ram, and ram access speed and ram command delay are generally the primary bottleneck for start up time.

I'm right now refactoring our mc and some of that will involve a bunch of New() work (for things that add themselves to one of the tickers) So i can take a look at this, but generally we've always favored slightly higher memory usage (and slightly higher startup times) if it made code faster later on (part of memory usage has to do with turfs and other objects storing their deconstructed form to make explosions and other mass destruction faster)

I do have to ask, what are your system specs and what kind of start up times are you seeing.
In response to Optimumtact
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.

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.


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:

// bad idea
obj
var/mylist = list(1,2,3)

New()
...

This is a better one:

// better
obj
var/mylist

New()
mylist = list(1,2,3)
...

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.
In response to MrStonedOne
That was on my wife's computer, so I don't recall the specs offhand. But the startup was glacial, like several minutes. That's really not acceptable when debugging.

The pattern I noticed overall was that the code wasn't making the kinds of tradeoffs you suggest, where it uses more memory up front for speed later. Rather, it was making bad choices without justification, like the empty attack_verb list in /obj and the armor list in /obj/item. (If you have to loop through all armor types for the item at any point, I see some justification for the armor list, but you'd still be better off having a global var that serves as the default that you can fall back on.)

The lighting code is a tough call, because I didn't take the time to study it in depth. But I suspect you'll see almost no runtime speed loss at all for getting rid of turf/New() or atom/movable/New() in favor of a dirty-info marker, and losing atom/movable/New() from there might help with the other obj creations that happen more frequently.

The map reader code was the weirdest for me. I don't know what that atom/New() is doing, but I'm sure there has to be a much, much better way to do it. So that's yet another New() proc that's impacting everything in the world, and again you'll see gains not just at startup but at runtime from killing it with fire.
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
   if(attack_verbs==null)
attack_verbs = get_unique_attack_verbs()

attack_msg = "blah blah [pick(attack_verbs)]"


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
The map reader one handles object var initializations from the map, Mainly as an attempt to get the map edited vars loaded before the atom's new() or at least when the atom's new() calls ..()

if we could use newlist() with a string for the vars, that would kill that off, other wise i don't see any way to properly do that since some atoms change how they New() based off of their vars at New()

as for lighting, I think that was for modularity, rather than add snowflake code in each object's code, lighting can bind to New() as a way of getting notified when something is created.

I'm not sure if that is necessary, but I can see the appeal of abusing sister overriding procs as a OOP way of doing binds and events.

Anywho, I'll dig into these New()s and see what can't be cut down (lighting for example, already does its init stuff in a batch when the MC finishes setting up, so i can just have that check to see if the map is loading, and just early return, but i don't think i can remove it flat out without the maintainers objecting over the addition of snowflake to some other part of atoms, unless it massively reduces start up time by an order of magnitude)

Oranges, could you look at git bisect'ing that and tell me what commit fucked up load times, we can look at fixing that.
In response to Optimumtact
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.

#define ATTACK_VERB(item) (istype((item.attack_verb),/list) && length(item.attack_verb) ?\
pick(item.attack_verb) :\
((item.attack_verb) || "attack"))

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.

#define ATTACK_VERB(item) (pick(splittext(item.attack_verb||"attack", ",")))

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

#define ATTACK_VERB(item) (istype((item.attack_verb),/list) && length(item.attack_verb) ?\
pick(item.attack_verb) :\
pick(splittext(item.attack_verb||"attack", ",")))

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.
Thanks for that advice.

Definitely stuff we can implement here.
Page: 1 2