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

TravelerList::system_region_miles #228

Open
yakra opened this issue Apr 24, 2022 · 2 comments
Open

TravelerList::system_region_miles #228

yakra opened this issue Apr 24, 2022 · 2 comments

Comments

@yakra
Copy link
Owner

yakra commented Apr 24, 2022

This function. This function right here.

/* Return mileage across all regions for a specified system */
double TravelerList::system_region_miles(HighwaySystem *h)
{ double mi = 0;
for (std::pair<Region* const, double>& rm : system_region_mileages.at(h)) mi += rm.second;
return mi;
}


  1. Change the name. Probably named in the early "translate from Python" days before I had a clear picture of what was going on. There's nothing regional about it. It's the total across all regions.
  2. double t_system_overall = 0;
    if (system_region_mileages.count(h))
    t_system_overall = system_region_miles(h);
    This has to look up the HighwaySystem twice. Once to make sure we're good to call the function, and again once the function is called. Instead, just do double t_system_overall = system_region_miles(h);, have the function catch the exception and keep on truckin', returning 0.
    Nope! 6.8310 -> 7.0672 s on BiggaTomato.
    Maybe this could be a good improvement for FreeBSD? 😀 Try it anyway for giggles... Nope. 5% longer.
  3. auto it = t->system_region_mileages.find(this);
    if (it != t->system_region_mileages.end())
    { sprintf(fstr, ",%.2f", t->system_region_miles(this));
    sysfile << t->traveler_name << fstr;
    for (Region *region : regions)
    try { sprintf(fstr, ",%.2f", it->second.at(region));
    Eliminating the redundancy here is trickier. It's not as simple as calling the function and proceeding if result > 0, because the iterator is reused later. Maybe dereference it & use std::accumulate.
  4. On that note, would std::accumulate magically be faster for item 1 above? Because accumulators LOL.
@yakra
Copy link
Owner Author

yakra commented Apr 24, 2022

userlog versions

tag BT
time
commit descr
522 6.8310 e36c526
hrm1a 6.8232 a021901 std::accumulate with overloaded +. Only check membership @ beginning.
No more derefs yet.
hrm0 6.8316 2 redundant conditionals combined into 1. No diffs. 0.3 ms slower? Just noise! 🤣
Less code + less branches = good!
hrm1b 6.8363 hrm1a + hrm0 lol I want a mulligan on the time!
hrm1c 6.8355 ff2b65f map_sum template. Fewer lines of code than overloaded +
surely lower compile time than numeric. :P

@yakra
Copy link
Owner Author

yakra commented Apr 27, 2022

Seems to make things slower, counterintuitively. Might get back to this later.
Rather than overload the + operator & #include <numeric> entirely (compilation time?) to use std:accumulate, here's what I came up with:

template <class map>
double map_sum(const map& m)
{	double sum = 0;
	for (auto& kv : m) sum += kv.second;
	return sum;
}

Leaving this here for now while I delete the branch.
May come back to this once other options are in place, e.g. the chop3 branch's to_write & format_clinched_mi optimizations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant