refactor markers_placement_finder #3338
Conversation
- refs mapnik#3327 Replace member variant of placement-type implementations with plain union. The active implementation is chosen at construction time like before. Make placement-type implementation classes virtual to allow invoking the active union member through a base class pointer.
|
I've kicked off a scratch build on Fedora of 3.0.10 with this patch applied: http://koji.fedoraproject.org/koji/taskinfo?taskID=13194331 |
|
Looks good - all three platforms built successfully on Fedora. |
|
Hmm.. but travis is still failing https://travis-ci.org/mapnik/mapnik/jobs/113005661. @tomhughes - have you tried latest master on Fedora? I have a feeling it might pass as well, curious to find out. |
Just gathering some stats so we can make an intelligent decisionsIf travis is to believe (big ?) current master got much further with the compilation /cc @lightmare @tomhughes @springmeyer refactor markers_placement_finder PRcurrent master |
|
Travis is only failing because the compile took too long right? I don't know what the timeout is in travis but i386 and x86_64 each took about 1h20 on the Fedora build farm, and arm took just under 4 hours. When you say "try master" do you mean with this patch? or without it? |
|
@tomhughes - just realised that If memory requirements are dropped this a very positive sign, thanks @lightmare. But I believe we can(must) also squash compile times to fit travis imposed 50 min limit. |
|
Giving a travis another chance ;) |
|
One thing about the Fedora builds is that I deliberately don't pass -j when building so the build is not parallel at all - that was done to fix previous problems running out of memory I think I'm right that mapnik now avoids building problem files in parallel though, so I can likely try adding that back in? |
libmapnik.dylib size + compile speeds on OS X with clang++master before #3338master after #3338 |
|
+1.5MB in lib size? That's another surprise 😕 The difference the is even bigger on Linux: 17.2 => 18.9 |
|
@artemp - regarding travis build times, I think they vary wildly, perhaps depending on overall load. My favourite build is 5006. There were a few green builds before it, but none after. And it coincides with the time we started fixing As for this commit increasing the library size by 10%, I don't get it. I removed the whole variant move-construction and apply_visitor machinery, and replaced it with 6 virtual function tables, that's all. Where does it come from? |
|
@lightmare - it's hard to be 100% sure, but v-tables means compiler can't do certain optimisations/inlining. The binary size increase is probably a trickle-down side-effect of this change. |
- move Locator/Detector-dependent stuff back to markers_point_placement - slightly reduces library size, by about 20% of what mapnik#3338 added
- move Locator/Detector-dependent stuff back to markers_point_placement - slightly reduces library size, by about 20% of what #3338 added
Replace member variant of placement-type implementations with plain
union. The active implementation is chosen at construction time like
before.
Make placement-type implementation classes virtual to allow invoking
the active union member through a base class pointer.
clang 3.6
gcc 4.8