http2, async-wrap: introduce AliasedBuffer class #15077

mike-kaufman · 2017-08-29T15:42:15Z

This change introduces an AliasedBuffer class and updates async-wrap
and http2 to use this class.

A common technique to optimize performance is to create a native buffer
and then map that native buffer to user space via JS array. The runtime
can efficiently write to the native buffer without having to route
though JS, and the values being written are accessible from user space.

While efficient, this technique allows modifications to user
space memory w/out going through JS type system APIs, effectively
bypassing any monitoring the JS VM has in place to track program state
modifications. The result is that monitors have an incorrect view
of program state.

The AliasedBuffer class provides a future placeholder where this
technique can be used, but writes can still be observed. To achieve
this, the node-chakra-core fork will add in appropriate tracking logic
in the AliasedBuffer's SetValue() method. Going forward, this class can
evolve to support more sophisticated mechanisms if necessary.

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included (tests added, happy to run benchmarks, just lmk)
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

async-wrap, http2

mike-kaufman · 2017-08-29T15:46:27Z

/cc @jasnell, @addaleax, @mrkmarron.

mike-kaufman · 2017-08-29T15:47:33Z

src/aliased_buffer.h

+   */
+  inline void SetValue(const int index, NativeT value) {
+    // if (index >= count_) {
+    // TODO(@mike-kaufman): Assert?  Fail fast?


Wasn't clear if I should do anything here. Please let me know if there's existing precedent around such checks.

CHECK is likely appropriate. /cc @bnoordhuis

added a CHECK.

mike-kaufman · 2017-08-29T15:47:45Z

src/aliased_buffer.h

+   */
+  inline const NativeT GetValue(const int index) const {
+    // if (index >= count_) {
+    // TODO(@mike-kaufman): Assert?  Fail fast?


similiar here...

fixed now with a CHECK.

mike-kaufman · 2017-08-29T15:48:27Z

src/aliased_buffer.h

+      free(this->buffer_);
+    }
+    // TODO(@mike-kaufman):  what do we do about freeing the
+    // v8::Global<ArrayBuffer> & the v8::Global<U>


not clear to me if I need to do anything to manage lifecycle of a v8::Global? Do I need to call reset()?

That I'm unsure about. @addaleax or @bnoordhuis may know.

A Global<> does need to be reset.

fixed. calling .Reset() on the v8::Global.

mike-kaufman · 2017-08-29T15:53:41Z

src/node_http2_state.h

@@ -0,0 +1,116 @@
+#ifndef SRC_NODE_HTTP2_STATE_H_


I had to move most of the stuff here out of node_http2.cc because http2_state was forward declaration, and I was seeing a compiler warning about calling delete on a forward declaration. I initially tried to introduce a #include "node_http2.h" into env.h, but that was causing cycles.

mike-kaufman · 2017-08-29T15:56:07Z

test/cctest/test_aliased_buffer.cc

+    // v8::MaybeLocal<v8::Number> v3 = v2->ToNumber();
+    // v8::Local<v8::Number> v4 = v3.ToLocalChecked();
+    // NativeT actualValue = *(reinterpret_cast<NativeT*>(&v4->Value()));
+    // EXPECT_TRUE((NativeT)(v4->Value()) == oracle[i]);


I want to validate the value being read from the JS api is the same as the native type value, but I couldn't get this to work. Is there an easy way to do this? Some casting magic I need to do?

Not that I'm aware of.

jasnell · 2017-08-29T16:35:48Z

src/aliased_buffer.h

+
+    // allocate native buffer
+    this->buffer_ = reinterpret_cast<NativeT*>(
+      calloc(count, sizeof(NativeT)));


Could this follow the same pattern as MaybeStackBuffer... or even make use of that to avoid the calloc where possible?

I could, but I'm not following why. Can you help me understand your concerns w/ using calloc? I was trying to be consistent w/ original buffers that were alloc'd on heap.

Consistency would be the main reason but it's certainly not critical. I don't think the performance difference is going to be enough to worry about.

I'm now using node::UncheckedCalloc. Is using MaybeStackBuffer still something to investigate?

jasnell · 2017-08-29T16:40:44Z

Very good. I'll give it a thorough review and sign off later but I'd like to get @addaleax, @bnoordhuis and @trevnorris to take a look also

mscdex · 2017-08-29T19:15:29Z

What kind of performance impact does this have?

mike-kaufman · 2017-08-29T21:49:50Z

What kind of performance impact does this have?

@mscdex - what tests would you suggest I run to measure this?

I ran acmeair-nodejs benchmark, comparing my local master (at this commit) with my changes.

Results didn't show any meaningful impact:

master:

    "numRequests": 533758,
    "elapsedTime": 209787,
    "requestsPerSecond": 2544.2853942331985,
    "averageLatency": 9.682236144469966,

with my change:

    "numRequests": 536971,
    "elapsedTime": 209800,
    "requestsPerSecond": 2559.4423260247854,
    "averageLatency": 9.625374554677999,

Note that intention with this change was to have the GetValue()/SetValue() methods inline and effectively result in 0/minimal overhead vs previous code (i.e., directly accessing offsets in the native buffer). Now, the use the overloaded operator [] may add some overhead here (it is a bit more complex).

I'm happy to run a better set of tests to measure impact here, I just don't know what they are.

jasnell · 2017-08-29T21:52:02Z

@mscdex ... with regards to performance, it's important just to note that this is largely just a formalization of a pattern we were already using. I wouldn't expect the performance to be much different than what we see currently, tho I do suspect the likelihood of bugs to go way down ;-)

