HaxeFlixel / flixel

Free, cross-platform 2D game engine powered by Haxe and OpenFL
https://haxeflixel.com/
MIT License
1.92k stars 427 forks source link

Use `FlxColorInt` instead of `Int` for RGBA to reduce variable size for CPP target #3171

Closed Just-Feeshy closed 3 weeks ago

Just-Feeshy commented 3 weeks ago

It is just an area for memory usage optimization for the CPP target. The primary purpose is to use unsigned char instead of regular int to avoid wasting memory. I wasn't able to reopen the previous branch because I was force-pushed.

This was the repo I made to do my testing and gathering of information: https://github.com/Just-Feeshy/Draw

Issue with the previous PR The reason why the FlxColor.subtract resulted in FFFF8000 instead of the intended FFFF0000 is that the test program tried to store the value -255 into a byte-sized integer, which surpasses the 8-bit capacity because of the negative sign.

What did I do to fix the PR? Since the variable is "connected to a setter," if that's the right way to phrase it, Haxe is a little weird regarding getters and setters. I just have the setter function data type instead be a 16-bit sized integer since 32-bit is still too much, so I can use at least 9 of those 16 bits to store the negative value. Since the getter doesn't deal with value manipulation, that means when retrieving the value of either red, green, blue, or alpha, the program retrieves the original 8-bit value of that variable. However, when setting a value to that variable, HXCPP is casting it to have the capacity of 16-bit for situations where a negative sign might be used.

The performance change with this PR HXCPP usually uses static casting for the different types of integers. There are no performance hits due to the casting being done in compile time rather than run time, though I'm by no means an expert on how HXCPP works. There might be cases where dynamic casting is present, but even then, it wouldn't cause a massive delay.

How much memory do I save with this change Since the main focus of my "testing repo" is to store a large capacity of FlxColors to see a significant change in memory consumption, these are the screenshotted results. It took a lot of work to gather data on the constant fluctuation of memory usage, so these might not be 100% accurate; I timed both tests for 10 minutes with no user interaction. This was tested, targeting the Arm64 processor using an M1 Pro Macbook.

This was the latest version of HaxeFlixel (not including my commit).

Test1

This was the test with the commits I made:

Test2

I gained about 3MB saved for using less memory for creating a FlxColor per pixel on a fixed resolution of 800x600.

Just-Feeshy commented 3 weeks ago

Hey @Geokureli, mind telling me what DemoState.hx is?

Geokureli commented 3 weeks ago

Hey @Geokureli, mind telling me what DemoState.hx is?

Where do you see that?

Just-Feeshy commented 3 weeks ago

Hey @Geokureli, mind telling me what DemoState.hx is?

Where do you see that?

I was testing by reducing the size for HashLink as well. However, the test case failed, and I saw it mentioned that it mismatched Float with FlxColorInt16 in DemoState.hx

Geokureli commented 3 weeks ago

I was testing by reducing the size for HashLink as well. However, the test case failed, and I saw it mentioned that it mismatched Float with FlxColorInt16 in DemoState.hx

Sounds like one of the flixel-demos failed. Theres 4 demos that have a DemoState.hx, your error should give the full path

Just-Feeshy commented 3 weeks ago

I was testing by reducing the size for HashLink as well. However, the test case failed, and I saw it mentioned that it mismatched Float with FlxColorInt16 in DemoState.hx

Sounds like one of the flixel-demos failed. Theres 4 demos that have a DemoState.hx, your error should give the full path

Thanks, I'll check out what's going on.

Geokureli commented 3 weeks ago

I've decided not to merge this PR, I did some personal tests and saw no difference, and did not expect to see a big difference, becuase ints are not garbage collected, they clear from memory when the stack exits the function where they were created, unlike by-reference objects. The task manager is not an accurate measurement your program's memory usage, also most of my projects will climb by 3mb when sitting idle for a few minutes, your program, does too.

On top of not making a measurable difference, I think this adds complexity to the class when adding or modifying features, it will also potentially cause new errors in old projects, like so:

final col = FlxColor.WHITE;
final arr = [col.alpha, col.red, col.green, col.blue];
final arrInt:Array<Int> = arr;// error: flixel.util._FlxColor.FlxColorInt8 should be Int

To quote the Contribution Guide:

Only submit features that you have a practical use for [...]. Please, do not submit features that you think people will "theoretically" need

So I must ask, what problem did you detect in your project that was remedied by this change. My guess is that you only "theorized" that this will help. In the end FlxColors are still Ints, even after this change, the only thing this changes is what is used to change specific components of a color and your test case rarely ever manipulates specific components of colors. the only place I could find that would be affected by your change is the single call to private static var defaultStroke:FlxColor = FlxColor.fromRGB(51, 51, 51); up top, this only happens once in the entire program, and I doubt that is the reason for the 3mb difference in your task manager

Just-Feeshy commented 3 weeks ago

I think you are right about this being more theoretical than practical. The point of the "Draw" repo was to store a large quantity of FlxColor in a 2D array to show that it has a practical use case since I did read the Contributing.md. However, after doing the math, with the 800x600, you'll only save about 1.92 MBs since the main focus was the __giantMatrix since it gets removed from the stack once the program closes. Still, it's not as big of a change as I initially thought, and it's too specific for it to matter.

Just-Feeshy commented 3 weeks ago

I've decided not to merge this PR, I did some personal tests and saw no difference, and did not expect to see a big difference, becuase ints are not garbage collected, they clear from memory when the stack exits the function where they were created, unlike by-reference objects. The task manager is not an accurate measurement your program's memory usage, also most of my projects will climb by 3mb when sitting idle for a few minutes, your program, does too.

On top of not making a measurable difference, I think this adds complexity to the class when adding or modifying features, it will also potentially cause new errors in old projects, like so:

final col = FlxColor.WHITE;
final arr = [col.alpha, col.red, col.green, col.blue];
final arrInt:Array<Int> = arr;// error: flixel.util._FlxColor.FlxColorInt8 should be Int

To quote the Contribution Guide:

Only submit features that you have a practical use for [...]. Please, do not submit features that you think people will "theoretically" need

So I must ask, what problem did you detect in your project that was remedied by this change. My guess is that you only "theorized" that this will help. In the end FlxColors are still Ints, even after this change, the only thing this changes is what is used to change specific components of a color and your test case rarely ever manipulates specific components of colors. the only place I could find that would be affected by your change is the single call to private static var defaultStroke:FlxColor = FlxColor.fromRGB(51, 51, 51); up top, this only happens once in the entire program, and I doubt that is the reason for the 3mb difference in your task manager

I think you are right about this being more theoretical than practical. The point of the "Draw" repo was to store a large quantity of FlxColor in a 2D array to show that it has a practical use case since I did read the Contributing.md. However, after doing the math, with the 800x600, you'll only save about 1.92 MBs since the main focus was the __giantMatrix since it gets removed from the stack once the program closes. Still, it's not as big of a change as I initially thought, and it's too specific for it to matter.

Geokureli commented 3 weeks ago

after doing the math, with the 800x600, you'll only save about 1.92 MBs since the main focus was the __giantMatrix since it gets removed from the stack once the program closes.

__giantMatrix is a Array<Array<FlxColor>>, and as I mention before, FlxColors will still be Ints, even with this change. so I don't see how you would save any memory in your example

Just-Feeshy commented 3 weeks ago

That is another flaw I completely forgot to mention, which I should have considered properly. I thought Hxcpp would store the values of rgba into other arrays for caching getters, and I figured out that it doesn't after I tried to fix the HashLink build and then read through the source for the compiled cpp target. As I said, it would be such a minor change even if it did.

I should have known better beforehand, and the reason why I assumed so in the first place was because when I saw 3 MBs save now, I don't know why now.

after doing the math, with the 800x600, you'll only save about 1.92 MBs since the main focus was the __giantMatrix since it gets removed from the stack once the program closes.

__giantMatrix is a Array<Array<FlxColor>>, and as I mention before, FlxColors will still be Ints, even with this change. so I don't see how you would save any memory in your example