Skip to content
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

Take into account the data queued in the sender #971

Merged
merged 1 commit into from Jan 25, 2017
Merged

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Jan 18, 2017

This makes the bufferedAmount getter take into account the data queued in the sender.

Fixes #970.

cc: @alexions

This makes the `bufferedAmount` getter take into account the data
queued in the sender.
readOnly = false;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this block of code to a separate function, like

makeBuffer (data) {
  var readOnly = true;

  if (data && !Buffer.isBuffer(data)) {
    if (data instanceof ArrayBuffer) {
      data = Buffer.from(data);
    } else if (ArrayBuffer.isView(data)) {
      data = viewToBuffer(data);
    } else {
      data = Buffer.from(data);
      readOnly = false;
    }
  }

  return [data, readOnly];
}

Usage:
[data, readOnly] = makeBuffer(data);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't like having this code duplication. The reason for not using a separate function is to avoid creating an array.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Array can make additional overhead... ok :) Speed is better than duplicates :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be benchmarked to see if it actually makes any difference :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be to add the readOnly flag to the returned buffer. For example:

const kReadOnly = Symbol('read-only');

function toBuffer(data) {
  var readOnly = true;

  if (!Buffer.isBuffer(data)) {
    if (data instanceof ArrayBuffer) {
      data = Buffer.from(data);
    } else if (ArrayBuffer.isView(data)) {
      data = viewToBuffer(data);
    } else {
      data = Buffer.from(data);
      readOnly = false;
    }
  }

  data[kReadOnly] = readOnly;
  return data;
}

but I'm still not happy with it as that changes the buffer hidden class.

@lpinca
Copy link
Member Author

lpinca commented Jan 21, 2017

I would like to have at least one LGTM from the other @websockets/admin members before merging.

Copy link

@alexions alexions left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lpinca How do you like this variant?
#975

@lpinca
Copy link
Member Author

lpinca commented Jan 23, 2017

@alexions thanks but I'm -1 on that approach as that proposes to check the things twice (2x Buffer.isBuffer(), 2x ArrayBuffer.isView(), etc.), first to see if data is readOnly then to convert it to a buffer. I would rather keep the duplicated code.

@alexions
Copy link

ok. Agree that your changes will work faster.

@lpinca
Copy link
Member Author

lpinca commented Jan 23, 2017

Before this change the whole if...elsechain was used in 2 functions, now in 3 so I think it's not so bad. I'm just waiting for the approval of another @websockets/admin member :)

@lpinca
Copy link
Member Author

lpinca commented Jan 24, 2017

I'll merge this tomorrow if there are no objections.

@lpinca lpinca merged commit d74a32e into master Jan 25, 2017
@lpinca lpinca deleted the fix/buffered-amount branch January 25, 2017 08:36
@lpinca
Copy link
Member Author

lpinca commented Jan 25, 2017

@alexions thanks!

@alexions
Copy link

@lpinca you're welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants