TLDR, no real reason to waste your time reading this.
Mostly talking to myself here; decided to save all this rather than just bin it.
The shootout!it looks like I'll be dropping 5 of these alternatives from consideration right away.
Only 4 are out; 1 is back in, at least temporarily.
Turns out I was grabbing the wrong timestamps from the siteupdate.log files, and not measuring one of the tasks that slowed down in order to speed other tasks up. This made the newer alternatives artificially look better in comparison to siteupdate_2.
Meanwhile, 10 more alternatives were added, making 35 in total.
More on this below.
siteupdate_3mtx, 2mtx & 1mtx were 3 different flavors of Option 1 listed under "What to do"
here. The idea was to experiment with having fewer mutexes than the original 3, and balance reducing lock contention with reducing lock overhead. Less overhead wins; siteupdate_1mtx generally performed best, but the siteupdate_2+ alternatives outperformed all these; they were 3 of the 4 options dropped from considertion.
siteupdate_1B was
proposed to decrease mutex overhead by creating a temporary hash table. This one was absolute bottom of the barrel everywhere except on bsdlab, where its performance fell between siteupdate_3mtx & 2mtx.
siteupdate_2 as revised
here, when
Computing stats per traveler, iterates with each thread processing a
Region, and avoids bringing back per-traveler mutexes at the cost of setting up hash tables' key/value pairs in a threadsafe manner in another function beforehand. CompStatsRThread/compute_stats_r & CompStatsTThread/compute_stats_t are still separate entities. This is the alternative that was mistakenly removed from consideration then put back on the table; I'll weigh it against whatever gets selected from The Big 30 in the final round.
The Big 30:These all re-unify CompStatsRThread & CompStatsTThread into a single CompStatsThread that, again, iterates with each thread processing a
Region, as outlined in the last set of bullet points at the bottom of
this post.
So there are 30 remaining alternatives. 2 times 3 times 5.
For those unfamiliar with
brace expansion, hopefully this will still make some sense from context:
siteupdate_{3,4}{m,s,S}{t,ta,r,pa,ra}The
4 family attempted to improve on
3 by storing a few variables to eliminate some redundant dereferencing & arithmetic. In practice, this didn't have much effect.
The 3xx family was previously referred to as 2xx variants upthread, confusingly.
m vs
s: Right now,
this code is threadsafe, with each thread processing a
HighwaySystem. However, switching to processing a
Region will change that, and we'll either need a
mutex, or to
setup the key/value pairs in advance.
The capital
S family is where the 10 new alternatives came in --
m combined the
HighwaySystem mutexes as proposed in
yakra#210 to preserve my sanity & code organization/readability, while
s didn't really need to do that & went no-build. After noticing some counterintuitive benchmark results on lab2, I decided to go for a more apples-to-apples comparison, and
S combines the mutexes too. Prior benchmarks suggested a slight speedup anyway.
t is more of a no-build compared to
r -- it continues to keep a running
system_mileage sub
total, and add it to
r->system->mileage_by_region[this] afterwards, while
r defines
system_mileage as a
reference to
r->system->mileage_by_region[this] and adds to it directly. So we have one less arithmetic operation per route, and slightly cleaner code. No reason to use
t, really.
ta,
ra,
pa: 'a' is for "at". Background is at
#517; search for "No-Build". Leaving this ugly/clunky block of code as it was performed better than changing to a more concise
system->mileage_by_region[region] += system_mileage;It's possible that,
two days later, using the clang -O3 flag optimized away any/most of the difference. I don't remember testing that at the time; gonna have to do that now.
Annnd... it does appear to optimize that away, yes.
ta is no-build here, still adding/assigning the
total in the try/catch blocks.
Would-be
ra is less straightforward -- the reference can't be declared in the try/catch blocks due to scope limitations, and can't be declared before that as it must be assigned upon declaration.
pa declares a
pointer first, assigns it in the try/catch blocks, and dereferences if when adding to
*system_mileage.
ra declares a
pointer first, then uses it to get a
reference right after the try/catch.
The winner: siteupdate_3Sr!S slightly outperforms
s, and has cleaner code.
S slightly outperforms
m, though it's very close. I just like the idea of going mutex-free too.
No significant difference between
3 &
4. Going with
3 for cleaner code.
For the remaining "Big 5", no real observable difference in speed;
r has the cleanest code, and in theory is faster than
t even if not noticeably so in practice.
Taking another look at
2 compared to
3Sr, it's in the same ballpark. Slower on 6/7 machines, faster by about 12 ms on average on lab1. Just noise in the data.
Disadvantage of 3Sr: key/value pair setup for HighwaySystem::mileage_by_region during ReadWptThread.
Advantages of 3Sr: Less HighwaySystem mutex overhead; less pointer-chasing & arithmetic; no region mutexes; half the iterating thru routes because only one threaded ComputeStats task, not 2.
Plus, cleaner code -- fewer threads, fewer functions, fewer files, and fewer lines in what's left.
3Sr has the 2nd-cleanest code overall, with only 5 more lines than
3mr.
Anyway! Enough faffing about; I've waster a staggering amount of time. Time to delete all the other experimental HighwayData branches and get to work on committing this.
Dropping this here as I delete dead branches, in care I wanna refer back to it in the future:
3mtx 7b090e1be7a05fb173f27f0ccb1d83951c502707
2mtx bc71d69b7697ce22974b94137d13011c6fc6334f
1mtx b031cd744eab66d35078df29971ffd59e2019d77
1B 1e9fd5803803e8a3442f290d0ac65b08816e06fd
2 1b6f07d72a205a7aace79e9f9dea71412d537892
3mt 5b4794db55e8a0f64d499d720622b67cd2773ae9
3mta eadec347d03e9c05280f2e4938ea4edfe964ad75
3mr 4f91059cf83d628c0aa350829304a83a58dd29f4
3mpa 43c6d014f8781c0c30ed4f65b845542743530186
3mra 62ea0e698519b795c6a6edb23572943028eec134
3st 836a5f4b203d06bb53634441c51eae3d161ba3bd
3sta e29c2d9f6bc188d8866a23d94726fe0711eede86
3sr 2f394b13510d6d9c69e0e72d19be6bf72b0f546f
3spa 1e8bd750e89849796b3079ca094a9a7df86703e3
3sra a2cb69eb94a5be1038c1116dc6a688f263f45c61
3St 8bb3052c31e40f888006781eb397cf0bb2d6f707
3Sta a368b2ef656727b5bd5e84bee45561a9512b3a73
3Sr 9198035f49ae24842941eaec8be17d898e1baf5b
3Spa a1c39077211e693f5a67b328bcccb962e28a715b
3Sra 372ea9ed3ee8f184a0ab16d4ff17df0067fd5544
4mt 933ce42c93d37970d7d0c53c9042f7545ebb768b
4mta 7582a07b53fd04df0d6072581d208135fc0ecd32
4mr df840c78da7e75088db08c6584a45a4ffb691299
4mpa d01b65d995eb76f928e061a2b7de312fff4f68f2
4mra 1dbfcacaab3089f3db0e835bffe8a1f32cb46dff
4st 50492dada8521bdd5b8fe449dd50bdf4a06d6993
4sta d58e3edcfaf3afb81f273311296baff1b86340bb
4sr 518210b631e62b2f438a8ae9c4d547d12d7d46f9
4spa e1cf7b2f28cba203c60163dea11ca5d76d0389b7
4sra cc82288859fe56981f2af06f5d7eb71b76becf5c
4St 4eac70852d19573e7d56e80ef93054351753b205
4Sta c5ebe2ebf78071533528b13cc0bbddf995afc84c
4Sr 63d085cb27463438e8937debc53fff09ac92dd96
4Spa fe5d268886959791fa9cb29ed39031447a4a5987
4Sra 95e5cf58413d77ffad00d5cb40279304c60c0987
CompStats3 0eaf81ae77e613f3802f15089584c1e83f81ae7d
2sr 83f42cfd4c86eabe5ded700cd9fb0bc81db6aad4
2mr 0112222c87860fe89f9a94256f5ab981bec096e8
CompStats2 1e9fd5803803e8a3442f290d0ac65b08816e06fd
y210_rebase 62b6dc6a0e9526532708a3b7d8ac0c49a100aaac
Final ToDo List:Check wording in comments & fix if needed, incl CompStatsThread.cpp
Make sure all hash table pre-setup is conditioned out in siteupdateST -- or else done in siteupdate in the thread functions only.
Make sure siteupdateST still gives the same output.
Probably implement the rest of yakra#210, closing the issue.
Userlogs: Don't compare traveled vs total mileage, and instead see if num_con_rtes_clinched == everything in the system. Python too?
Housekeeping: refactor datachecks into HighwaySystem::route_integrity. Python too, to keep things similar.
Implement a few items from #574 since we already need to recompile things.Clear TravelerList::clinched_segments after concurrency augments? Leave it in in case I make changes to DB population? Decide later.
Interactive rebase / squash
