Skip to content

ARROW-2397: [Documentation] Update format documentation to describe tensor alignment.#1837

Closed
robertnishihara wants to merge 2 commits intoapache:masterfrom
robertnishihara:fixdoc
Closed

ARROW-2397: [Documentation] Update format documentation to describe tensor alignment.#1837
robertnishihara wants to merge 2 commits intoapache:masterfrom
robertnishihara:fixdoc

Conversation

@robertnishihara
Copy link
Copy Markdown
Contributor

No description provided.

format/IPC.md Outdated
When writing a standalone encapsulated tensor message, we use the format as
indicated above, but additionally align the starting offset (if writing to a
shared memory region) to be a multiple of 8:
indicated above, but additionally align the starting offset of the metadata as well as the starting offset of the tensor body (if writing to a shared
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you wrap the text at the same line width as above?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

shared memory region) to be a multiple of 8:
indicated above, but additionally align the starting offset of the metadata as
well as the starting offset of the tensor body (if writing to a shared memory
region) to be multiples of 64 bytes:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that this used to be 8 and I replaced it with 64. Please check that this is correct.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From what we want we want as alignment in the Arrow standard, this is correct. I don't know if all our implementations that emit tensors conform to this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see, I was just looking at

Status AlignStreamPosition(io::OutputStream* stream) {
int64_t position;
RETURN_NOT_OK(stream->Tell(&position));
int64_t remainder = PaddedLength(position) - position;
if (remainder > 0) {
return stream->Write(kPaddingBytes, remainder);
}
return Status::OK();
}

and

static constexpr int kArrowAlignment = 64;
// Align on 8-byte boundaries in IPC
static constexpr int kArrowIpcAlignment = 8;
static constexpr uint8_t kPaddingBytes[kArrowAlignment] = {0};

@robertnishihara
Copy link
Copy Markdown
Contributor Author

@xhochy should we merge this? Or should I change the number back to 8?

@xhochy
Copy link
Copy Markdown
Member

xhochy commented Apr 13, 2018

@robertnishihara I think we can go ahead and merge this.

@robertnishihara robertnishihara deleted the fixdoc branch April 14, 2018 21:01
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.

2 participants