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

[breaking/format] Remove vlen from entry header #945

Merged
merged 5 commits into from
Aug 7, 2019

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Jul 27, 2019

This commit removes value length from the entry header stored in each SST.
We don't need vlen to store the length of the value. We can find it by
using the entry offsets in the footer of the blocks.

Entries in the table are of the form

Klen1 (Point A) Plen1 Key1 Value1 Klen2 (Point B)

And we have the entry index at the end of each block.

Entry 1 offset (Point A) Entry 2 Offset (Point B)

Using the entry index and current position in the buffer, we can find
the length of the value.

Note this PR also reduces the size of the header from 8 bytes to 4 bytes.


This change is Reviewable

This commit removes value length from entry header stored in each SST.
We don't need vlen to store the length of the value. We can find it by
using the entry offsets stored in the footer of the blocks.

Entries in the table are of the form
+-----------------+-------+------+--------+-----------------+------+
| Klen1 (Point A) | Plen1 | Key1 | Value1 | Klen2 (Point B) | .... |
+-----------------+-------+------+--------+-----------------+------+

And we have the entry index at the end of each block.
+---------------------------+--------------------------+
| Entry1 offset 1 (Point A) | Entry Offset 1 (Point B) |
+---------------------------+--------------------------+

Using the entry index and current position in the buffer, we can find
the length of the value.
@jarifibrahim jarifibrahim requested a review from poonai July 27, 2019 06:08
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Test it thoroughly. Great change, but just ensure that nothing is breaking.

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @jarifibrahim and @manishrjain)


table/iterator.go, line 46 at r1 (raw file):

	itr.pos = 0
	itr.err = nil
	itr.baseKey = itr.baseKey[:0]

Not so sure about this change. Have to be sure that we're not using that array anywhere else.


table/iterator.go, line 165 at r1 (raw file):

	// We're at the last entry in the block.
	if itr.currentIdx == itr.numEntries-1 {
		valEndOffset = uint32(itr.entriesIndexStart)

Is it possible that this might be zero?

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @jarifibrahim and @manishrjain)


table/iterator.go, line 176 at r1 (raw file):

		return
	}
	itr.val = y.SafeCopy(itr.val, itr.data[itr.pos:valEndOffset])

Add a TODO here to check if we can avoid this copy.

Copy link
Contributor Author

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @manishrjain)


table/iterator.go, line 46 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Not so sure about this change. Have to be sure that we're not using that array anywhere else.

We do a Copy(..., itr.basekey) everywhere.


table/iterator.go, line 165 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Is it possible that this might be zero?

No. Every time table.Finish() would be called we will write all the entries to the end of the block and entriesIndexStart will point to that offset.


table/iterator.go, line 176 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Add a TODO here to check if we can avoid this copy.

Done.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain)

@jarifibrahim jarifibrahim merged commit 88d5a3c into master Aug 7, 2019
@jarifibrahim jarifibrahim deleted the ibrahim/remove-vlen branch August 7, 2019 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants