moved from the unsafe pointer to the string type for the store
Git issue: https://github.com/DiceDB/dice/issues/350
Issue Description
-
Storing
map[unsafe.Pointer]*Objis 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. -
Moving key to a string type don't have any issues because ultimately every key is getting stored in the string format.
-
The keypool of type
map[string]*stringis nothing but an indexing for keys which can be removed in the refactoring.
Solution:
- store keys can be stored in the form of string and it can point to the Object type:
map[string]*Obj - Currently keypool is changed as
map[string]*stringsince it's pointing to the address of key. (we can remove it)
All cases passed successfully.
@AshwinKul28 Can you run a benchmark test here comparing it with the previous approach?
@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"
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.
@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-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.
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
Everything looks correct and expected. 🚀