dice icon indicating copy to clipboard operation
dice copied to clipboard

moved from the unsafe pointer to the string type for the store

Open AshwinKul28 opened this issue 1 year ago • 6 comments

Git issue: https://github.com/DiceDB/dice/issues/350

Issue Description

  1. Storing map[unsafe.Pointer]*Obj is not, because it removes statically type safety at compile time. Using unsafe.Pointer allows you to cast between different pointer types without any type checking, which can lead to subtle and hard-to-debug runtime errors.

  2. Moving key to a string type don't have any issues because ultimately every key is getting stored in the string format.

  3. The keypool of type map[string]*string is nothing but an indexing for keys which can be removed in the refactoring.

Solution:

  1. store keys can be stored in the form of string and it can point to the Object type: map[string]*Obj
  2. Currently keypool is changed as map[string]*string since it's pointing to the address of key. (we can remove it)

AshwinKul28 avatar Aug 21 '24 06:08 AshwinKul28

All cases passed successfully.

Screenshot 2024-08-21 at 12 05 05 PM

AshwinKul28 avatar Aug 21 '24 06:08 AshwinKul28

@AshwinKul28 Can you run a benchmark test here comparing it with the previous approach?

gsarmaonline avatar Aug 21 '24 11:08 gsarmaonline

@AshwinKul28 does the existing tests include edge cases and strings with unicode characters such as: Below are some of the valid key types supported by Redis:

redis:6379> SET "" abc OK redis:6379> SET "\xff" def OK redis:6379> SET "� " ghi OK redis:6379> SET " 😀" jkl OK redis:6379> SET '\xff' mno OK

redis:6379> get "" "abc" redis:6379> get "\xff" "def" redis:6379> GET " 😀" "jkl" redis:6379> GET "� " "abc" redis:6379> GET '\xff' "mno"

soumya-codes avatar Aug 21 '24 11:08 soumya-codes

The changes look great!

@AshwinKul28 does the existing tests include edge cases and strings with unicode characters such as: Below are some of the valid key types supported by Redis:

We don't have to support these scenarios in this PR if they were not supported earlier. However if they worked earlier, we may need to take a call of what to do.

Please let me know whats your opinion.

soumya-codes avatar Aug 21 '24 12:08 soumya-codes

@AshwinKul28 Can you run a benchmark test here comparing it with the previous approach?

@gauravsarma1992 yes was thinking the same. let me run it now.

dice-benchmark.txt

dice-refactoring-benchmark.txt

[Note: this suite doesn't have BenchmarkEvalMSET test included. Opened an issue for that - #385

I see good numbers in the benchmark test suite.

AshwinKul28 avatar Aug 21 '24 17:08 AshwinKul28

redis:6379> SET "" abc OK redis:6379> SET "\xff" def OK redis:6379> SET "� " ghi OK redis:6379> SET " 😀" jkl OK redis:6379> SET '\xff' mno OK

redis:6379> get "" "abc" redis:6379> get "\xff" "def" redis:6379> GET " 😀" "jkl" redis:6379> GET "� " "abc" redis:6379> GET '\xff' "mno"

As per above key types mentioned by soumya, here's observation on the dice server using redis cli

Screenshot_2024-08-22_at_12 25 51_AM

Everything looks correct and expected. 🚀

AshwinKul28 avatar Aug 21 '24 19:08 AshwinKul28