Advertisement

Weird crash in operator overloads

Started by February 19, 2021 03:13 PM
7 comments, last by WitchLord 3 years, 9 months ago

Hi there,

I recently updated to the latest AngelScript version from … like 2014 or something. Had to rewrite some parts of my script bindings, especially the asBehaviour vs. opStuff functions.

Small side note: why? This is not an improvement, and it seriously complicated my math library. More on that at the end.

But first: the crash: AngelScript code looks like this

AssignSomeColor( TColor( GetColor()) * pow( 0.1f, GetTimeDiff()));

Severely reduced snippet. Involved operators are these:

explicit constexpr TColor::TColor( const TAlphaColor& f) noexcept { ... }
inline constexpr TColor operator * (const TColor& f, float scale) noexcept
 { return TColor{ ... };
 }

mEngine->RegisterObjectBehaviour( "TColor", asBEHAVE_CONSTRUCT, "void f( const TAlphaColor& in)", asFUNCTIONPR( ScriptHelper::ConstructColor, (const TAlphaColor &, TColor *), void), asCALL_CDECL_OBJLAST);
mEngine->RegisterObjectMethod( "TColor", "TColor opMul( float)", asFUNCTIONPR( operator *, (const TColor&, float), TColor), asCALL_CDECL_OBJFIRST);

So it looks like a standard set of constructor and operator overload. But it crashes. Something in the above line leads AngelScript to pass the TColor argument of operator * by value instead of by reference. It crashes inside the operator * with the first Argument being the bit value of a float disguised as a pointer/reference. With some ugly inspection magic I can actually see the three floats being passed by value. The method registration cited above is a class method, so it should pass the first arg by reference.

Am I doing something wrong? I have a whole bunch of these operator invocations all over the script functions. And they work everywhere else. TColor * float, float * TColor, TColor / float, all fine. The only difference I can spot is that in this line I'm creating a temporary TColor object.

And concerning the opStuff method of registering operator overloads: this forces the C++ operators to take their arguments as const ref. And this leads to less-than-ideal code generation in CLang and Visual Studio. In simple cases the Compiler emits the same code, in some more complex cases though this leads to severe performance regressions. I understand this is the way now, but I still wanted you to know.

Thanks in advance for any hint or comment. Oh wow, it must be 10 years and I still have that signature. I'm not even an indie anymore.

----------
Gonna try that "Indie" stuff I keep hearing about. Let's start with Splatter.

Schrompf said:
Am I doing something wrong? I have a whole bunch of these operator invocations all over the script functions. And they work everywhere else. TColor * float, float * TColor, TColor / float, all fine. The only difference I can spot is that in this line I'm creating a temporary TColor object.

I don't see anything wrong what little code you have shared. Would you be able to share the full code for the TColor class and how you have registered it? Or else a working code snippet that reproduces the problem so I can debug it?

It could very well be that you've come across a bug in angelscript, but in order to confirm that I need to be able to reproduce the problem.

Schrompf said:
Small side note: why? This is not an improvement, and it seriously complicated my math library. More on that at the end.

This was done to have a single unified way of overloading operators from scripts and when registering the application interface.

Schrompf said:
And concerning the opStuff method of registering operator overloads: this forces the C++ operators to take their arguments as const ref. And this leads to less-than-ideal code generation in CLang and Visual Studio. In simple cases the Compiler emits the same code, in some more complex cases though this leads to severe performance regressions. I understand this is the way now, but I still wanted you to know.

Can you elaborate more on this? If there is a way to improve performance without sacrificing design I'm interested in hearing about it. What change would you suggest in order to improve the performance?

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

Advertisement

Hi WitchLord, and thanks for the quick reply.

I tried to produce a minimal reproduction example, but I'm at a few thousand lines now and the crash still doesn't occur in it :-( Would you be willing to try it in the full live build? ~500MB of download, should build out of the box in recent VisualStudio 2019, or I zip all of it ready-built, which is a few GB then. Run, load a save game, kill the last cyborg, see the crash. The stuff there is difficult to see, though, as the various script code sections are hidden inside the level file format and can't be changed anymore.

Please do not refrain from saying “no”. From what I gathered around here your life is quite stuffed right now.

Can't quote here, sorry, this javascript editor crashed two times while I tried. So without quotation on the topic of code performance:

I do not have any suggestions for AngelScript, at least no non-destructive options. The current method of registering operators is by fake member functions. In my code most operators are free functions, at least in my code. So we have an C++ operator like this:

constexpr Vector operator + (Vector a, Vector b) noexcept { return Vector{ a.x + b.x... }; }

In AngelScript this operator can only be registered as

engine->RegisterObjectMethod( "Vector", "Vector opAdd( Vector)", asMETHODPR( Vector, operator +, (Vector), Vector), asCALL_CDECL_OBJFIRST);

This means that the first argument to the Plus operator is the this pointer. So the first argument can't be ByValue, it needs to be ByReference. This is ok for the moment, I changed the signatures of my operator overloads to take their arguments as const ref. But over the years I met several occasions where I learned that compilers have a hard time optimizing code when pointers or references are involved. It's probably due to aliasing analysis - args ByValue are local and therefore isolated, args ByReference can alias. A simple example would be

operator += (const Vector& o) { this->x += o.y; this->y += o.x; }

Artificially exagerated, nobody would do this, but it nicely shows the issue: If the method would take the arg ByValue, the code could be vectorized because o is always independent. But as soon as it's ByReference, the compiler can't be sure if the arg aliases this, and an optimized operation would modify this→x before being used as o.x in the second statement.

In this example it's obvious, and you could probably godbolt it easily to prove the compiler still does the right thing. But if operators are stacked more deeply and more complex, this fails soon. This is the reason why I rewrote all of my simple operator overloads to take their args ByValue over the years.

So the only suggestion I have is “allow operators to be free functions”, and this would be quite a large change. You're not going to do this, understandably. So I have no suggestions.

----------
Gonna try that "Indie" stuff I keep hearing about. Let's start with Splatter.

Schrompf said:
Please do not refrain from saying “no”. From what I gathered around here your life is quite stuffed right now.

You're right. I wouldn't have the time to work through the full code. But if you can just send me the code for the TColor type and how you register it. I might be able to figure out what's going on.

Schrompf said:
But over the years I met several occasions where I learned that compilers have a hard time optimizing code when pointers or references are involve

This is an interesting point. I had never really noticed that and intuitively I always thought that by reference would be better since it would not require a copy of the value, but I can see how the compiler would be able to optimize by value better. The question here is really how much of a performance improvement are you getting by doing it by value instead of by reference? If it is worth it I'll certainly look into adding support for operator overloads as free functions.

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

WitchLord said:

Schrompf said:
Please do not refrain from saying “no”. From what I gathered around here your life is quite stuffed right now.

You're right. I wouldn't have the time to work through the full code. But if you can just send me the code for the TColor type and how you register it. I might be able to figure out what's going on.

No need to! I just achieved reproducing it, and since then reduced it to a mere 70 lines. http://www.splitterwelten.info/privat/scripterror.zip​ (500kb)

I also learned that I should put a “const” on every operator method. This marks the corresponding this arg const and therefore allows accepting args from const Ref& getSomething().

Schrompf said:
But over the years I met several occasions where I learned that compilers have a hard time optimizing code when pointers or references are involve

This is an interesting point. I had never really noticed that and intuitively I always thought that by reference would be better since it would not require a copy of the value, but I can see how the compiler would be able to optimize by value better. The question here is really how much of a performance improvement are you getting by doing it by value instead of by reference? If it is worth it I'll certainly look into adding support for operator overloads as free functions.

Interesting. I can't put any hard numbers. Simple benchmarks like crunching a million vectors aren't affected at all. Last time I actually measured a difference was a complex concatenation of templated functions which did a switch() and some calculations based on an enum in its args. Passing those args by reference emitted a few hundred instructions, passing those args by value and suddenly Clang was able to resolve the switch( some_enum ) at compile time and the whole two pages of C++ code folded to a few instructions.

As you might have guessed this left a lasting impression on me.

----------
Gonna try that "Indie" stuff I keep hearing about. Let's start with Splatter.

Hmm. I wasn't able to reproduce the problem with the code you sent.

Would you mind giving the latest WIP version a try to see if the problem has been fixed already?

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

Advertisement

Thanks for the quick response. Weird it doesn't crash for you, but I tried the WIP version and indeed it fixes it. First line of changelog was a strong hint, too.

Thank you very much for your continued support! Wouldn't have expected this when I choose Angelscript back in… 2006 or something. Wow.

----------
Gonna try that "Indie" stuff I keep hearing about. Let's start with Splatter.

Schrompf said:
Weird it doesn't crash for you

Not so weird. I only tried it with the latest WIP ?

Schrompf said:
Wouldn't have expected this when I choose Angelscript back in… 2006 or something. Wow.

Yeah, had I known I would still be doing this in 2021 when I first started on AngelScript back in 2003 I probably wouldn't have started at all. But I'm glad I did, this is by far my favorite project to work on, I just wish I had more time to spend on it.

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

This topic is closed to new replies.

Advertisement