`trade.py trade` definitely does more work than it should

[ultimatespirit]

Calling trade.py source destination appears to attempt to load the entire database and then perhaps do some additional processing considering the amount of time it takes.

This is obviously wasteful, it has been given to exact starting and end zones, all it really needs to do is pull out the data for both of those locations and then compare them. And if pulling out the data requires loading the entire database rather than searching the database with much lower memory requirements... then that sounds like a pretty major usability issue considering how large the database is now.

I've noticed this for all the commands really, they seem to spend tons of time loading the entire database and then doing the culling operations, if even, requested of the operation, making all queries slow. The load could be avoided by just making the application a REPL, something I believe that has been mentioned in previous issues, or some sort of (local) sever-client architecture, though making cases that obviously don't need a full load not do the full load would be a good step too.

[ultimatespirit]

After consuming around 12GB of memory (my system only has 16GB, so I'm unsure if that 12 is an upper bound, or if it just has a "reasonable" percent of using 66-67% total system memory) and 2-3 minutes of wall time, the following command was finally able to output something (granted said something is still not the most helpful but that's a separate issue I've brought up before [1]):

/usr/bin/time -f '\nreal\t%Es\nuser\t%Us\nsys\t%Ss\nMax mem\t%M kb' ./trade.py trade "lushertha/de caminha station" "Anlave/Brunton Gateway" -vv
<Trade output elided>

real    2:27.16s
user    122.43s
sys 9.72s
Max mem 11641064 kb

Using GNU time.

For comparison, the actual commodities buy command that even got me those points to check their trades of:

/usr/bin/time -f '\nreal\t%Es\nuser\t%Us\nsys\t%Ss\nMax mem\t%M kb' ./trade.py buy --near lushertha --ly 23 --age 0.5 -S "basic medicine" "non-lethal weapons" "reactive armour" --limit 10
<Actual trade output elided>

real    0:28.75s
user    20.67s
sys 2.18s
Max mem 155708 kb

A command that at minimum had to be executing queries on distance and multiple queries on commodity prices used 155.7MB of memory and 30 seconds of time, versus a command to output two precise location's data diffed against each other and then sorted taking 150 seconds and 11.6 GB of memory. And yes, if it weren't obvious, I was interested in how trade.py would fare for the community goal haha.

Anyway, something has clearly broken here, and I'm trying to resist the urge to dive into the code at the moment to root cause it versus actually playing the game and enjoying that CG.

(P.S. [1] refers to my previously opened issue #143, with a subitem asking for trade.py trade to please output results for both directions of trade, or to include an option to output that. If this issue causes anyone to audit what trade.py trade is doing and rewrite portions of it, pretty please add that logic for bi-directional trade too haha)

(P.P.S. It would also be pretty nice if the buy command had an option to output the profit if the found commodity is sold at a specified station (and defaulting that station to the system / station specified by --near, if a system choosing the best station in the system per commodity I guess)

[ultimatespirit]

By the way, the reason trade is doing so much is because tradeCalc(...)'s init will load the entire database as no environment will be present to clamp down on age, or any other parameters. This occurs before even sanity checks occur so something like trade src src should result in a huge load time followed by the error message for having the same name twice.

Refactoring tradeCalc is non-trivial, since I assume it's used as part of run, though it likely should be done considering how the SQL processing could be improved to reduce work in Python and __init__() doing so much there makes it harder to reuse parts of its logic versus having an explicit loadItems() function or something.

I saw that the buy command uses direct SQL statements, doing something similar for trade will likely be the easiest solution prior to any work to make tradeCalc more easily used for a general case like this. If I get time this weekend I'll try to do that.

P.S. This line in the buy command should use the next() function to avoid constructing an entire list just to throw it away for its first element. I believe this is the time to once again point at that @kfsone guy's past decisions ;)

[kfsone]

Don't underestimate my potential for derp -- I totally started TD as a learn-me-some-python project; to emphasize that:

P.S. This line in the buy command should use the next() function to avoid constructing an entire list just to throw it away for its first element.

queries isn't that big, and likely when I first wrote that I was flapping my arms at the Python 2.7/Python 3 disparity in behavior of dicts and iterators. TIL: While Python 2.7 had ivalues() Python 3 folded it into values(), but - subtly - stopped producing an iterator:

https://python.godbolt.org/z/nn7KrEqv8

and that may be as far as I looked?

I also wouldn't known of/thought of next(iter(queries.values())) to test if it was more efficient at that point.

https://python.godbolt.org/z/a9WxcrEPb

This was also probably before I discovered I wasn't going to be able to scale performance with threading :)

There's another issue/discussion elsewhere where I have suggested what was always my original plan, for TD to be-its-own-service so that the data is loaded once. SQLite was just the most accessible ACID-storage option I had at the time, I didn't actually intend to use it as a database, and that's really what you're seeing in this code, is my trying to avoid the lure of writing SQL queries believing that to do so would make it harder to funnel things through TradeDB rather than pure SQLite.

I seem to remember that scoped-loading either existed or was planned. I don't know whether I simply screwed it up with order of operations in this one or the sql-creep bent this out of shape.

I've also commented elsewhere that a HUGE problem with the station-items table is that it's a single table instead of separate buying/selling tables, which results in a pair of "x_price > 0" indexes which turn out to have logarithmic complexity maintaining - so when the db was much smaller it was an optimization, now it's a giant hindering turdbuger :(

I was working on changes to move those into their own tables, alongside changes to also change the cache build so that it doesn't rebuild the file from scratch every time, allowing the db to benefit from reuse.

https://github.com/kfsone/Trade-Dangerous/tree/kfsone/inplace-cache-rebuild https://github.com/kfsone/Trade-Dangerous/tree/kfsone/import-via-table

[Tromador]

SQLite was just the most accessible ACID-storage option I had at the time.

When I was very much younger, I studied chemistry at university and also took a lot of ilicit mind altering drugs. Either way "acid storage" means something unrelated to computers, could you educate me?

when the db was much smaller it was an optimization, now it's a giant hindering turdbuger :(

The utterly ridiculous database size is a large (if not the exclusive) reason we've started routinely purging data more than 30 days old. To put some numbers to this, before the purge, the DB was ~5.5GB in size, it's now ~500MB. So the vast majority of the data available (for example from spansh) is wildly out of date, sometimes literal years old. This should also hugely reduce the daily listings.csv download size, which has been mentioned elsewhere.

[kfsone]

Atomic, Consistent, Isolated, Durable; if you're in the middle of writing something to the disk, it can't end up in an unrecoverable state.

Say the first 1KB of your file is an index, four bytes of an ID and four bytes of an offset into the file the data for that ID as at.

e.g.

(3612, 8000) - meaning offset for id 3612 is end-of-index + 8000 bytes.

You get an update that 3612 was deleted and 3613 added, but the new record is bigger so it has to be relocated, and it turns out there's a big enough free space at offset 2000.

writing this data into your file is going to take a non-trivial quantity of time - dozens or even hundreds of cpu instructions.

say the code does:

seek(index_position) write(new_id, new_offset)

There's no data at the new offset, you didn't move the old data into the free list, and the old data is still there but now orphaned? If those values are something to do with payments, you just threw money away. There are ways you can work around it to provide stability, a common one being that you write the steps you need to execute to a "journal", and you don't allow access to the data while the journal is non-empty. After that you still have to be careful not to put the data into an intractable state, but you can at least recover from crashes or power outages. https://en.wikipedia.org/wiki/ACID Unfortunately, as I was building up TD, I kept finding SQLite to _not be all that_; it was fairly routine for it to render the database unrecoverable. It was still giving me headaches at work up until about 4-5 years ago but - touch wood - haven't seen one in at least 4 years.

[ultimatespirit]

I also wouldn't known of/thought of next(iter(queries.values())) to test if it was more efficient at that point.

I didn't even notice that queries was a dict, based off of the whole single mode thing I guess it was supposed to just be a single item dict though? Since, even had you known you could do next(iter(...)), or for that matter next(v for v in queries.values()), dictionary order wasn't made consistent officially until python 3.7, which wouldn't have been out yet by the time that code was written. So it would be malformed for anything that isn't a single element dictionary, which I guess this is. In which case as odd as it looks I guess list(queries.values())[0] really is the only (easy) way to do it lol...

is my trying to avoid the lure of writing SQL queries believing that to do so would make it harder to funnel things through TradeDB rather than pure SQLite

I did notice that, and you've mentioned it somewhat elsewhere IIRC. Honestly, it may make sense at this point to just migrate to a pure NoSQL key-store instead of using SQL at that point. Though I'm honestly surprised that sqlite is so inefficient as to not be feasible to offload to for concurrent lookup at least, given Python's inability to do threading and poor multi-process data passing ability (pre 3.13 at least, if that ever materialises). For all the areas where additional pure-data checks are being done in Python rather than SQL that is (like "is the price less than this number?" or "Is the pad size.." etc.).

[Tromador]

I'm a sysadmin by trade, not a programmer. I'll hack crappy code together to achieve something simple when I've no other choice. So a lot of this goes over my head. My question therefore is how much of these discussions are realistically achieveable? I've done a lot of project management and this project has zero budget very limited resources, so I look at this and whilst it may be academically interesting discussion, does it lead to something that can be made to happen with reasonable expectations on time input for all concerned? I mean, by all means carry on the discussion, but if this is all pie in the sky, then also consider if there's some lower hanging fruit we can more easily deal with to improve the state of the software.