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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bucket.findEntryOffset() can reduce calls to getKeySize() #5179

Closed
swirlds-automation opened this issue Mar 3, 2023 · 2 comments 路 Fixed by #5949
Closed

Bucket.findEntryOffset() can reduce calls to getKeySize() #5179

swirlds-automation opened this issue Mar 3, 2023 · 2 comments 路 Fixed by #5949
Assignees
Labels

Comments

@swirlds-automation
Copy link
Collaborator

馃敄 In Bucket.findEntryOffset(), there is no need to compute the key size for the last entry.

...
	// TODO ideally we would only read this once and use in key.equals and here
	// TODO also could avoid doing this for last entry
@swirlds-automation
Copy link
Collaborator Author

migrated from:
url=https://github.com/swirlds/swirlds-platform/issues/3932
author:tinker-michaelj, #:3932, createdAt:2021-09-28T15:02:29Z, updatedAt=2023-02-21T23:50:54Z
labels=Performance,Platform Virtual Map,Platform Data Structures,Migration:Data

@artemananiev
Copy link
Member

Skipping call to getKeySize() for the last key in a bucket looks easy, let me do that.

As for getting key sizes and leveraging them in equals(), does it really make sense? I checked all existing virtual key serializers in HS code, some of them use fixed-sized keys (so they just return a constant in deserializeKeySize()), others just read a single byte from the buffer. Not much to save.

OK, if we still decide to calculate key sizes just once, I've got two ideas:

  • Call deserializeKeySize() regardless, then pass the size as an extra arg to equalsTo(). Leave it up to implementations to make use (or not) this key size
  • Change equalsTo() return value from boolean to int. If it returns 0, the key is considered equal to what's in the buffer. In not 0, it indicates the key size from the buffer. Something like Either<Boolean, Integer> in scala terms

Both ideas don't look very nice to me. Given that we would be able to save just a single call to ByteBuffer.get(), I don't think they are worth implementing. @tinker-michaelj @jasperpotts @OlegMazurov what do you think about it?

artemananiev added a commit that referenced this issue Apr 4, 2023
Fixes: #5179
Fixes: #5188
Reviewed-by: Ivan Malygin <ivan@swirldslabs.com>, Oleg Mazurov <oleg.mazurov@swirldslabs.com>
Signed-off-by: Artem Ananev <artem.ananev@swirldslabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants