src: use DataView for B.{read,write}{Float,Double}

[skomski]

Adds lazy-initialized dataView property to Buffer
Removes C++ functions: ReadFloat, WriteFloat, ReadDouble, WriteDouble
About 37,5% faster

[bnoordhuis]

I have to say I'm not really a fan of how this adds a new property to the Buffer prototype.

[targos]

Results of buffer benchmarks on current master before and after applying this patch: https://gist.github.com/targos/438a9cc9dc48fa178729

[Fishrock123]

cc @trevnorris

[skomski]

@bnoordhuis performance or api burden? Because I did not plan to document this property and I can rename to make it more clear.

@targos My patch can't be only difference in your benchmark. This patch could only have a big impact for float and double related tests or insignificant slow down for example buffer-creation but cannot improve buffer-iterate.

[targos]

@skomski you are right, I ran the before with current v4 instead of master...
Gist updated with much more meaningful numbers.

[domenic]

Should be configurable and probably enumerable

[targos]

What about this:

Object.defineProperty(Buffer.prototype, 'dataView', {
  get: function() {
    return this.dataView = new DataView(this.buffer, this.byteOffset, this.byteLength);
  },
  configurable: true,
  enumerable: true
});

[domenic]

You can't set something that only has a getter. The current body of the getter is fine.

[targos]

Right, my bad. So the body should call Object.defineProperty(this, 'dataView', { value: ... });
I don't like the fact that we need two properties (dataView and _dataView) for one.

[Fishrock123]

I'm with @targos here. We do that some other places too.

[domenic]

I like adding a dataView property to the prototype, from an API perspective. But it should be documented. If it's not going to be documented then just use the _dataView property directly.

[trevnorris]

Let me investigate why this is faster. The v8 API should be comparable in terms of performance.

[trevnorris]

There are several extra checks that don't need to be done in the current implementation. A quick refactor removed the performance difference between the two. This would be the better approach.

@skomski If you'd like to take this on, I can point you at which checks need to be changed/removed. Otherwise I can throw up a PR. Thanks for investigating the performance here. In the last refactor I went overly paranoid w/ checking to make sure nothing slipped through the cracks.

[domenic]

I think there's independent value in (a) adding an easy dataView accessor; (b) removing all that C++ code in favor of using the stuff V8 already has. So this still seems like a good change to me...

[ChALkeR]

@trevnorris

1. Is there any speed profit in keeping the (fixed) c++ implementation?
2. Are there any compatibility problems here?

[trevnorris]

I don't believe it's a good idea to mutate the object (i.e. add property after instantiation). Which would in turn mutate the ObjectMap. Everything else in Buffer is either attached to the prototype or is statically assigned.

In terms of performance, difference between the two is within the margin of error. Our implementation is slightly faster if noAssert == true.

In terms of compatibility difference, noAssert is now pointless. Before it would bypass all checks and abort if a value was incorrect. Now it will always throw regardless.

The first can be addressed by setting _dataView = null right after the Uint8Array() is constructed. I don't see any noticeable difference in construction time doing this. Except this won't work for Buffers created from C++ (e.g. incoming data from I/O).

Setting object property after instantiation is my concern. Can anyone think of a way to get around it?

[skomski]

@trevnorris Pushed an update that simply defines the property _dataView so the assignment later doesn't mutate the object property etc.

[trevnorris]

@skomski Issue is as I mentioned:

Except this won't work for Buffers created from C++ (e.g. incoming data from I/O).

Any Buffers created from C++ won't have this property set. So we'll then have two object maps being passed around. Solutions would be to also set the property in C++ (though I have reservations on the performance impact that would have, since JS operations in native are slow) or... Not sure. Have looked for a way to do this without setting a property on the object, but haven't found one.

If it wasn't for needing to store a value on the instantiated Buffer (primarily because the operation needs to be performed in C++ and JS) I'd be +1 for this change. And if someone can find a way I think that would be great. Until then I'm inclined to say the better approach is to remove the overzealous checks. Which would still give comparable performance.

