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

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@skomski
Copy link
Contributor

skomski commented Sep 16, 2015

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

@targos targos added the buffer label Sep 16, 2015

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Sep 16, 2015

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

@targos

This comment has been minimized.

Copy link
Member

targos commented Sep 16, 2015

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

@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 commented Sep 16, 2015

@skomski

This comment has been minimized.

Copy link
Contributor Author

skomski commented Sep 16, 2015

@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

This comment has been minimized.

Copy link
Member

targos commented Sep 16, 2015

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

@domenic

View changes

lib/buffer.js Outdated
@@ -295,6 +295,17 @@ function byteLength(string, encoding) {
Buffer.byteLength = byteLength;


Object.defineProperty(Buffer.prototype, 'dataView', {
get: function() {

This comment has been minimized.

@domenic

domenic Sep 16, 2015

Member

Should be configurable and probably enumerable

This comment has been minimized.

@targos

targos Sep 16, 2015

Member

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
});

This comment has been minimized.

@domenic

domenic Sep 16, 2015

Member

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

This comment has been minimized.

@targos

targos Sep 16, 2015

Member

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.

This comment has been minimized.

@Fishrock123

Fishrock123 Sep 16, 2015

Member

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

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Sep 16, 2015

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

This comment has been minimized.

Copy link
Contributor

trevnorris commented Sep 16, 2015

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

@trevnorris

This comment has been minimized.

Copy link
Contributor

trevnorris commented Sep 16, 2015

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

This comment has been minimized.

Copy link
Member

domenic commented Sep 16, 2015

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

This comment has been minimized.

Copy link
Member

ChALkeR commented Sep 16, 2015

@trevnorris

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

This comment has been minimized.

Copy link
Contributor

trevnorris commented Sep 16, 2015

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 skomski force-pushed the skomski:buffer-use-dataview branch Sep 17, 2015

@skomski

This comment has been minimized.

Copy link
Contributor Author

skomski commented Sep 17, 2015

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

@trevnorris

This comment has been minimized.

Copy link
Contributor

trevnorris commented Sep 17, 2015

@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

This comment has been minimized.

Copy link
Contributor

trevnorris commented Sep 17, 2015

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.

src: use DataView for B.{read,write}{Float,Double}
Adds lazy-initialized dataView property to Buffer
Removes C++ functions: ReadFloat, WriteFloat, ReadDouble, WriteDouble
About 37,5% faster

@skomski skomski force-pushed the skomski:buffer-use-dataview branch to 576af3b Sep 18, 2015

@chrisdickinson

This comment has been minimized.

Copy link
Contributor

chrisdickinson commented Sep 29, 2015

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

This comment has been minimized.

Copy link
Contributor

trevnorris commented Sep 29, 2015

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.

@Trott Trott force-pushed the nodejs:master branch to 082cc8d Dec 27, 2015

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Mar 22, 2016

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

@jasnell jasnell added the stalled label Mar 22, 2016

@trevnorris

This comment has been minimized.

Copy link
Contributor

trevnorris commented Mar 23, 2016

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

This comment has been minimized.

Copy link
Member

jasnell commented Mar 27, 2016

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

@rvagg rvagg force-pushed the nodejs:master branch 2 times, most recently to 83c7a88 Oct 18, 2016

@MylesBorins MylesBorins force-pushed the nodejs:master branch to 54fef67 Feb 1, 2017

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Feb 28, 2017

Closing given the lack of progress

@jasnell jasnell closed this Feb 28, 2017

@TimothyGu TimothyGu referenced this pull request Mar 30, 2018

Open

buffer: add {read|write}Big[U]Int64{BE|LE} methods #19691

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment