[RESOLVED] Too many casts...

[Mybowlcut]

Hey.

I'm writing a function that involves calculating the percentage of XML elements loaded from a file. The calculation that determines the percentage uses heaps of casts though:

// Reads multiple Ts, starting from element (element must be pointing to the first element
// or Ts will be skipped).
// Pushes a MESSAGE screen event at each percentage interval specified by "interval".
// Uses "estimated_elements" to determine the percentage.
// "author_name" is a way to let any listeners know the name of the object that called this function.
template<typename T, typename Container>
Container& get_multi_element_progress_read(Element* parent, const std::string& child_name,
Container& container, const std::string& author_name, unsigned int estimated_elements,
unsigned short percentage_interval, bool throw_if_not_found = false)
{
    Element* element = parent->FirstChildElement(child_name, throw_if_not_found);
    T t;
    // Can't assume container will be empty. We only want the
    // quantity of elements that we've added.
    unsigned short start_size = container.size();
    unsigned short percent_done = 0;
    while(element)
    {
        container.insert(container.end(), get_element_read(element, t));
        // Need cast otherwise calculation won't work.
        // Times by 1000.0 instead of 100.0 because we don't want decimal percentages (e.g. .03).
        // Instead, we want want integer percentages from 0 to 100.
        percent_done = static_cast<unsigned short>((container.size() - start_size) /
                static_cast<double>(estimated_elements) * 1000.0);
        if(percent_done % percentage_interval == 0)
        { // We're at the desired interval; let listeners know of read progress.
            Screen_Event_Manager::Get().Push(Screen_Event(
                MESSAGE, author_name, "", Conversions::to_string<unsigned short>(percent_done)));
        }
        element = element->NextSiblingElement(child_name, throw_if_not_found);
    }

    return container;
}

Can anyone recommend a more efficient way of doing this calculation?

Cheers.

[Paul McKenzie]

Can anyone recommend a more efficient way of doing this calculation?

Do those casts really cause any inefficiency? None of them are dynamic_cast<>, which may cause some performance degradation.

Regards,

Paul McKenzie

[ixSci]

static_cast<double>(estimated_elements) * 1000.0);

This is useless cast. Result of this expression will be double without any casts

