Nexela / PickerExtended

MIT License
12 stars 16 forks source link

Migration script explodes memory consumption. #39

Closed NoxyNixie closed 7 years ago

NoxyNixie commented 7 years ago

Heya Nexela,

The following migration may be slightly over the top https://github.com/Nexela/PickerExtended/blob/master/migrations/migration_0.6.2.lua.

When trying to migrate your mod from 2.6.1 to 2.7.1 on a 41MB save the memory consumption exceeds 8GB (at which point my only solution is to kill factorio). Without updating the mod factorio does not exceed 3.5GB and successfully loads.

Is it really necessary to iterate through all entities without any form of filter and then call update_connections()? What is this migration trying to solve? Can it be limited to a much much smaller subset of entities?

For now I'll have to stick with version 2.6.1.

Nexela commented 7 years ago

I will devise a better way to do this. in the meantime you can delete the migration file from the zip without issues as long as it is for SP only

Nexela commented 7 years ago

I have made some tweaks to the migration script for the next version. on a 40+MB map memory went to to about 5gigs before settling down.

NoxyNixie commented 7 years ago

Thanks for devising a better performing migration.

I still am curious why this is a necessity and what is it trying to "fix"?

Considering the API documentation of this method I suspect entities in the forces "neutral" and "enemy" should probably be skipped as this method probably does nothing to most entities in these forces. However I don't know what the migration is trying to fix in the first place so my deductions may be wrong.

Maybe something along the lines of this might work better?

for _, force in pairs(game.forces) do
    if #force.players > 0 then
        for _, surface in pairs(game.surfaces) do
            for _, ent in pairs(surface.find_entities_filtered{force = force}) do
                ent.update_connections()
            end
        end
    end
end

This should skip all trees, spawners and biters, unless they are player owned (which if I'm not mistaken is not possible for trees anyway).

Come to think of it if this method is only really useful for loaders and beacons wouldn't filtering for just those make more sense?

for _, force in pairs(game.forces) do
    if #force.players > 0 then
        for _, surface in pairs(game.surfaces) do
            for _, ent in pairs(surface.find_entities_filtered{type = "beacon", force = force}) do
                ent.update_connections()
            end
            for _, ent in pairs(surface.find_entities_filtered{type = "loader", force = force}) do
                ent.update_connections()
            end
        end
    end
end
dgw commented 7 years ago

on a 40+MB map memory went to to about 5gigs before settling down.

This is kind of excessive for a single migration, isn't it? I suspect this is why I can't add PickerExtended to my existing (25MB) save: the system only has 4GB of physical memory. Is deleting the migration script from the archive a viable workaround if I've never used the mod before with a specific save file? (Update: Disabling PickerExtended allows my save to load properly.)

Meanwhile, I'll sync all my saves & mods to another computer with more memory (8GB instead of 4) and see if I can't crash it too.

dgw commented 7 years ago

Loading the mod for the first time on my other system successfully ran the migration without running out of RAM. That's quite the overhead, but at least I can use Picker now on either system.

Nexela commented 7 years ago

Sorry I missed your earlier reply, You can safely delete the migration file if it is still an issue for you on existing maps. I will see if I can reduce it even further in the next update.