Handle negative varint #56

kjvalencik · 2016-07-15T21:38:17Z

Resolves #55

kjvalencik · 2016-07-15T21:39:16Z

index.js

@@ -374,30 +375,73 @@ Pbf.prototype = {
    }
 };

-function readVarintRemainder(val, pbf) {
-    var buf = pbf.buf, b;
+function readVarintRemainder(l, u, p) {


This function uses single character variables names to keep it under 600 characters. Longer functions are not inlined by v8, which drastically decreases performance.

kjvalencik · 2016-07-18T13:35:12Z

test/pbf.test.js

@@ -74,12 +76,12 @@ test('readVarint64', function(t) {
 test('readVarint & writeVarint handle really big numbers', function(t) {
    var buf = new Pbf();
    var bigNum1 = Math.pow(2, 60);
-    var bigNum2 = Math.pow(2, 69);


This is not a valid number to serialize in varint. The 64th bit on a signed integer is the sign bit. If we try to use the full 10th bytes, there is no way to distinguish the sign bit from the rest of the number.

kjvalencik · 2016-07-18T13:38:15Z

Unfortunately, binary literals aren't added until ES2015, which, IMHO, would make this easier to read / understand. Any thoughts on using constants as a pseudo-binary literal? E.g.,

// Instead of 0x80
var b10000000 = parseInt('10000000', 2);

mourner · 2016-07-22T15:17:42Z

Whoa. I somehow missed all the notifications for this PR, sorry for a late reply!

Since this is a substantial change, it will probably affect benchmark numbers — how do those look? Also, how much of a breaking change the isUnsigned parameter is? Should we worry about it, or try to add the change without changing the public API (e.g. with additional methods)?

kjvalencik · 2016-07-22T15:23:45Z

The isUnsigned value parameter isn't very important unless you are trying to encode uint64 that are greater than 2^63.

This did seem to affect the benchmarks somewhat. I think it may be because of the larger functions.

# Old
decode vector tile with pbf x 902 ops/sec ±1.47% (87 runs sampled)
encode vector tile with pbf x 589 ops/sec ±1.70% (84 runs sampled)

# New
decode vector tile with pbf x 875 ops/sec ±1.07% (84 runs sampled)
encode vector tile with pbf x 515 ops/sec ±1.38% (85 runs sampled)

I'm not sure on the precise impacts because the benchmarks are very noisy on my machine--possibly due to other processes running.

mourner · 2016-07-22T15:31:11Z

trying to encode uint64 that are greater than 2^63.

@kjvalencik interesting! Are you relying on this in your use case? I wonder whether it's something that could justify a 13% write performance drop, or something that we could document as a limitation instead.

kjvalencik · 2016-07-22T15:40:08Z

I'm not relying on it anywhere. Since we are always going to a 64-bit float anyway, having that large of a value would result most likely result in loss of precision anyway.

However, my use case does depend on putting negative numbers in varint fields. If I have some time, I'll try tweaking the code and see if I can determine where the slowdown is occurring. Even if the code is more verbose, there might be some shortcuts for smaller numbers.

mourner · 2016-07-22T15:46:38Z

Can we drop the isUnsigned parameter from the public API while retaining the fix then?

kjvalencik · 2016-07-22T16:05:40Z

I pushed a version without the isUnsigned parameter at kjvalencik@8ea9edb

Interestingly, it must have been something about handling unsigned that was causing the slowdown. After removing it, the new benchmarks are somewhat faster than before.

decode vector tile with pbf x 920 ops/sec ±1.16% (86 runs sampled)
encode vector tile with pbf x 615 ops/sec ±1.28% (83 runs sampled

If you would like to go forward with that, I will add some documentation in the README and update this PR. Thanks!

kjvalencik · 2016-07-22T16:11:58Z

index.js

+        }
+    }
+
+    if (toNum(low, high, true) < Math.abs(val)) {


Narrowed it down to this line causing the performance issue. Changing this to the following fixes the speed regression.

if (val >= 0x8000000000000000 || val < -0x8000000000000000) { throw new Error('Given varint doesn\'t fit into 10 bytes'); }

decode vector tile with pbf x 920 ops/sec ±1.04% (89 runs sampled) encode vector tile with pbf x 613 ops/sec ±1.11% (83 runs sampled

mourner · 2016-07-22T16:27:51Z

@kjvalencik fantastic! Let's clean up, rebase and it's good to merge. I wonder what caused the speedup though — the vector tile in the bench shouldn't have many big varints if at all.

kjvalencik · 2016-07-22T21:40:51Z

@mourner I was thinking about backwards compatibility. Even if I remove the isUnsigned flag, it breaks backwards compatibility because previously all values were treated as unsigned.

Instead, what do you think about flipping it to isSigned. That way, if the value isn't provided, it will be false and the API doesn't change. But, it's still available for new built files to take advantage of.

kjvalencik · 2016-07-25T17:09:10Z

@mourner Flipped isUnsigned to isSigned so that the API is backwards compatible.

mourner · 2016-08-25T12:58:11Z

compile.js

@@ -120,9 +120,9 @@ function compileFieldRead(ctx, field) {
    case 'bool':     return prefix + 'Boolean()';
    case 'enum':
    case 'uint32':
-    case 'uint64':
+    case 'uint64':   return prefix + 'Varint(false)';


Should we just use readVarint() for more compact code, or will it affect performance?

I'm not aware of any performance cost of coercing to boolean. 👍

mourner · 2016-08-25T13:21:19Z

Can you rebase on master please?

kjvalencik · 2016-08-25T13:27:26Z

Yes, but I won't have a chance until Monday. Thanks!

mourner · 2016-08-25T13:46:14Z

-> #60

kjvalencik reviewed Jul 15, 2016
View reviewed changes

kjvalencik force-pushed the neg-varint branch from 6207206 to ca216d7 Compare July 18, 2016 13:33

kjvalencik reviewed Jul 18, 2016
View reviewed changes

kjvalencik reviewed Jul 22, 2016
View reviewed changes

Handle negative varint

4f91920

kjvalencik force-pushed the neg-varint branch from ca216d7 to 4f91920 Compare July 25, 2016 17:08

mourner reviewed Aug 25, 2016
View reviewed changes

mourner mentioned this pull request Aug 25, 2016

Handle negative varint #60

Merged

mourner closed this Aug 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle negative varint #56

Handle negative varint #56

kjvalencik commented Jul 15, 2016

kjvalencik Jul 15, 2016 •

edited

Loading

kjvalencik Jul 18, 2016

kjvalencik commented Jul 18, 2016

mourner commented Jul 22, 2016

kjvalencik commented Jul 22, 2016 •

edited

Loading

mourner commented Jul 22, 2016

kjvalencik commented Jul 22, 2016

mourner commented Jul 22, 2016

kjvalencik commented Jul 22, 2016

kjvalencik Jul 22, 2016

mourner commented Jul 22, 2016

kjvalencik commented Jul 22, 2016

kjvalencik commented Jul 25, 2016

mourner Aug 25, 2016

kjvalencik Aug 25, 2016

mourner commented Aug 25, 2016

kjvalencik commented Aug 25, 2016

mourner commented Aug 25, 2016

Handle negative varint #56

Handle negative varint #56

Conversation

kjvalencik commented Jul 15, 2016

kjvalencik Jul 15, 2016 • edited Loading

Choose a reason for hiding this comment

kjvalencik Jul 18, 2016

Choose a reason for hiding this comment

kjvalencik commented Jul 18, 2016

mourner commented Jul 22, 2016

kjvalencik commented Jul 22, 2016 • edited Loading

mourner commented Jul 22, 2016

kjvalencik commented Jul 22, 2016

mourner commented Jul 22, 2016

kjvalencik commented Jul 22, 2016

kjvalencik Jul 22, 2016

Choose a reason for hiding this comment

mourner commented Jul 22, 2016

kjvalencik commented Jul 22, 2016

kjvalencik commented Jul 25, 2016

mourner Aug 25, 2016

Choose a reason for hiding this comment

kjvalencik Aug 25, 2016

Choose a reason for hiding this comment

mourner commented Aug 25, 2016

kjvalencik commented Aug 25, 2016

mourner commented Aug 25, 2016

kjvalencik Jul 15, 2016 •

edited

Loading

kjvalencik commented Jul 22, 2016 •

edited

Loading