Author Topic: Simutrans 64bit build  (Read 670 times)

0 Members and 2 Guests are viewing this topic.

Offline Antarctica

Simutrans 64bit build
« on: June 10, 2017, 03:26:33 AM »
Are there plans to release a 64bit Windows build of SimuTrans?

I just ran into 4GB memory maximum for the first time (chose a big map and had a high number of vehicles), and got an "Out of memory" error.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 4635
  • Total likes: 168
  • Helpful: 108
  • Languages: EN, NO
Re: Simutrans 64bit build
« Reply #1 on: June 10, 2017, 09:08:10 AM »
When Simutrans needs that much memory, it has kind of been assumed that performance will have become major problem long before then. And 64-bit build can possibly perform worse than 32-bit builds. I haven't noticed that, but that was on what is now a low screen resolution and a 512x512 map.

Offline Antarctica

Re: Simutrans 64bit build
« Reply #2 on: June 10, 2017, 02:35:02 PM »
Well, in my case, the performance wasn't an issue at all; the game was fully playable with decent framerates until the end.

And I didn't even create the largest map allowed by the game. 10k by 10k is already estimated at 6 Gigabytes of memory, without a single vehicle. I only had a map of 64 by 20k with about 40k ships.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 4635
  • Total likes: 168
  • Helpful: 108
  • Languages: EN, NO
Re: Simutrans 64bit build
« Reply #3 on: June 10, 2017, 04:32:56 PM »
64 times 20000 is not a particularly large map. I play on a 2048x2048 map. Or did you mean a 64000 times 20000 map? If so, I'm rather perplexed how anyone can manage such a large map. Then again, 40000 ships, or vehicles at all, is also something that sounds inhuman to me.

It seems to be more likely that it is the contents of the map: the vehicles, the stuff the vehicles move, and the things that generate the stuff the vehicles move that drains the memory. However I have to defer to more experienced developers for a more thorough analysis.

Offline Antarctica

Re: Simutrans 64bit build
« Reply #4 on: June 10, 2017, 07:02:12 PM »
64 by 20k; a really really long really really narrow stretch, and then I let ships run fuel and cargo from one end to the other, so there are quite some ships required.

I think I have found the memory leak that caused this issue.

To reproduce:

- Open two instances of Simutrans 122.2.2 - r8163 with the same pakset (in my case, 128-german, but any other should suffice).
- In each, click "Load" and "Cancel" to get to the start map, and pause the game.
- Build a depot each, buy a single vehicle each, assign it to a new line with two waypoints. Bus, ship, train, monorail, plane, it really doesn't matter, just use the same in both.
- Now for the difference: In one simutrans instance, open the line window, select the line and keep it open. In the other one, open the line window, select the line and close the line window again.
- Open task manager, check that the Memory footprint of each instance is about the same (in my case, 246 vs 256 megabytes)
- Now, in both of them, copy the vehicle about 4999 times (once using mouse, and 4998 times more by putting a letter opener on your Space key).
- Open the line window in the instance where it was closed.
- Check the task manager again, and find there's a whopping difference in memory usage.
- Close the line window in both instances
- Check the task manager again - but expect no visible change in memory footprint of either instance.

For me, Simutrans with line window open during the convoi copy operation went up from 256 to 570 MB, without line window open from 246 to 252 MB.

Offline DrSuperGood

Re: Simutrans 64bit build
« Reply #5 on: June 11, 2017, 06:25:45 AM »
Memory leak possibly at line 620 in schedule_list.cc.

Code: [Select]
cont.remove_all();
scr_coord_val ypos = 0;
for(  uint32 i=0;  i<icnv;  i++  ) {
gui_convoiinfo_t* const cinfo = new gui_convoiinfo_t(new_line->get_convoy(i));
cinfo->set_pos(scr_coord(0, ypos));
cinfo->set_size(scr_size(400, 40));
cont.add_component(cinfo);
ypos += 40;
}
cont.set_size(scr_size(500, ypos));
Many new gui_convoiinfo_t are created when populating/refreshing the list. However they are never deleted. Instead the list it is attached to is correctly deleted, losing reference to the gui_convoiinfo_t object, hence a memory leak. Also applies to the stop list objects, halt_list_stats_t.

