Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Codechange: C++-ify lists for SaveLoad #9323

Merged
merged 1 commit into from May 31, 2021

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented May 31, 2021

Motivation / Problem

I kept getting annoyed about two things:

  1. to constantly see C-style list iterators
  2. to not know whether Saveload * was indicating a single item or a list of items.

So why not fix both in 1 go?

Description

Basically, this changes "SaveLoad *" to either:
1) "SaveLoadTable" if a list of SaveLoads was meant
2) "SaveLoad &" if a single entry was meant

As added bonus, this removes SL_END / SLE_END / SLEG_END

As we have parts that need dynamic SaveLoad lists, SaveLoad [] is not always possible to use. In those cases std::vector<SaveLoad> is used, but converting that to a SaveLoad [] is not possible ofc.

What I turned up doing, is copying parts of gsl-lite into core/span_type.hpp to give us a std::span-lite variant (std::span is C++20). This allows both types to be used by SaveLoad functions, without them knowing the details of the container.

For SaveLoad [] the span<> stores the first and last element, and after that you can safely move the span around. For std::vector it does the same. So we have one type which captures both in a safe manner.

Mind you, span<> doesn't own the memory, so no using stack variables now :)

Limitations

  • We can argue if copying parts of gsl-lite is a good idea, or that we should just drop it in 3rdparty and be done with it. We could also upgrade to C++20.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@JGRennison
Copy link
Contributor

Converting std::initializer_list to const std::vector& on every call to the various SlObject methods doesn't really seem better than converting constant data to std::vector once at static init.
At the moment it looks like a new std::vector is being created, with memory allocated, memcpy called and memory deallocated on every loop iteration (i.e. each individually saved/loaded item) in most of the chunks. This seems likely to be bad for performance.
It would seem better to take a std::span instead, but unfortunately this is C++20. Some other equivalent wrapper type which holds begin and end pointers/iterators, or begin and length could be used instead.
In general const std::vector& tends to not be a good function signature.

@TrueBrain
Copy link
Member Author

TrueBrain commented May 31, 2021

At the moment it looks like a new std::vector is being created, with memory allocated, memcpy called and memory deallocated on every loop iteration (i.e. each individually saved/loaded item) in most of the chunks. This seems likely to be bad for performance.

I was really hoping compilers would do the right thing, but clearly it does not. Which is a bit sad :P I now solved it in a slightly different way by just templating both variants. Godbolt at least tells me that it is doing the right thing now.

In general const std::vector& tends to not be a good function signature.

Sadly, as C-programmer, these kind of statements don't really help me. I do need some container that is dynamic, and people tell me to use std::vector for it. So I see no other way then to use const std::vector & as function signature in those cases. If you have other suggestions, I am more than happy to hear, as I would love to learn this a bit more. But I need a bit more guidance here :)

@TrueBrain TrueBrain force-pushed the settings-rework-2 branch 2 times, most recently from 20b7295 to d518354 Compare May 31, 2021 12:15
@JGRennison
Copy link
Contributor

Sadly, as C-programmer, these kind of statements don't really help me. I do need some container that is dynamic, and people tell me to use std::vector for it. So I see no other way then to use const std::vector & as function signature in those cases. If you have other suggestions, I am more than happy to hear, as I would love to learn this a bit more. But I need a bit more guidance here :)

The contents of a container is not the same things as the container itself.
The real purpose of std::vector is to automatically manage memory. Each std::vector with non-zero capacity has it's own exclusively owned memory allocation. std::vector cannot be used as an aliased view over some other existing slice of memory, you need something like std::span, std::string_view or just a pointer/iterator for that.
In this case SlObject and other such functions do not need to do any std::vector memory management operations and so do not need a std::vector. A const std::vector& argument implies that the function needs a pointer to a std::vector container with all the memory management operations that that implies, in particular it's own exclusively owned memory allocation. However the function promises not to do any actual std::vector memory management operations on it because it is const.

Both std::initializer_list and std::vector are abstractions which return a contiguous array, so you can just pass the contiguous array directly without needing any allocations, copies or templates.

Underneath all the C++ syntactic sugar, what you really want is either:
SlObject(void *object, const SaveLoad *begin, const SaveLoad *end)
or
SlObject(void *object, const SaveLoad *begin, size_t length)

For the sake of convenience/prettiness, you can combine the begin and end, or begin and length, into a single type.
Which of these you use and what you call the type is entirely arbitrary and doesn't actually matter.
std::span is an implementation of the latter.
There are various other similar implementations such as gsl::span, boost::range.
It is trivial enough that rolling your own version and putting it in src/core/ would make sense. I could perhaps look at that later in the week.

@TrueBrain
Copy link
Member Author

TrueBrain commented May 31, 2021

You are awesome, tnx for this; I seriously appreciate it :) Now I get what you mean ;) And yes, it makes sense. I just want something over which I can iterate :P And I really do not care what that "something" is :D

@TrueBrain
Copy link
Member Author

Right, I spend some time reading up on std::span and how to put your story in perspective. I ended up copying parts of gsl-lite into OpenTTD to give us a std::span-lite. I tried to minimize the code as much as possible, to make it as readable as possible. I am sure I forgot things or did silly stuff .. but for that we have reviews :)

It is a lite variant, as I did not implement all the functions. Mostly as I don't think we need them, but it is easy enough to add them all if we really want to.

Alternatively, we could add gsl-lite to our 3rdparty folder, or .. switch to C++20. Sky it the limit :D

Tnx again for the explanation @JGRennison , it helped a lot in figuring out what this all is doing .. and I learnt a lot today :D

@TrueBrain TrueBrain force-pushed the settings-rework-2 branch 4 times, most recently from e1acdda to 3bfcaa0 Compare May 31, 2021 18:47
@TrueBrain
Copy link
Member Author

After some talk on IRC, decided to remove std::initializer_list, as it is kinda odd to have to use that. Turns out C-style arrays works fine, as long as we also have span<> to track the length as soon as we need it. So I changed this PR into that :)

@TrueBrain TrueBrain force-pushed the settings-rework-2 branch 4 times, most recently from 4e52926 to eb3baf4 Compare May 31, 2021 20:00
Basically, this changes "SaveLoad *" to either:
1) "SaveLoadTable" if a list of SaveLoads was meant
2) "SaveLoad &" if a single entry was meant

As added bonus, this removes SL_END / SLE_END / SLEG_END. This
also adds core/span.hpp, a "std::span"-lite.
@TrueBrain TrueBrain merged commit 9fff00b into OpenTTD:master May 31, 2021
@TrueBrain TrueBrain deleted the settings-rework-2 branch May 31, 2021 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants