Everything about development and the OpenMW source code.
User avatar
Site Admin
Posts: 1193
Joined: 05 Aug 2011, 22:21
Location: Wroclaw, Poland


Post by lgromanowski » 10 Aug 2011, 22:47

Zini wrote: In game/esm_store/reclists.hpp you have the following lines:

Code: Select all

  typedef std::map<std::string,X*> MapType;

   MapType list;
(I saw this after reading your newest commit).

Using a pointer here is a big mistake IMHO. As you already noticed you have to clean up afterwards and more importantly this looks like a performance killer. new is relative expensive; certainly more expensive than the copying involved by putting X directly into the map (which can be avoided anyway in this case). So the typedef should be

Code: Select all

 typedef std::map<std::string,X> MapType;
Zini wrote: Unfortunately this isn't really an option for CellList, because you can't determine where to put the cell before loading it. Still, for RecListT and RecIDListT I was able to change the code. Even if it doesn't improve the speed much, having less new is generally considered good.

I also found a bug in cell_store. When searching for a cell reference in a reclist, you were always making a copy of the whole reclist. That fix must give some speed improvement for sure.

Will send a pull request later.
nicolay wrote: Thanks, I've pulled it.

The reason I changed it to pointers in the first place was actually because of a strange crash bug I got, where references pointed to by LiveCellRef::base got invalidated.

But after looking at your patch I realized that this was of course caused by the same copying of the reclist that you also fixed. The pointer was referencing temporary copies. So all in all, much better now!
Zini wrote: Exactly! Had this crash too. That is how I found the copy-problem.
best regards,