The leak mentioned above can probably be reproduced by opening and closing the line list dialog a few thousand times. Reason it is so large in the given example is the leak complexity of adding convoys with the line list open is O(n^2), and after 5,000 convoys that is a lot of leaked objects.

Solution is to iterate through the list and delete the objects before clearing the list. I will look into testing this later.
« Last Edit: June 11, 2017, 07:37:10 AM by DrSuperGood »

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 4635
  • Total likes: 168
  • Helpful: 108
  • Languages: EN, NO
Re: Simutrans 64bit build
« Reply #6 on: June 11, 2017, 09:17:19 AM »
Good catch!

It seems that gui_container_t is not meant to actually own its elements. The random selection of other uses I've looked at only adds references to instances held elsewhere.

It also looks like the vehicle list for lines is done quite differently from the complete vehicle list. I don't see the latter uses gui_convoiinfo_t in a gui_container_t.

Offline DrSuperGood

Re: Simutrans 64bit build
« Reply #7 on: June 11, 2017, 10:11:03 PM »
Should be fixed with r8247. Please check (might have to wait until it or later is built by nightly servers).

The best solution would be to use unique_ptr to manage the objects as that auto deletes them when going out of scope. However that is a C++11 feature.

The current solution is to add them to a vector upon creation. This vector can be iterated through and the objects deleted during refresh or destruction. Not the most efficient but sufficient.
« Last Edit: June 11, 2017, 10:31:44 PM by DrSuperGood »

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 8725
  • Total likes: 304
  • Helpful: 229
  • Languages: De,EN,JP
Re: Simutrans 64bit build
« Reply #8 on: June 12, 2017, 12:26:01 AM »
Apart from that, Simutrans will run out of convoi handles when the 65354 forth convoi is started. (That is independent from the 32/64 bit issue, sinc ethe convoi ID must be 16 bit to maintain network conatibility between the two builds.)

Of course it is possible to extend this. But so far the need has never arisen.

On the other hand, this is really a nice demonstration of the power of the Dwachs regional ship pathfinder, because before a map with 250 ships would be unplayable. (For profiling, I would be interesting to try your map.)

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 4635
  • Total likes: 168
  • Helpful: 108
  • Languages: EN, NO
Re: Simutrans 64bit build
« Reply #9 on: June 12, 2017, 05:42:37 AM »
The best solution would be to use unique_ptr to manage the objects as that auto deletes them when going out of scope. However that is a C++11 feature.

In this particular case, one may be able to avoid the problems with auto_ptr, though. But doing it by hand, as has been done, isn't too complicated in this case either. In that case, having an extra lists is probably not technically necessary.

Offline DrSuperGood

Re: Simutrans 64bit build
« Reply #10 on: June 12, 2017, 04:15:05 PM »
Quote
In this particular case, one may be able to avoid the problems with auto_ptr, though.
Except...
Quote
auto_ptr was fully removed in C++17
Hence it is not a future proof or recommended solution. The current approach is more future proof without coupling to C++11 or later. When raising to C++11 then one can replace with unique_ptr.

Optimization is not really a concern in this case as the memory leak was far worse than a slightly suboptimal implementation of fixing it.
« Last Edit: June 12, 2017, 10:39:52 PM by DrSuperGood »

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 4635
  • Total likes: 168
  • Helpful: 108
  • Languages: EN, NO
Re: Simutrans 64bit build
« Reply #11 on: June 12, 2017, 07:45:05 PM »
I wasn't really thinking about optimization, in terms of code performance at least. I'm not sure what to call it, but it would perhaps have been less code to write. Since it's done, it's probably more of a "waste" to change it again, anyway. Not to mention that it might be less clear what is going on for other developers at a later time, than the current solution.

(And I'm no more worried about using auto_ptr when programming in C++98 than I am with using ballpoint pens on paper, despite auto_ptr working as bad in C++17 as ballpoint pens do on iPads.)

Offline DrSuperGood

Re: Simutrans 64bit build
« Reply #12 on: June 29, 2017, 05:07:24 AM »
This thread should probably be moved to the bug report section.

Antarctica can you please confirm that the reported memory leak has been fixed in the nightly builds so this bug report can be archived?