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

buffer: align chunks on 8-byte boundary #1126

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Mar 11, 2015

When slicing global pool - ensure that the underlying buffer's data ptr
is 8-byte alignment to do not ruin expectations of 3rd party C++ addons.

NOTE: 0.10 node.js always returned aligned pointers and io.js should do
this to for compatibility.

When slicing global pool - ensure that the underlying buffer's data ptr
is 8-byte alignment to do not ruin expectations of 3rd party C++ addons.

NOTE: 0.10 node.js always returned aligned pointers and io.js should do
this too for compatibility.
@indutny indutny force-pushed the fix/buffer-slice-ptr-alignment branch from 024592c to c7111d5 Compare March 11, 2015 14:28
@indutny
Copy link
Member Author

indutny commented Mar 11, 2015

Suggested reviewer: @trevnorris @bnoordhuis

@@ -157,6 +157,12 @@ function palloc(that, length) {
var buf = sliceOnto(allocPool, that, start, end);
poolOffset = end;

// Ensure aligned slices
if (poolOffset & 0x7) {
poolOffset |= 0x7;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, why is this line needed if the condition already succeeded?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. poolOffset = 1
  2. poolOffset & 0x7 === 1
  3. poolOffset |= 0x7 => 0x7
  4. poolOffset++ => 0x8
  5. Voila, poolOffset is aligned!

Copy link
Member Author

Choose a reason for hiding this comment

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

@mscdex in other words, I'm just setting all bits that are lower than alignment value and increment the number to align it.

Copy link
Member

Choose a reason for hiding this comment

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

A less convoluted way of writing that is poolOffset = (poolOffset & ~7) + 8 :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I used something like this: poolOffset += 8 - poolOffset % 8; :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I do this to for variable-size alignment or just non-power-of-two.

@indutny
Copy link
Member Author

indutny commented Mar 11, 2015

@bnoordhuis
Copy link
Member

LGTM

@indutny
Copy link
Member Author

indutny commented Mar 11, 2015

@bnoordhuis thanks! @trevnorris : please put your LGTM here too, just to make sure that I'm not messing up with any performance-critical things here.

@trevnorris
Copy link
Contributor

LGTM

@piscisaureus
Copy link
Contributor

lgtm

@indutny
Copy link
Member Author

indutny commented Mar 11, 2015

oh, last minute LGTM :) I almost pushed the commit without your name in Reviewed-By.

indutny added a commit that referenced this pull request Mar 11, 2015
When slicing global pool - ensure that the underlying buffer's data ptr
is 8-byte alignment to do not ruin expectations of 3rd party C++ addons.

NOTE: 0.10 node.js always returned aligned pointers and io.js should do
this too for compatibility.

PR-URL: #1126
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-by: Bert Belder <bertbelder@gmail.com>
@indutny
Copy link
Member Author

indutny commented Mar 11, 2015

Landed in 07c0667, thank you!

@indutny indutny closed this Mar 11, 2015
@indutny indutny deleted the fix/buffer-slice-ptr-alignment branch March 11, 2015 18:41
@rvagg rvagg mentioned this pull request Mar 14, 2015
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.

6 participants