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

rework autosegs to use global constructors instead of scanning segments, removing all platform-dependent code #2887

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

RicardoLuis0
Copy link
Collaborator

No description provided.

@coelckers
Copy link
Member

Make it an option at least. Doing it with constructors is grossly inefficent on at least MSVC.
There are thousands of objects being initialized with these.

@RicardoLuis0
Copy link
Collaborator Author

RicardoLuis0 commented Jan 6, 2025

yes, but it only runs at program initialization, i think the very small increase in startup time is worth it for getting rid of extremely-platform-specific code (just parsing/compiling zscript will take way longer than these constructors)

@Boondorl
Copy link
Contributor

Boondorl commented Jan 7, 2025

Make it an option at least. Doing it with constructors is grossly inefficent on at least MSVC. There are thousands of objects being initialized with these.

Let's see the metrics on this. I don't buy that this would have any serious impact whatsoever on a machine that's remotely modern.

@coelckers
Copy link
Member

It does create a lot of bloat, though - the autosegs were used precisely to AVOID what this PR reinstates.

@madame-rachelle
Copy link
Collaborator

It does create a lot of bloat, though - the autosegs were used precisely to AVOID what this PR reinstates.

So I decided to give this statement a fair shake, and needless to say I am entirely unconvinced.

https://drive.google.com/file/d/1RYLWZF9UI-GAHo_PJsN8KGmWLpqhAegl/view?usp=sharing

To comply with the GPL and show that I am not doing anything fishy, both were compiled from this branch: https://github.com/madame-rachelle/gzdoom/tree/custom2 - this is a dev branch that I am using to aid in game development and just has a few extra commits to aid with that.

gzdoom-test1.exe is compiled without this PR.
gzdoom-test2.exe is compiled with this PR.
2025-01-07 03:27 11,380,736 gzdoom-test1.exe
2025-01-07 03:30 11,964,416 gzdoom-test2.exe
Startup times for both is nearly identical. FPS entirely unaffected.

Now I have a few questions. How is 583680 bytes a "lot of bloat"?

Most importantly, with the nearly identical startup times and a measly 5.12867% (yes I plugged that into Calculator to get that exact value) increase in executable size, how does holding onto ancient hacks that are actively holding back the development of the port a justification for such small gains?

@prof-hastig
Copy link
Contributor

There is one issue with this unrelated to the philosophy of using platform specific code or not:

The way the static FAutoseg is set up to not construct its TArray is a bit gross. TArray does have a proper constructor for static global variables so adding fragile hacks like this is not necessary.

Aside from this I got one question: Which platform is not compatible with the old code? If this was such a problem why has nobody mentioned it before as a practical issue?

@RicardoLuis0
Copy link
Collaborator Author

RicardoLuis0 commented Jan 7, 2025

The way the static FAutoseg is set up to not construct its TArray is a bit gross. TArray does have a proper constructor for static global variables so adding fragile hacks like this is not necessary.

The issue is initialization order, the other alternative would be to hold the TArray in a static variable inside a member function to force it to be initialized before it's used:

template<typename T>
class FAutoSeg
{
    // [...]
    TArray<T> &GetFields()
    {
        static TArray<T> fields;
        return fields;
    }
    // [...]
}

and

template<typename T>
struct FAutoSegEntry
{
    FAutoSegEntry(FAutoSeg<T> &seg, T* value)
    {
        seg.fields.push_back(value);
    }
}

(but that silently breaks if more than one FAutoSeg is defined for a given T)

@Boondorl
Copy link
Contributor

Boondorl commented Jan 7, 2025

Aside from this I got one question: Which platform is not compatible with the old code? If this was such a problem why has nobody mentioned it before as a practical issue?

It did end up being a practical problem recently which is why it was taken under a microscope in the first place. This platform dependent code hell is just begging for all kinds of issues should it need to be changed/examined again in the future, and whatever issue it was trying to solve a decade ago is probably completely irrelevant at this point.

Also, I'm going to ask you don't switch accounts during the middle of a conversation, Graf. It's childish.

@prof-hastig
Copy link
Contributor

prof-hastig commented Jan 7, 2025

Why don't you use the TArray special constructor for this case?

`
////////
// This is a dummy constructor that does nothing. The purpose of this
// is so you can create a global TArray in the data segment that gets
// used by code before startup without worrying about the constructor
// resetting it after it's already been used. You MUST NOT use it for
// heap- or stack-allocated TArrays.
enum ENoInit
{
NoInit
};
TArray (ENoInit dummy)
{
}

@RicardoLuis0
Copy link
Collaborator Author

RicardoLuis0 commented Jan 7, 2025

Why don't you use the TArray special constructor for this case?

` //////// // This is a dummy constructor that does nothing. The purpose of this // is so you can create a global TArray in the data segment that gets // used by code before startup without worrying about the constructor // resetting it after it's already been used. You MUST NOT use it for // heap- or stack-allocated TArrays. enum ENoInit { NoInit }; TArray (ENoInit dummy) { }

ah, didn't realize those were a thing, my bad

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.

5 participants