static_cast<unsigned short>((container.size() - start_size)

and this is unsafe cast. The result of the expression may exceed the unsigned short. It would be better to use boost::numeric_cast if you can

[Mybowlcut]

Do those casts really cause any inefficiency? None of them are dynamic_cast<>, which may cause some performance degradation.

Regards,

Paul McKenzie

Well as it stands currently, the level loading takes a very long time loading 1000 tile objects - that is the whole reason I thought of making a progress bar. But if there are 5000 tiles, it's going to take a whole lot longer and I've always had Meyer's advice in the back of my head regarding casts (Item 27 Effective C++: Minimize Casting). I was just worrying that these casts were unecessary or could be eliminated. There seemed to be so many in one place to do such a small thing and it seemed stupid.

This is useless cast. Result of this expression will be double without any casts

I took it out and it didn't work. I'm pretty sure this is because of the order of evaluation. An int divided by an int will always be an int.

and this is unsafe cast. The result of the expression may exceed the unsigned short. It would be better to use boost::numeric_cast if you can

Good point. I changed it (and replaced 1000.0 with 100.0 because my logic was wrong):

// Reads multiple Ts, starting from element (element must be pointing to the first element
// or Ts will be skipped).
// Pushes a MESSAGE screen event at each percentage interval specified by "interval".
// Uses "estimated_elements" to determine the percentage.
// "author_name" is a way to let any listeners know the name of the object that called this function.
template<typename T, typename Container>
Container& get_multi_element_progress_read(Element* parent, const std::string& child_name,
Container& container, const std::string& author_name, unsigned int estimated_elements,
unsigned short percentage_interval, bool throw_if_not_found = false)
{
    Element* element = parent->FirstChildElement(child_name, throw_if_not_found);
    T t;
    // Can't assume container will be empty. We only want the
    // quantity of elements that we've added.
    size_t start_size = container.size();
    size_t percent_done = 0;
    while(element)
    {
        container.insert(container.end(), get_element_read(element, t));
        percent_done = static_cast<size_t>((container.size() - start_size) /
            static_cast<double>(estimated_elements) * 100.0);
        if(percent_done % percentage_interval == 0)
        { // We're at the desired interval; let listeners know of read progress.
            Screen_Event_Manager::Get().Push(Screen_Event(
            MESSAGE, author_name, "", Conversions::to_string<unsigned short>(percent_done)));
        }
        element = element->NextSiblingElement(child_name, throw_if_not_found);
    }

    return container;
}

[ixSci]

I took it out and it didn't work. I'm pretty sure this is because of the order of evaluation. An int divided by an int will always be an int.

yes, I was wrong. I didn't see that the expression after division operator wasn't wrapped by the parentheses

[ixSci]

I think it'll be better if you write something like it:

...
size_t percent_done = 0;
double tmp_estimated_elements = estimated_elements;
    while(element)
    {
        container.insert(container.end(), get_element_read(element, t));
        percent_done = (container.size() - start_size) /
            tmp_estimated_elements * 100.0;
...

Now you have one implicit conversation outside the loop and one conversation inside the loop instead of two perfomance-consuming(perhaps) and one useless casts inside the loop. It seems good

BTW, how did you highlight your code? I tried [code=cpp] and [code=c++] but I wasn't successful

[Mybowlcut]

I think it'll be better if you write something like it:

...
size_t percent_done = 0;
double tmp_estimated_elements = estimated_elements;
    while(element)
    {
        container.insert(container.end(), get_element_read(element, t));
        percent_done = (container.size() - start_size) /
            tmp_estimated_elements * 100.0;
...

Now you have one implicit conversation outside the loop and one conversation inside the loop instead of two perfomance-consuming(perhaps) and one useless casts inside the loop. It seems good

BTW, how did you highlight your code? I tried [code=cpp] and [code=c++] but I wasn't successful

Excellent haha. I had actually thought of doing that, too! Good idea. It now looks like this:

// Reads multiple Ts, starting from element (element must be pointing to the first element
// or Ts will be skipped).
// Pushes a MESSAGE screen event at each percentage interval specified by "interval".
// Uses "estimated_elements" to determine the percentage.
// "author_name" is a way to let any listeners know the name of the object that called this function.
template<typename T, typename Container>
Container& get_multi_element_progress_read(Element* parent, const std::string& child_name,
Container& container, const std::string& author_name, unsigned int estimated_elements,
unsigned short percentage_interval, bool throw_if_not_found = false)
{
    Element* element = parent->FirstChildElement(child_name, throw_if_not_found);
    T t;
    typedef unsigned short ushort;
    // Can't assume container will be empty. We only want the
    // quantity of elements that we've added.
    ushort start_size = static_cast<ushort>(container.size());
    // We only push events if the percent has changed.
    ushort percent_done = 0, last_percent_done = 0;
    // Saves casts in the loop.
    double temp_est_elements = estimated_elements;
    while(element)
    {
        container.insert(container.end(), get_element_read(element, t));
        last_percent_done = percent_done;
        percent_done = static_cast<ushort>((container.size() - start_size) / temp_est_elements * 100.0);
        if(percent_done % percentage_interval == 0 && percent_done != last_percent_done)
        { // We're at the desired interval; let listeners know of read progress.
            Screen_Event_Manager::Get().Push(Screen_Event(
                MESSAGE, author_name, "", Conversions::to_string<ushort>(percent_done)));
        }
        element = element->NextSiblingElement(child_name, throw_if_not_found);
    }

    return container;
}

This function was designed with a particular SDL structure in mind (SDL_Rect) which has signed and unsigned shorts as members, so unfortunately I have to account for it. I could throw an exception if the container contains more elements than the capacity of an unsigned short?

Oh, I use Yves M's syntax highlighter. It's very handy.. click on the demo project link at the bottom of that link (I think that link should be re-named) and you'll get the program. It's completely standalone... I just chucked it in a folder in program files and made a shortcut to it in my quick launch bar. It's really useful.

Cheers!

[Paul McKenzie]

I've always had Meyer's advice in the back of my head regarding casts (Item 27 Effective C++: Minimize Casting).

Not having the book in front of me, Meyer's was mostly concerned with your code making a Car into an Elephant by casting. In other words, excessive casting results in code being incorrect at runtime, not necessarily slow.

However, the cast that usually does incur runtime overhead is dynamic_cast.

Regards,

Paul McKenzie

[Mybowlcut]

Not having the book in front of me, Meyer's was mostly concerned with your code making a Car into an Elephant by casting. In other words, excessive casting results in code being incorrect at runtime, not necessarily slow.

However, the cast that usually does incur runtime overhead is dynamic_cast.

Regards,

Paul McKenzie

Oh, ok. Cheers.