[trevnorris]

Example script to demonstrate my issue:

'use strict';

let b = new Buffer(16);

for (let i = 0; i < 1e4; i++)
  b.write('hi all');

b.readFloatLE(0);

b.write('hi all');

Running this script currently:

$ node --trace-deopt test.js
$

Running script with this PR (before the recent change of placing the DataView on the prototype):

$ node --trace-deopt test.js
[deoptimizing (DEOPT eager): begin 0x2789c2c71419 <JS Function Buffer.write (SharedFunctionInfo 0x2789c2c691b1)> (opt #1) @10, FP to SP delta: 64]
            ;;; deoptimize at 10898: wrong map
  reading input frame Buffer.write => node=5, args=14, height=4; inputs:
      0: 0x2789c2c71419 ; (frame function) 0x2789c2c71419 <JS Function Buffer.write (SharedFunctionInfo 0x2789c2c691b1)>
      1: 0x1a0db60fc229 ; rax 0x1a0db60fc229 <an Uint8Array with map 0x33f0e172db89>
      2: 0x2789c2cb9559 ; [fp + 40] 0x2789c2cb9559 <String[6]: hi all>
      3: 0x3bc1b7e04131 ; [fp + 32] 0x3bc1b7e04131 <undefined>
      4: 0x3bc1b7e04131 ; [fp + 24] 0x3bc1b7e04131 <undefined>
      5: 0x3bc1b7e04131 ; rdx 0x3bc1b7e04131 <undefined>
      6: 0x1a0db6075c41 ; r8 0x1a0db6075c41 <FixedArray[27]>
      7: 0x3bc1b7e04131 ; (literal 1) 0x3bc1b7e04131 <undefined>
      8: 0x3bc1b7e04131 ; (literal 1) 0x3bc1b7e04131 <undefined>
      9: 0x3bc1b7e04131 ; (literal 1) 0x3bc1b7e04131 <undefined>
  translating frame Buffer.write => node=14, height=24
    0x7ffdcbb31870: [top + 88] <- 0x1a0db60fc229 ;  0x1a0db60fc229 <an Uint8Array with map 0x33f0e172db89>  (input #1)
    0x7ffdcbb31868: [top + 80] <- 0x2789c2cb9559 ;  0x2789c2cb9559 <String[6]: hi all>  (input #2)
...

Altering the map will cause a deoptimization. Now, if we could simply immediately set it in JS after creating the Uint8Array I'd be fine with that. But since we also have to do this in C++ things are more complicated. IMO the more straight forward approach is to fix the current checks.

[chrisdickinson]

If you wanted to avoid mutating the object, add a WeakMap to the file and have the getter/setter key the buffer into the weakmap to get the dataview:

var dataViewMap = new WeakMap()
Object.defineProperty(Buffer.prototype, 'dataView', {
  enumerable: true,
  get: function() {
    if (!dataViewMap.has(this)) {
      var dv = new DataView(this.buffer, this.byteOffset, this.byteLength);
      dataViewMap.set(this, dv);
      return dv;
    }
    return dataViewMap.get(this);
  }
});

Unsure what affect this would have on the perf of backing the buffer methods with dataview, but it should solve the mutated object problem.

[trevnorris]

Pending #3080 the time gap between implementations will be around 5ns. There are additional optimizations to do afterwards, but will wait until the PR lands.

[jasnell]

What's the status on this one? @trevnorris any reason to keep this open?

[trevnorris]

I haven't benchmarked recently, but last I checked this was within 10-20 ns of our implementation if there are enough writes to discount the time it takes to create the DataView, and mutating the object and changing the map after construction doesn't feel like a good move. If this can be accomplished in a way that adds so much of a performance gain that it clearly discounts the DEOPT or can be done without changing the object map then I'd say we have an implementation worth considering.

[jasnell]

Noted. @skomski ... is this still something you wish to pursue?

[jasnell]

Closing given the lack of progress