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

[Issue 1021][client] FIX: use maphash instead of crypto/sha256 for hash function of hashmap in Schema.hash() #1022

Merged
merged 1 commit into from
May 31, 2023

Conversation

bpereto
Copy link
Contributor

@bpereto bpereto commented May 29, 2023

use golang/hash/maphash instead of crypto/sha256 for hash function of hashmap in Schema.hash()
as the hash function is used for determining the hash key, it is safe to use a non-cryptographic function.

Fixes #1021

Motivation

Improve performance of the SchemaCache by replacing the cryptographic hash function.

Modifications

Replaced the crypto/sha256 with golang/hash/maphash function.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no

Documentation

  • Does this pull request introduce a new feature? no

Copy link

@kevinkl3 kevinkl3 left a comment

Choose a reason for hiding this comment

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

LGTM

@bpereto
Copy link
Contributor Author

bpereto commented May 29, 2023

I saw the seed is random. so theres a method needed to fix the seed.

@bpereto bpereto changed the title [Issue 1021][client] FIX: use maphash instead of crypto/sha256 for hash function of hashmap in Schema.hash() WIP: [Issue 1021][client] FIX: use maphash instead of crypto/sha256 for hash function of hashmap in Schema.hash() May 29, 2023
@bpereto bpereto changed the title WIP: [Issue 1021][client] FIX: use maphash instead of crypto/sha256 for hash function of hashmap in Schema.hash() [Issue 1021][client] FIX: use maphash instead of crypto/sha256 for hash function of hashmap in Schema.hash() May 29, 2023
@bpereto
Copy link
Contributor Author

bpereto commented May 29, 2023

Added a static seed with MakeSeed.
tested it with my application, works as intended for me.

its ready from my side.

@bpereto bpereto force-pushed the fix-schema-hash branch 3 times, most recently from e6fbaa3 to 2615403 Compare May 29, 2023 21:05
…p in Schema.hash()

- replace sha256 hash function with hash/maphash
- use uint64 (expected from maphash) as schema cache hashmap key
- init static seed as it is random generated
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thank you!

To double-check the patch - it changes local access to the schema map, without any changes to the actual schema go through the wire, right?

@bpereto
Copy link
Contributor Author

bpereto commented May 31, 2023

Thank you!

To double-check the patch - it changes local access to the schema map, without any changes to the actual schema go through the wire, right?

yes, no changes to the schema itself, just the hash method to get a key for the local hashmap struct to update or access the cached schema.

@tisonkun
Copy link
Member

Merging...

@tisonkun tisonkun merged commit 2eb3405 into apache:master May 31, 2023
@tisonkun
Copy link
Member

Thank you!

@BewareMyPower BewareMyPower added this to the v0.11.0 milestone Jun 5, 2023
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.

Schema.hash is ressource intensive
4 participants