node-chakracore
node-chakracore copied to clipboard
nodejs
HTTP2 functionality requires changes to work with TTD
- Version: master
- Platform: all
- Subsystem: TTD, http2
Some changes to http2 require updates to support TTD properly: the upstream commit is https://github.com/nodejs/node/commit/e46ae99a2a8153d6af84dbf2883f5f0938065a3e and will need similar changes to those in https://github.com/nodejs/node-chakracore/pull/194, or otherwise will require a change in how we handle this scenario.
I spoke with @mrkmarron and he had this to offer:
This aliasing a backing array through a C++ and JS variable just seems like a terrible idea to me and I was hoping that it was a one off optimization. However, it looks like it is spreading so I think we may want to do a bit of work to create an API that we can use to track these cases and which will not create a lot of merge conflicts going forward. E.g. add a few macros for CREATE_ALIASED_BUFFER() and MODIFY_ALIASED_BUFFER() where we can hide the rest of the changes behind them (and maybe even try to upstream as nops).
I certainly agree that the aliasing of the backing array is a bit of a hack but it's an effective one in terms of performance. The speedup is absolutely non-trivial. Just as an FYI, I'm also doing this with a key part of the Performance API implementation. Until we have a better solution with a similar or improve performance profile, it's likely to be something we stick with.
@jasnell thanks for the insight- would Mark's suggestion of creating macros for this pattern be something we could PR upstream?
Hi James, I am sure the perf improvement is non-trivial. The cost of cross-runtime calls is painful is going to be painful on a hot-paths like this.
In the short term my big concerns are (1) adding TTD and managing merge conflicts and (2) general maintainability. As the code is written now there is no encapsulation or indication that a write is happening to a JS aliased array (and which one it might be). As per my previous comment I think a adding some macros on the C++ side for this idiom would help a lot with these issues and simplify later code changes.
In the longer term I think this is a good opportunity to start looking at what a Node.js embedder API would look like. For example in the case of fs.stat there is a similar native/JS shared array for the fs stats data that is directly written into to avoid the cost of multiple cross runtime boundary calls. But even with this optimization the JS layer still needs to unpack each value in the array and assign to a property in a JS object (line 245 in fs.js). Having an API like:
JsCreateFlagObjectFromPropertiesAndValues(size_t propertyCount, JsObject* prototype, ...),
called like:
JsCreateFlagObjectFromPropertiesAndValues(2, statsProto, "mode", modeVal, "uid", uidVal)
would allow all of this to happen in one step in the native code and be great for improved performance, better maintainability, and (for me personally) lower cost/more useful TTD recording.
The possibility of adding a macro makes sense. It could get a bit complicated.
@jasnell I spent some time talking with @mike-kaufman about the macro option. I think we are going to give it a try to see how it seems to work out.
Spent some time looking at this yesterday. The macro solution feels pretty kludgy IMO. What about introducing an AliasedBuffer class to allow access to the raw buffer & the JS Array? All writes to the buffer could be routed through an API, which gives a clean place for TTD-tracking code to sit.
Functions should be inlined so perf impact should be minimal. E.g., a class roughly like the following:
template <class T, class U>
class AliasedBuffer {
public:
AliasedBuffer(const int count);
~AliasedBuffer();
T* GetBuffer();
v8::Local<U> GetJSArray();
// set the value at specified index. Since this is an explicit write, chakra's fork
// can add in code to do TTD-specific tracking. Need to know this is a write
// vs a read, so overloading operator[] won't work AFAIU (let me know if not the case)
void SetValue(const int index, T value);
// get value at specified index.
const T GetValue(const int index);
private:
T* buffer_; // raw buffer that is written to
v8::Global<U> jsArray_; // JS array over the buffer
};
}
So, then a client can have something like
...
private:
AliasedBuffer* fields_;
public:
init() {
fields_ = new AliasedBuffer<double, Float64Array>(kfieldsCount);
}
doSoemthing() {
double status = ...;
fields_->SetValue(kMyStatusFieldIndex, status);
}
@jasnell , @mrkmarron , @digitalinfinity - let me know your thoughts on this approach. Particularly
- any perf concerns?
- any API concerns w/ what I proposed?
- Any pushback with merging such a solution upstream?
- Any gaps in supporting TTD scenarios?
This aliasing a backing array through a C++ and JS variable just seems like a terrible idea to me and I was hoping that it was a one off optimization. However, it looks like it is spreading
Fwiw, async-hooks is another place where this also came up.
@mike-kaufman Your proposed solutions sounds good to me!
so overloading operator[] won't work AFAIU (let me know if not the case)
You could make operator[] work by returning a reference-ish object, similar to what std::bitset::operator[] does, but that’s obviously just cosmetics.
You could make operator[] work by returning a reference-ish object, similar to what std::bitset::operator[] does, but that’s obviously just cosmetics.
Ahh, I see, thanks. My preference is to have separate get/set methods, as it seems simpler, but I'd like to keep things consistent w/ what's in the code base. Let me know if there's anything to suggest one direction or the other.
I think this works also. The important bit is going to be ensuring that these backing buffers can be set per-Environment and that they are persistent over time. I have no perf concerns with this approach. I agree that an operator[] would be helpful, and by convention we have used operator* consistently in core to provide the equivalent of your GetBuffer(). Also common is using data() or Data() to do the same -- but as @addaleax says, that's just cosmetics.
One other consideration is this: In some cases, we have a single TypedArray backed by a single ArrayBuffer backed by a single backing array. In other cases, we have multiple TypedArray instances backed by a single aggregate ArrayBuffer backed by a single backing array. This is done simply because it is more efficient than having multiple ArrayBuffers. It would be nice if this approach accounted for that model as well.
I believe this is easily something that we could get merged into core and would help make our application of this pattern more consistent. Thank you very much for working on this.
we have multiple TypedArray instances backed by a single aggregate ArrayBuffer backed by a single backing array
@jasnell - Can you point me to any examples of this? I didn't see it when I was looking around yesterday.
I agree that an operator[] would be helpful, and by convention we have used operator*
OK, makes sense, i'll overload the operators.
See here: https://github.com/nodejs/node/blob/master/src/node_http2.cc#L1179-L1199
Just to make sure it doesn’t get lost, operator* should only be const, otherwise it kind of circumvents what your approach is trying to achieve :)
@jasnell, @addaleax - thanks for your help. Plz take a look at https://github.com/nodejs/node/pull/15077.