✨ Stop marking Sound as sealed

[mathew-odwyer]

Complete The Items Below

• I have updated the title without removing the ✨ emoji.
• I searched whether or not a feature request already exists.

Feature Request Purpose

Personally, when providing my own layer on top of Sound it would make life easier as I wouldn't resort to creating an instance and invoking it's methods like so:

public class MySound : Sound, IMySound
{
    private Sound sound;

    public MySound(string filePath)
    {
        this.sound = new Sound(filePath);
    }

    public void Play()
    {
        this.sound.Play();
    }
}

Solution

Marking it as sealed means I have to write more boilerplate; when I could just do:

public class MySound : Sound, IMySound
{
}

Anything Else

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[CalvinWilkinson]

@softwareantics
Just some questions for curiosity sakes.

Is your goal to create your own layer to abstract away to the users of your library that CASL is even used and to prevent them from having access to CASL types?
OR
Is your goal to extend the CASL types?

The reason I ask these is because the sound in CASL is meant to be sealed.

This is for 2 reasons below:

1. Since this is a library, it is much better to keep public APIs sealed and not extensible unless it is really needed. This helps reduce breaking changes in the future for library users.

   Note For example, it is not a https://github.com/KinsonDigital/CASL/labels/%F0%9F%A7%A8breaking%20changes to remove sealed from a class in a future release, but it is a https://github.com/KinsonDigital/CASL/labels/%F0%9F%A7%A8breaking%20changes when adding sealed to a class in a future release.

2. There a major performance benefits with sealed classes over non-sealed classes.

   Note Performance in the context of the Sound class is related to loading content. Loading content in games is a heavy hitter when it comes to performance

I apologize but for these reasons, I am very hesitant right now to remove the sealed from the Sound class. This is a possibility though as time goes on if there are enough people asking and it becomes more of a good reason to do so.

Currently, Velaptor is actually wrapping the class to abstract away the CASL type from Velaptor users.

Now one point that somebody might make is that the performance overhead involved with wrapping the Sound class would decrease performance due to the Sound class and the wrapper class having to be instantiated. This would also increase allocations. But you know what they say about performance, and that is to not make assumptions and to go prove it out for yourself.

So this is what I am gonna do. I am gonna label and comment this issue appropriately and go benchmark this idea and I will verify personally if the performance decrease in wrapping the Sound class will be more of a performance decrease than the performance increase by using the class as sealed vs not sealed without wrapping it.

If the performance gains by using sealed is still more than the loss with wrapping the class, then I will keep the class as sealed for now until there is more need in the future to change the class to non-sealed.

I will post my performance findings in the comments as well for transparency.

[CalvinWilkinson]

Waiting for https://github.com/KinsonDigital/CASL/labels/%F0%9F%94%ACresearch to be done based on the previous comment.

[mathew-odwyer]

My goal is to do both. At some point I'd like to design some signal processors.

However, my engine, for the most part has a collection of interfaces that users are welcome to implement however they see fit. That being said, CASL is being used to provide an implementation of my (currently super lacking) audio backend.

Looking over your reasoning I can see why it would be ideal to mark the class as sealed and doing so could affect other users upon a new release.

Thinking about it more has also made me realise that if this change was made, it would make it hard for me to test my class. So, it might just be easier for me to inject ISound into the constructor and call it a day.

[CalvinWilkinson]

My goal is to do both. At some point I'd like to design some signal processors.

However, my engine, for the most part has a collection of interfaces that users are welcome to implement however they see fit. That being said, CASL is being used to provide an implementation of my (currently super lacking) audio backend.

Looking over your reasoning I can see why it would be ideal to mark the class as sealed and doing so could affect other users upon a new release.

Thinking about it more has also made me realise that if this change was made, it would make it hard for me to test my class. So, it might just be easier for me to inject ISound into the constructor and call it a day.

I can see the points on both sides.

[CalvinWilkinson]

@softwareantics

So I did some basic perf testing and also hit up some blogs. Here is a great one about sealed and performance.

Breaking down the numbers from this post, I calculated the increase in performance based on the blog post.

Usage Type	Non Sealed	Sealed	Perf Increase
Calling Virtual Methods	0.4465	0.0107	4072.90%
Casting Objects	1.656	0.1505	1000.33%
Arrays	3.42	2.951	15.89%

---

Now these time scales are in the range of nano-seconds. This comes out to 0.0000034ms!! So, it is very very fast. The testing only took into account the absolute minimum for testing the concept of sealed. So basically, there is no implementation in the methods. But the results are relative of course.

So if for some reason the class was exactly as in the tests in the blog post, that means that one operation could execute 4,873,294.35 times in a 16ms frame time. Great!! But not so great. 😔

A class is useless if it does nothing. This is why everybody says to benchmark, benchmark, benchmark. Everybody's code and use cases are different.

I have no idea how the Sound class would perform but I know for a fact that it would not be anywhere near 3.42ms. So as of right now, the performance that I would gain by leaving it sealed would get negated by the performance that class implementation would have. Also, who in their right mind is loading thousands of sounds, let alone hundreds of thousands of sounds in such a small time frame. The use case for loading Sound completely trumps the gains I would get by leaving it sealed.

So basically the MAIN benefit that I want sealed is not for performance, but for not having to break things later. The idea here is to unlock things as needed and requested by a case by case basis as to reduce breaking changes. Like right now. I took some time to weight the upsides and downsides of your request, which came up with some interesting things.

So due to the performance benefits being too small compared to the Sound classes use case, I think it would benefit people more to have it not sealed so you can have more control of how you want to use it.

I will be removing the unsealing the class!!

Thanks for the request!!

More Info:

• Here is a good video on why you should seal classed by default.