buffer: add buf.mask for ws #1202

chrisdickinson · 2015-03-19T09:27:56Z

This patch adds buf.mask(maskValue, targetBuffer, targetOffset, sourceStart, sourceEnd), needed by the ws package for masking and unmasking websocket frames. The original implementation is here. It is implemented as a partially unrolled loop, operating on 32-bit values and dealing with the 0-3 byte remainder at the end – I'm not sure if we want to retain this optimization, since the sourceStart and targetOffset params make it possible to trigger non-8-byte-aligned copies.

I didn't implement Unmask: it should be equivalent to buf.mask(mask, buf). Merge is also omitted. I'll have another PR up for Utf8Validate tomorrow.

R=@trevnorris, @piscisaureus

bnoordhuis · 2015-03-19T10:17:10Z

src/node_buffer.cc

+  Environment* env = Environment::GetCurrent(args);
+
+  uint32_t mask32 = args[0]->Uint32Value();
+  char* mask = (char*)&mask32;


Use reinterpret_cast, don't use C-style casts.

I'm not sure if you're aware of this but this would have been pointer aliasing with any other pointer type.

Aliasing with char is okay right? @bnoordhuis

Yes, a pointer to char may alias a pointer to another type (but not the other way around.)

Fishrock123 · 2015-03-19T14:24:13Z

Not a code review but +1 to these additions. :D

trevnorris · 2015-03-19T16:34:21Z

src/node_buffer.cc

+  uint32_t mask32 = args[0]->Uint32Value();
+  char* mask = (char*)&mask32;
+
+  Local<Object> target = args[1]->ToObject(env->isolate());


Before doing this should do CHECK(args[1]->IsObject()); and here do args[1].As<Object>(). Unless you don't expect args[1] to always be an object?

Will do. Should I add a similar check to buffer.copy?

Go ahead and add it to this PR, but leave it in a separate commit. Thanks :)

I'll add this to Copy in a separate PR – it causes test failures.

strange. well, don't worry about it then. don't want that change holding this up.

chrisdickinson · 2015-03-19T23:30:05Z

It might be worthwhile to get @3rd-Eden's opinion on these changes, too.

Fishrock123 · 2015-03-23T15:30:03Z

@chrisdickinson see #1010 (comment) :)

chrisdickinson · 2015-03-23T19:37:09Z

@Fishrock123 Ah, I meant that we should get @3rd-Eden's opinion on the specific differences from our mask vs. bufferutil's mask.

@bnoordhuis Your comments should be addressed now, PTAL (and thanks for the review.) Do you think it would be worthwhile to add the following optimization for aligned addresses?

#if INTPTR_MAX == INT64_MAX
if (!(target_data & 0x7) && !(obj_data & 0x7))
  AlignedMaskCopy64(obj_data, target_data, mask)
else 
#endif
if (!(target_data & 0x3) && !(obj_data & 0x3))
  AlignedMaskCopy32(obj_data, target_data, mask)
else {
  // current approach
}

theturtle32 · 2015-03-25T01:11:39Z

I'm a bit torn here... on the one hand it would be convenient to have core support for the native optimizations that both ws and my own websocket module use.

On the other hand, I think this particular mask function is so purpose-built that it would probably be used almost exclusively by the authors of WebSocket implementations. And on that basis, I don't think it really belongs in core.

Also, see my comment on iojs/io.js#1010 – I think the real issue is the difficulty of building native modules on Windows, and the user experience when those modules fail to build. I'd rather put a little bit of effort into improving the UX for Windows users without the ability to build the native optimizations, as they're not really necessary to use WebSockets anyway.

bnoordhuis · 2015-03-25T12:04:08Z

src/node_buffer.cc

+  Environment* env = Environment::GetCurrent(args);
+
+  CopyDWord mask;
+  mask.as_dword = args[0]->Uint32Value();


It doesn't hurt but you don't have to store the mask in a union, just a uint32_t is alright.

bnoordhuis · 2015-03-25T12:15:45Z

I've been reading through #1010 and I agree with @theturtle32 that this buffer method seems kind of specific.

chrisdickinson · 2015-03-25T19:04:11Z

From IRC, for posterity:

[11:25:57]  <chrisdickinson>    bnoordhuis: are you -1 on shipping buffer.mask() in core?
[11:30:48]  <bnoordhuis>    chrisdickinson: -1-ish. -1 unless ws module authors are asking for it and committing to using it once it exists
[11:31:27]  <chrisdickinson>    `3rdEden actually asked for it: https://github.com/iojs/io.js/issues/1010#issuecomment-85048624
[11:33:56]  <bnoordhuis>    chrisdickinson: okay, complaint withdrawn

@theturtle32: I don't think that improving this part of the Windows experience will hinder other efforts to make the native module system better. There are still plenty of other native modules that will continue to cause pain for Windows users (node-sass springs immediately to mind). Also, keep in mind that my effort here is not diverting attention from addressing the Windows build UX problem – I am sure that the folks at npm, inc. are working on that issue separately.

theturtle32 · 2015-03-25T20:33:04Z

@bnoordhuis @chrisdickinson – re: the IRC snippet above, I have a differing opinion from @3rd-Eden about whether this should be in core. I don't think it should, because I feel that it's too niche, so I'm not asking for it. But if it is shipped in core, of course I'll update websocket to use it once it exists.

On the other hand, I'd have to keep the existing native extensions to handle older versions of node/iojs anyway, which would still lead to a subpar experience for Windows users during npm install, since there's not a way to indicate in package.json that specific optionalDependencies should only be installed on certain engine versions. Otherwise l'll have to write a build script to check whether the native module is needed during installation, I suppose?

3rd-Eden · 2015-03-25T20:47:56Z

@theturtle32 The snippet above confidently misses the remarks I made in the channel. Copied IRC messages always lack and are often pulled out of context.

<`3rdEden> chrisdickinson: bnoordhuis: my comment didn't specifically asked for the masking additions. Nothing is needed in core to make websockets work.
<`3rdEden> Having buffer masking in core doesn't make a lot of sense as there no major use-case for it out side of websockets. UTF-8 validation on the other hand might be much more important.
<`3rdEden> chrisdickinson: yes, but masking only makes sense in core if protocol parsing and encoding is also included
<`3rdEden> Basically all or nothing
<`3rdEden> As just shipping buffer masking is like 1% of what you need for websockets.

3rd-Eden · 2015-03-25T20:49:57Z

Also, even if this change is merged in, it doesn't really solve anything as we will continue to ship our binary add-on as optionalDependency to support older versions of node and iojs.

theturtle32 · 2015-03-25T21:01:57Z

@3rd-Eden - based on that clarification, I'm pretty sure that you and I are actually on the same page.

chrisdickinson · 2015-03-25T22:02:24Z

@3rd-Eden The snippet was pasted before you made those comments, sorry. I continued on in IRC to explain that this is the first of the bufferutil/validate-utf8 functions I was PR'ing in (not the last), and asked if that changes your opinion of including these changes, but got no response.

In any case, this doesn't seem to be worthwhile. Closing.

theturtle32 · 2015-03-25T22:05:44Z

@chrisdickinson apologies for the cross-post, but I just mentioned in #1010:

While I'm not crazy about adding WebSocket-specific masking to core, I would be in favor of adding something like this for UTF-8 validation:

buffer.toString('utf8', 0, buffer.length, { strict: true }); // Throws on invalid UTF-8

chrisdickinson added 5 commits March 18, 2015 20:48

initial commit

914dc46

rework to adhere more closely to buf.copy

3e425fc

test style fixes

7bf9a2d

fixup whitespace

e92f4b7

document and add rough tests for return value

7ef29ac

chrisdickinson added buffer Issues and PRs related to the buffer subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 19, 2015

bnoordhuis reviewed Mar 19, 2015
View reviewed changes

trevnorris reviewed Mar 19, 2015
View reviewed changes

sam-github mentioned this pull request Mar 19, 2015

Ship websockets in core #1010

Closed

chrisdickinson added 3 commits March 19, 2015 12:54

test: nix cleverness

fb4ac8a

src: move to union to avoid pointer aliasing

0f7b918

src: fix style

8c72550

bnoordhuis reviewed Mar 25, 2015
View reviewed changes

chrisdickinson closed this Mar 25, 2015

This was referenced Mar 13, 2018

Feature Idea: Brotli support in core #18964

Closed

Adding Websocket support to core #19308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: add buf.mask for ws #1202

buffer: add buf.mask for ws #1202

chrisdickinson commented Mar 19, 2015

bnoordhuis Mar 19, 2015

piscisaureus Mar 19, 2015

bnoordhuis Mar 19, 2015

Fishrock123 commented Mar 19, 2015

trevnorris Mar 19, 2015

chrisdickinson Mar 19, 2015

trevnorris Mar 19, 2015

chrisdickinson Mar 19, 2015

trevnorris Mar 19, 2015

chrisdickinson commented Mar 19, 2015

Fishrock123 commented Mar 23, 2015

chrisdickinson commented Mar 23, 2015

theturtle32 commented Mar 25, 2015

bnoordhuis Mar 25, 2015

bnoordhuis commented Mar 25, 2015

chrisdickinson commented Mar 25, 2015

theturtle32 commented Mar 25, 2015

3rd-Eden commented Mar 25, 2015

3rd-Eden commented Mar 25, 2015

theturtle32 commented Mar 25, 2015

chrisdickinson commented Mar 25, 2015

theturtle32 commented Mar 25, 2015

buffer: add buf.mask for ws #1202

buffer: add buf.mask for ws #1202

Conversation

chrisdickinson commented Mar 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Mar 19, 2015

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

chrisdickinson commented Mar 19, 2015

Fishrock123 commented Mar 23, 2015

chrisdickinson commented Mar 23, 2015

theturtle32 commented Mar 25, 2015

Choose a reason for hiding this comment

bnoordhuis commented Mar 25, 2015

chrisdickinson commented Mar 25, 2015

theturtle32 commented Mar 25, 2015

3rd-Eden commented Mar 25, 2015

3rd-Eden commented Mar 25, 2015

theturtle32 commented Mar 25, 2015

chrisdickinson commented Mar 25, 2015

theturtle32 commented Mar 25, 2015