Help with Optimization

ID:2346114
 
Feb 15 2018, 11:53 pm (Edited on Feb 16 2018, 12:00 am)
Someone who is good at the profiler please help me optimize this. my cpu is dying

Code:
//-- Preloading ----------------------------------------------------------------

turf
    var
        dmm_suite/preloader/dmm_preloader

atom/New(newLoc)
    if(istype(newLoc, /turf))
        var /turf/turfLoc = newLoc
        if(turfLoc.dmm_preloader)
            turfLoc.dmm_preloader.load(src)
    . = ..()

dmm_suite
    preloader
        parent_type = /datum
        var
            list/attributeKeys
            list/attributeValues
            turf/location
        New(turf/loadLocation, list/_attributeKeys, list/_attributeValues)
            loadLocation.dmm_preloader = src
            attributeKeys = _attributeKeys
            attributeValues = _attributeValues
        proc
            load(atom/what) // All CPU spends all its time here.
                if(!what) del src
                var keyLen = attributeKeys.len
                for(var/I = 1 to keyLen)
                    what.vars[attributeKeys[I]] = attributeValues[I]
                del src


Problem description:
This is taken from a larger map loading system. Specifically, an update of my dmm_suite library. That load function is taking 7/8th of all time spent loading maps, and I've never looked much into optimizing DM so I don't know why. I'm guessing it has something to do with indexing variables by string, but I don't know how to get around that considering my use case.

The idea is that I need to set values on any loaded atoms before any custom code in New() is executed. The variables to be loaded could be absolutely anything the developer defines on an atom.

Originally I had one associative list for attribute keys => values, but the version above performed slightly better. Any ideas?
Feb 16 2018, 1:04 am (Edited on Feb 16 2018, 1:10 am)
The brunt of your CPU usage is gonna be the explicit deletion. That can be fixed by bailing out of worst case by cleaning up your improperly maintained circular references.

atom/New(newLoc) //this has no business being on /atom/New(). We're gonna get rid of it entirely.
    if(istype(newLoc, /turf)) //slow type check as opposed to fast isturf() check. We should never need to check the type of something unless we've improperly placed behavior under the wrong type... Which we have.
        var /turf/turfLoc = newLoc //pointless cast.
        if(turfLoc.dmm_preloader)
            turfLoc.dmm_preloader.load(src)
    . = ..() //retval is meaningless in New().


Ditch this.

turf/New()
    if(dmm_preloader)
        dmm_preloader.load(src)
    ..()



dmm_suite
    preloader
        var
            list/attributes //use an associative list. They are fast enough. Two lists is just silly.
            turf/location

        New(turf/location, list/attributes)
            location.dmm_preloader = src
            src.location = location
            src.attributes = attributes
            ..() //never depend on a lack of expansion.

        Del() //we need to clean up the circular reference to prevent worst-case deletion behavior
            location.dmm_preloader = null
            location = null
            attributes = null
            ..()

        proc
            load(atom/target)
                //target should never be null. ditch the if statement and src deletion.
                var/list/l = attributes, len = l.len, attr //local vars are faster, let's reference locally.
                for(var/pos in 1 to len)
                    attr = l[pos]
                    target.vars[attr] = l[attr]
                del src


That's as close to optimal as it's gonna get without blowing up the rest of your suite and rewriting the whole thing. I can't promise that preloaders aren't still dropping into worst-case deletion behavior. I don't know the rest of your suite. Read the developer reference on Garbage collection and the del proc. I rewrote part of that section and Lummox revised it as well. It'll open your eyes to what's happening.
Feb 16 2018, 11:06 am (Edited on Feb 16 2018, 1:24 pm)
Thanks for the tips! There's a couple "duh" things that you called out, like the explicit deletion, and I didn't know about the speed of local vs instance variables. Reworking those will definitely be better.

As for a lot of the structural changes, I don't think you understand how the preloader works or the kinds of situations it needs to handle. That's to be expected, I didn't post the entire thing or go into much detail. I'll explain now, and if I've missed anything, you can point it out:

Here's the basic idea: When DS creates atoms from DMM files at the start of a world, those atoms already have their variables set to new values before their New() procs are called. In order to simulate this behavior, I need to be able to change variables on atoms before New() has returned. This is the rational for the preloader.

As far as I know, there's no way for me to obtain a reference to an object before its New() is called, so I somehow have to override New() for every kind of thing that can be placed in a map file. This is the rational for redefining atom/New(). Because I don't know what else the developer will be defining on Atom/New(), I do ..() so as to not disrupt their code.

Now that we have a preloader object and a time to reference it, we need a way to reference it. The dmm_suite doesn't use a singleton pattern, so I can't refer to it directly. I know that all atoms in need of preloading will have a turf passed as the first argument to New(), so if I place the preloader on the turf, I'll be able to access it. The great part is that even new turfs can access the old turf this way, so a turf is never accessing its preloader variable, but its predecessor's! If you can think of a better way to gain a reference to the preloader without passing it as an argument to New() (this potentially breaks code defined by the developer using the library) I'm all ears.

I was confused when you mentioned a circular reference, then I saw /dmm_suite/preloader/var/turf/location. That's part of a test I wrote last night while trying to speed things up, and it didn't get deleted with the rest of the test. It never gets set, and there is no circular reference.

--------------------

Using your tips, here's what I've come up with:
//-- Preloading ----------------------------------------------------------------

turf
    var
        dmm_suite/preloader/dmm_preloader

atom/New(turf/newLoc)
    if(isturf(newLoc))
        var /dmm_suite/preloader/preloader = newLoc.dmm_preloader
        if(preloader)
            newLoc.dmm_preloader = null
            // The above is to prevent any atoms generated at this loc by this
            // atom in its New() proc from getting preloaded, too.
            preloader.load(src)
    . = ..()

dmm_suite
    preloader
        parent_type = /datum
        var
            list/attributes
        New(turf/loadLocation, list/_attributes)
            loadLocation.dmm_preloader = src
            attributes = _attributes
            . = ..()
        proc
            load(atom/newAtom)
                // if(!newAtom) del src
                // Note to self: The above is Necessary for the special case of areas.
                // Will remove to that section of the code when I'm back at my dev PC.
                var /list/attributesMirror = attributes
                for(var/attributeName in attributesMirror)
                    newAtom.vars[attributeName] = attributesMirror[attributeName]


I'll let you know how much time that shaved off when I get back to my dev PC.

(Edit: Removed a lot of errors from the load proc.)
Feb 16 2018, 1:30 pm
Yup. Removing the del statement reduced time by a factor of 8.5
Feb 17 2018, 9:08 am
http://www.byond.com/forum/?post=2345869

Check that out, while loading will take a tad longer the overall result is a much less laggy and streamlined because it will be spread over multiple ticks.
Feb 17 2018, 9:15 am
In response to Laser50
Laser50 wrote:
Check that out, while loading will take a tad longer the overall result is a much less laggy and streamlined because it will be spread over multiple ticks.

Thanks!