trevnorris

The patch doesn't follow general whitespace and line break rules that the remainder of core follows. I haven't run the linter to see if they violate the current rules, but regardless they should align with what node already uses.

Overall I'm -1 on this change. The abstraction adds more code than it removes, and it substantially adds more complexity. This isn't a pattern that we need to use much, and I'm willing to bet that it's one we won't need to use much in the future.

trevnorris · 2017-08-30T07:07:23Z

src/aliased_buffer.h

+    freeBuffer_(true) {
+    const v8::HandleScope handle_scope(this->isolate_);
+
+    const int sizeInBytes = sizeof(NativeT) * count;


sizeof() returns a size_t.

trevnorris · 2017-08-30T07:12:03Z

src/aliased_buffer.h

+
+    // allocate native buffer
+    this->buffer_ = reinterpret_cast<NativeT*>(
+      calloc(count, sizeof(NativeT)));


Two problems here. 1) please use node::UncheckedCalloc() instead of using calloc() directly. 2) You're not checking if this->buffer_ == nullptr afterwards.

Thanks @trevnorris, will do. Is there an example of fail-fast behavior when out of memory occurs? e.g., is there a specific error code to return?

Good catch. I tend to completely forget that we have those Calloc wrappers there.

trevnorris · 2017-08-30T07:20:31Z

src/aliased_buffer.h

+    v8::Uint8Array>& backingBuffer) :
+    isolate_(isolate),
+    count_(count),
+    byte_offset_(byte_offset),


A CHECK() to verify that byte_offset_ is aligned would be helpful.

Fwiw, that check isn't strictly necessary because the V8 TypedArray constructor checks it anyway

trevnorris · 2017-08-30T07:25:46Z

src/aliased_buffer.h

+      free(this->buffer_);
+    }
+    // TODO(@mike-kaufman):  what do we do about freeing the
+    // v8::Global<ArrayBuffer> & the v8::Global<U>


A Global<> does need to be reset.

jasnell · 2017-08-30T16:59:01Z

@trevnorris... see here for some of the background leading into this: nodejs/node-chakracore#372

The short version is, the way the code is written now, it causes some headaches for the Chakra folks that this revised method is intended to ease. Given the problem, I think this is the best way to address it but I'd certainly love to have you review and comment on that! :-)

mike-kaufman · 2017-08-30T18:55:12Z

@trevnorris -

The patch doesn't follow general whitespace and line break rules that the remainder of core follows.

Linter is clean on osx for me. Can you point out specifics? Or is there a style guide I'm missing?

Overall I'm -1 on this change. The abstraction adds more code than it removes, and it substantially adds more complexity.

The key thing here is this simplifies cases where we need to observe writes to that program state. e.g., take a look at https://github.com/nodejs/node-chakracore/pull/252/files. The code here to "mark" these writes is scattered around in a way that is prone to breakage & merge conflicts. The AliasedBuffer change lets us consolidate this at a single point in a maintainable/sane way.

Happy to discuss other approaches here if you have ideas.

jasnell

LGTM, would like to make sure @addaleax and @trevnorris are good on it tho

addaleax

Looks good, left a few style nits that would bring this a little more in line with our usual style, but nothing blocking :)

addaleax · 2017-08-31T13:06:54Z

src/aliased_buffer.h

+    this->buffer_ = UncheckedCalloc<NativeT>(count);
+    if (this->buffer_ == nullptr) {
+      ABORT();
+    }


At this point you might as well use the checked kind of alloc? ;)

addaleax · 2017-08-31T13:07:25Z

src/aliased_buffer.h

+
+    // allocate v8 ArrayBuffer
+    v8::Local<v8::ArrayBuffer> ab = v8::ArrayBuffer::New(
+      this->isolate_, this->buffer_, sizeInBytes);


tiny style nit: 4 spaces indentation for statement continuations (and we don't usually do this->)

added 4 spaces for continuations & removed this-> qualifiers.

addaleax · 2017-08-31T13:08:19Z

src/aliased_buffer.h

+
+    // allocate v8 TypedArray
+    v8::Local<V8T> jsArray = V8T::New(ab, this->byte_offset_, count);
+    this->jsArray_ = v8::Global<V8T>(isolate, jsArray);


tiny style nit: snake_case (js_array, js_array_) for variables/member fields

snake cased vars & memebers. LMK if I missed anything.

addaleax · 2017-08-31T13:09:06Z

src/aliased_buffer.h

+    v8::Uint8Array>& backingBuffer) :
+    isolate_(isolate),
+    count_(count),
+    byte_offset_(byte_offset),


Fwiw, that check isn't strictly necessary because the V8 TypedArray constructor checks it anyway

addaleax · 2017-08-31T13:11:36Z

src/aliased_buffer.h

+      return *this;
+    }
+
+    operator NativeT() {


This can be const, fwiw

addaleax · 2017-08-31T13:16:24Z

src/env-inl.h

@@ -457,7 +462,8 @@ inline double Environment::trigger_id() {
 }

 inline double Environment::get_init_trigger_id() {
-  double* uid_fields = async_hooks()->uid_fields();
+  AliasedBuffer<double, v8::Float64Array> uid_fields =
+    async_hooks()->uid_fields();


You could use a reference (AliasedBuffer<double, v8::Float64Array>&) instead, right? I think that would allow you to get rid of AliasedBuffer's copy constructor (and mark that as deleted), which seems like a good idea to me, I think

yeah, you're right. I'll fix...

addaleax · 2017-08-31T13:17:20Z

src/node_http2.cc

@@ -149,7 +93,8 @@ ssize_t Http2Session::OnCallbackPadding(size_t frameLen,
  Context::Scope context_scope(context);

  if (object()->Has(context, env()->ongetpadding_string()).FromJust()) {
-    uint32_t* buffer = env()->http2_state_buffer()->padding_buffer;
+    AliasedBuffer<uint32_t, v8::Uint32Array> buffer =
+      env()->http2_state()->padding_buffer;


All of these could be references as well, right?

addaleax · 2017-08-31T13:19:48Z

test/cctest/test_aliased_buffer.cc

+  // read through the API
+  for (size_t i = 0; i < size; i++) {
+    NativeT v1 = (NativeT)aliasedBuffer[i];
+    NativeT v2 = (NativeT)aliasedBuffer.GetValue(i);


style nit: Can you use static_cast for these?

removed cast, wasn't necessary.

addaleax · 2017-08-31T13:21:18Z

test/cctest/test_aliased_buffer.cc

+    // NativeT actualValue = *(reinterpret_cast<NativeT*>(&v4->Value()));
+    // EXPECT_TRUE((NativeT)(v4->Value()) == oracle[i]);
+    // TODO(@mike-kaufman)  figure out how to compare the value in the v8 array
+    // Expect_TRUE(*v.ToLocal() == v8NumberCreateFunction(oracle[i]);


I am not sure what this code would mean, if I'm honest. Is this code for checking object identity between number values?

I want to validate that the value read through JS APIs was the same value as what was written to the native array. I just never found the right set of APIs and/or casts to make this happen. Was hoping there was something straightforward that I was missing.

addaleax · 2017-08-31T13:22:21Z

test/cctest/test_aliased_buffer.cc

+    EXPECT_TRUE(v2->IsNumber());
+    // v8::MaybeLocal<v8::Number> v3 = v2->ToNumber();
+    // v8::Local<v8::Number> v4 = v3.ToLocalChecked();
+    // NativeT actualValue = *(reinterpret_cast<NativeT*>(&v4->Value()));


I don't think this could work, you're taking a pointer to a stack-allocated return value and casting it around`.

NativeT actualValue = static_cast<NativeT>(&v4->Value());?

I'll play around with that...

OK, got this to work. Think I was seeing seg fault earlier because I didn't pass context to ToNumber

addaleax · 2017-08-31T13:25:11Z

Overall I'm -1 on this change. The abstraction adds more code than it removes, and it substantially adds more complexity. This isn't a pattern that we need to use much, and I'm willing to bet that it's one we won't need to use much in the future.

@trevnorris This pattern solves a real problem, so if you have other solutions, feel free to voice them (but not doing so is kind of unproductive). Also, I would argue that the encapsulation that comes with this does remove complexity; it makes it more obvious to readers that a write to these ArrayBuffers is not just a memory write, but actually changes state for JS.

mike-kaufman · 2017-08-31T17:53:50Z

Thanks @addaleax for the review! I think I addressed all the feedback & rebased off of master. LMK if I missed anything. I didn't squash in case anyone wants to look at diffs, but I can do that.

jasnell · 2017-08-31T18:18:45Z

@trevnorris ... I absolutely have to agree with @addaleax on this. This addresses a real problem even if we don't use this pattern extensively. Please reconsider your -1

addaleax · 2017-09-03T16:43:31Z

@mike-kaufman Just occurred to me that a proper solution for the delete http2_state; problem might be to move the Environment constructor and destructor definition out of the header file and into env.cc, if you want to explore that. (You don’t need to, since you already figured out sth here)

jasnell · 2017-09-05T17:03:41Z

ping @trevnorris

addaleax · 2017-09-05T23:23:06Z

src/aliased_buffer.h

+   *  Get value at position index
+   */
+  inline const NativeT GetValue(const size_t index) const {
+    CHECK_LT(index, count_);


@trevnorris I’m pretty sure the reported performance difference is due to this check, it’s the only visible difference in the generated machine code. I’d suggest wrapping it in #if defined(DEBUG) && DEBUG?

confirmed. that is where the performance hit is coming from. wrapping it sounds good

this and the SetValue check are now in debug-only builds.

addaleax · 2017-09-05T23:28:16Z

src/aliased_buffer.h

+   *  Set position index to given value.
+   */
+  inline void SetValue(const size_t index, NativeT value) {
+    CHECK_LT(index, count_);


(same goes for this one, I assume)

mike-kaufman · 2017-09-10T17:43:53Z

Sorry for the delayed response here, I've been off the grid for the last week or so. I'll address latest feedback shortly.

@trevnorris -

Can you elaborate on how this "future placeholder" will be used? I don't see how this PR does anything to help notification of state change when JS writes to memory.

Short term, in the chakra fork, we'll add some logic in the SetValue method that will track anytime this state is modified. Long term is TBD. If need for something more general arises, you could have an API that allows callbacks to be registered that will get fired every time SetValue is called. Irrespective of the details, with this change there's now a consolidated place where these writes occur, namely SetValue.

mike-kaufman · 2017-09-10T19:13:21Z

I think I addressed the latest feedback. Please let me know if I missed anything.

@jasnell -

How about we separate concerns here. Let's pull the AsyncWrap related changes out of this PR and ,move them into a separate PR.

I'm happy to do this, but I feel like we're close here? LMK if not the case.

jasnell · 2017-09-12T10:26:35Z

As long as @trevnorris and @addaleax are happy, I'm good.

trevnorris · 2017-09-12T22:50:14Z

src/aliased_buffer.h

+    buffer_ = reinterpret_cast<NativeT*>(
+        const_cast<uint8_t*>(backing_buffer.GetNativeBuffer() + byte_offset));
+
+    v8::Local<v8::ArrayBuffer> ab = backing_buffer.GetArrayBuffer();


would it be worth it to assign backing_buffer.GetNativeBuffer() to a variable at the top, since it's used three times here? (not a blocker)

fixed. (assuming you meant GetArrayBuffer()).

trevnorris · 2017-09-12T22:57:24Z

src/aliased_buffer.h

+  inline const NativeT GetValue(const size_t index) const {
+    #if defined(DEBUG) && DEBUG
+      CHECK_LT(index, count_);
+    #endif


nit (non-blocking): more common indentation is to have no whitespace before the #if.

trevnorris

two questions/suggestions, but nothing technically blocking. LGTM.

This change introduces an AliasedBuffer class and updates asytnc-wrap and http2 to use this class. A common technique to optimize performance is to create a native buffer and then map that native buffer to user space via JS array. The runtime can efficiently write to the native buffer without having to route though JS, and the values being written are accessible from user space. While efficient, this technique allows modifications to user space memory w/out going through JS type system APIs, effectively bypassing any monitoring the JS VM has in place to track program state modifications. The result is that monitors have an incorrect view of prorgram state. The AliasedBuffer class provides a future placeholder where this technique can be used, but writes can still be observed. To achieve this, the node-chakra-core fork will add in appropriate tracking logic in the AliasedBuffer's SetValue() method. Going forward, this class can evolve to support more sophisticated mechanisms if necessary.

…r/variable names, adding const, some other stuff...

addaleax · 2017-09-13T09:07:17Z

Fresh CI: https://ci.nodejs.org/job/node-test-commit/12329/

This should be ready.

jasnell · 2017-09-13T15:34:01Z

Still LGTM

mike-kaufman · 2017-09-13T15:36:49Z

Great! Thanks @addaleax, @trevnorris, @jasnell! Anything else I need to do here? When can this get merged in?

jasnell · 2017-09-13T15:37:38Z

If someone else doesn't beat me to it, I'll be merging it in later on this morning.

addaleax · 2017-09-13T15:44:45Z

Landed in 35a526c

This change introduces an AliasedBuffer class and updates asytnc-wrap and http2 to use this class. A common technique to optimize performance is to create a native buffer and then map that native buffer to user space via JS array. The runtime can efficiently write to the native buffer without having to route though JS, and the values being written are accessible from user space. While efficient, this technique allows modifications to user space memory w/out going through JS type system APIs, effectively bypassing any monitoring the JS VM has in place to track program state modifications. The result is that monitors have an incorrect view of prorgram state. The AliasedBuffer class provides a future placeholder where this technique can be used, but writes can still be observed. To achieve this, the node-chakra-core fork will add in appropriate tracking logic in the AliasedBuffer's SetValue() method. Going forward, this class can evolve to support more sophisticated mechanisms if necessary. PR-URL: #15077 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This change introduces an AliasedBuffer class and updates asytnc-wrap and http2 to use this class. A common technique to optimize performance is to create a native buffer and then map that native buffer to user space via JS array. The runtime can efficiently write to the native buffer without having to route though JS, and the values being written are accessible from user space. While efficient, this technique allows modifications to user space memory w/out going through JS type system APIs, effectively bypassing any monitoring the JS VM has in place to track program state modifications. The result is that monitors have an incorrect view of prorgram state. The AliasedBuffer class provides a future placeholder where this technique can be used, but writes can still be observed. To achieve this, the node-chakra-core fork will add in appropriate tracking logic in the AliasedBuffer's SetValue() method. Going forward, this class can evolve to support more sophisticated mechanisms if necessary. PR-URL: nodejs/node#15077 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This change introduces an AliasedBuffer class and updates asytnc-wrap and http2 to use this class. A common technique to optimize performance is to create a native buffer and then map that native buffer to user space via JS array. The runtime can efficiently write to the native buffer without having to route though JS, and the values being written are accessible from user space. While efficient, this technique allows modifications to user space memory w/out going through JS type system APIs, effectively bypassing any monitoring the JS VM has in place to track program state modifications. The result is that monitors have an incorrect view of prorgram state. The AliasedBuffer class provides a future placeholder where this technique can be used, but writes can still be observed. To achieve this, the node-chakra-core fork will add in appropriate tracking logic in the AliasedBuffer's SetValue() method. Going forward, this class can evolve to support more sophisticated mechanisms if necessary. PR-URL: #15077 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This change introduces an AliasedBuffer class and updates asytnc-wrap and http2 to use this class. A common technique to optimize performance is to create a native buffer and then map that native buffer to user space via JS array. The runtime can efficiently write to the native buffer without having to route though JS, and the values being written are accessible from user space. While efficient, this technique allows modifications to user space memory w/out going through JS type system APIs, effectively bypassing any monitoring the JS VM has in place to track program state modifications. The result is that monitors have an incorrect view of prorgram state. The AliasedBuffer class provides a future placeholder where this technique can be used, but writes can still be observed. To achieve this, the node-chakra-core fork will add in appropriate tracking logic in the AliasedBuffer's SetValue() method. Going forward, this class can evolve to support more sophisticated mechanisms if necessary. PR-URL: nodejs/node#15077 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 29, 2017

mike-kaufman mentioned this pull request Aug 29, 2017

HTTP2 functionality requires changes to work with TTD nodejs/node-chakracore#372

Open

mike-kaufman commented Aug 29, 2017

View reviewed changes

jasnell reviewed Aug 29, 2017

View reviewed changes

mscdex added async_wrap http2 Issues or PRs related to the http2 subsystem. labels Aug 29, 2017

mike-kaufman force-pushed the mkaufman-fixup-aliased-arrays branch from 2684ac6 to 7d70f50 Compare August 29, 2017 21:51

trevnorris suggested changes Aug 30, 2017

View reviewed changes

mike-kaufman force-pushed the mkaufman-fixup-aliased-arrays branch from 7d70f50 to 7b33315 Compare August 30, 2017 15:13

mike-kaufman force-pushed the mkaufman-fixup-aliased-arrays branch from 7b33315 to f28aa61 Compare August 30, 2017 18:34

jasnell approved these changes Aug 30, 2017

View reviewed changes

addaleax approved these changes Aug 31, 2017

View reviewed changes

mike-kaufman force-pushed the mkaufman-fixup-aliased-arrays branch from d9ca7f4 to 19fa7b6 Compare August 31, 2017 17:49

addaleax reviewed Sep 5, 2017

View reviewed changes

mike-kaufman force-pushed the mkaufman-fixup-aliased-arrays branch from e4c2824 to 3d9f323 Compare September 10, 2017 19:02

trevnorris reviewed Sep 12, 2017

View reviewed changes

trevnorris approved these changes Sep 12, 2017

View reviewed changes

Mike Kaufman and others added 4 commits September 12, 2017 17:03

responding to PR feedback: cleaning up whitespace, snake-casing membe…

a47238b

…r/variable names, adding const, some other stuff...

responding to PR feedback and fixing up whitespace/formatting

d06a833

respnoding to PR feedback

c6508f7

mike-kaufman force-pushed the mkaufman-fixup-aliased-arrays branch from 3d9f323 to c6508f7 Compare September 13, 2017 00:58

addaleax closed this Sep 13, 2017

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

addaleax mentioned this pull request Oct 18, 2017

src: operator[] checks bounds in debug mode #16002

Closed

2 tasks

jasnell mentioned this pull request Jan 9, 2018

[Tracking] backporting current http2 to v8.x #18068

Closed

4 tasks

mrkmarron mentioned this pull request Feb 15, 2018

Time-Travel diagnostics in Node nodejs/diagnostics#164

Closed

joyeecheung mentioned this pull request Nov 30, 2018

doc: document AliasedBuffers usage scope in style guide #24724

Closed

4 tasks

http2, async-wrap: introduce AliasedBuffer class #15077

http2, async-wrap: introduce AliasedBuffer class #15077

Conversation

mike-kaufman commented Aug 29, 2017

Affected core subsystem(s)

mike-kaufman commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Aug 29, 2017

mscdex commented Aug 29, 2017

mike-kaufman commented Aug 29, 2017

jasnell commented Aug 29, 2017

trevnorris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Aug 30, 2017

mike-kaufman commented Aug 30, 2017

jasnell left a comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Aug 31, 2017

mike-kaufman commented Aug 31, 2017

jasnell commented Aug 31, 2017

addaleax commented Sep 3, 2017

jasnell commented Sep 5, 2017

addaleax Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-kaufman commented Sep 10, 2017

mike-kaufman commented Sep 10, 2017

jasnell commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Sep 5, 2017 •

edited

Loading

addaleax commented Sep 13, 2017 •

edited

Loading