-
Notifications
You must be signed in to change notification settings - Fork 7
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
node-mapnik faster on linux #78
Comments
I did some digging here (still on linux). I've noticed 2 things:
Doing this:
Gives this for And this for |
Further digging reveals one potential cause for I found this out by using I ran the benchmark with just one iteration (to keep things sane): gdb --args `which node` bench/bench.js --iterations 1 --concurrency 1 --package vtcomposite --compress --mem --only 'tiles completely made of points, overzooming, no properties' I changed the bench.js code like this (to reduce the deflate calls from node.js's diff --git a/bench/bench.js b/bench/bench.js
index 0da5d22..b6c405d 100644
--- a/bench/bench.js
+++ b/bench/bench.js
@@ -46,7 +46,7 @@ if (rules.length < 1) {
}
rules.forEach(function(rule) {
- if(argv.compress){
+ if(false){
rule.tiles.forEach(function(t){
const compressedTile = zlib.gzipSync(t.buffer);
t.buffer = compressedTile; Then in
Then I ask gdb to skip past all matches:
Then I ran the program:
That reveals that
Doing the same thing for This shows that node-mapnik only called
|
Okay, this hypothesis is starting to feel solid: that I've now also found that And I've been able to isolate outside of diff --git a/bench/bench.js b/bench/bench.js
index ea4fc3f..9175975 100644
--- a/bench/bench.js
+++ b/bench/bench.js
@@ -100,6 +100,7 @@ function runRule(rule, ruleCallback) {
if (err) {
throw err;
}
+ fs.writeFileSync('testcase-vtcomp.mvt',result);
return done(null,result);
});
break;
@@ -126,6 +127,7 @@ function runRule(rule, ruleCallback) {
if (err) {
return cb(err);
}
+ fs.writeFileSync('testcase-mapnik.mvt',result.getData());
let options = {compression:'none'}
if (rule.options.compress){ Then I ran the CLI from mapbox/gzip-hpp#29 on each of these
Which outputs this for
And this for
|
Aha! Dumping the
And 2 {
3: 1
- 1: 2000
4: "\t\240.\370."
+ 1: 2000
} So node-mapnik features look like:
In this order: While vtcomposite features look like:
In this order: test data at https://gist.github.com/springmeyer/743828e76218b2d2e70a02e3dbd9ff12 |
Wow. The different field order explains the difference in speed between node-mapnik and vtcomposite. Here is my proof: I rebuilt node-mapnik (on linux/stageyprod) with this patch applied to mapnik-vt (which makes the field order the same as vtcomposite): https://gist.github.com/springmeyer/407798879f7b552a3af52e7d3ecc81f3 Then I ran the benchmark again and profiled and 💥 the massive And the bench shows the difference too: normal/unmodified node-mapnik:
vtcomposite master:
modified node-mapnik (https://github.com/mapnik/node-mapnik/compare/vtzero-feature-field-order) with crippled performance due to the new gzip bottleneck:
|
The only guess I have here is that the different order either makes the data align better in some way or makes better use of CPU caches or so. The If this is the case, it should also depend on the IDs used, whether they fit in a byte or need more. Of course we could change the order of the fields in vtzero, but that could make other cases worse and/or depend on the zlib version and other details. We need many more benchmarks before we could justify this. It might well be that with slightly other data, things are different and the benchmarks are misleading here, because the data they use is quirky in some way. Oh, and there is a reason the |
I think @millzpaugh and @alliecrevier may be seeing a similar pattern as my single/local test in production with a huge range of traffic. But they will need to confirm. If production traffic shows an overall slowdown with vtcomposite and
Yes, its possible. Per my first comment I think @millzpaugh and @alliecrevier should benchmark and profile with |
👌 thanks for pointing that out. Which application has this usecase? (also, perhaps better to discuss this at /vtzero I realize) |
Wow @springmeyer, thank you for such clear documentation throughout your debugging process. @alliecrevier and I will pull two random samples of compositing traffic from the elb logs to run again and confirm that |
We are sticking with our current compression library gzip-hpp. Going to close this issue and reopen if we want to optimize feature field ordering + compression. |
👌 Good call to stick with the current for now. I will say however that I have a strong hunch that a speedup awaits us if we can dig into either changing the field order or moving to faster zlib implementation. But we can handle that in other tickets at a future time. |
@millzpaugh is seeing slower results in staging benchmarking, so I'm concerned that some of the vtcomposite code may not be faster than node-mapnik.
So I just ran the latest benchmarks on linux (a stageyprod machine) using vtcomposite @ 695dd3d (with the fixes to the bench in #77).
I'm able to consistently replicate slower results with
vtcomposite
thannode-mapnik
for 3 bench rules using this command:node-mapnik
vtcomposite
/cc @artemp @alliecrevier @millzpaugh @joto @ericfischer
The text was updated successfully, but these errors were encountered: