Replace dict with new hashtable: sorted set datatype

[rainsupreme]

This PR replaces dict with hashtable in the ZSET datatype. Instead of mapping key to score as dict did, the hashtable maps key to a node in the skiplist, which contains the score. This takes advantage of hashtable performance improvements and saves 15 bytes per set item - 24 bytes overhead before, 9 bytes after.

Closes #1096

[zuiderkwast]

This PR is ready for review! Needed changes have been merged and I've rebased

Great! You could mark it as not draft then. :) We use the top comment and PR title for the final commit message when it gets merged, so you could update those to concisely describe the change (i.e. like a commit message).

It's late in my time zone so I'll look tomorrow.

[codecov]

Codecov Report

Attention: Patch coverage is 92.55663% with 23 lines in your changes missing coverage. Please review.

Project coverage is 70.75%. Comparing base (b3b4bdc) to head (8316c90).
Report is 11 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/module.c	0.00%	10 Missing ⚠️
src/defrag.c	84.37%	5 Missing ⚠️
src/object.c	37.50%	5 Missing ⚠️
src/db.c	89.47%	2 Missing ⚠️
src/debug.c	92.30%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1427      +/-   ##
============================================
- Coverage     70.83%   70.75%   -0.09%     
============================================
  Files           120      120              
  Lines         64911    64959      +48     
============================================
- Hits          45982    45962      -20     
- Misses        18929    18997      +68     

Files with missing lines	Coverage Δ
src/aof.c	80.23% <100.00%> (+0.11%)	⬆️
src/evict.c	98.47% <100.00%> (-0.38%)	⬇️
src/geo.c	93.58% <100.00%> (+0.02%)	⬆️
src/rdb.c	76.75% <100.00%> (+0.67%)	⬆️
src/server.c	87.61% <100.00%> (+0.14%)	⬆️
src/server.h	100.00% <ø> (ø)
src/sort.c	94.82% <100.00%> (-0.34%)	⬇️
src/t_zset.c	96.80% <100.00%> (+1.13%)	⬆️
src/debug.c	52.12% <92.30%> (+0.13%)	⬆️
src/db.c	89.48% <89.47%> (-0.07%)	⬇️
... and 3 more

... and 37 files with indirect coverage changes

[zuiderkwast]

This looks awesome. Just a few nits.

[ranshid]

great work @SoftlyRaining !

Some minor comments and I feel somewhat uncomfortable about the way we implemented the union.

[zuiderkwast]

Just fix the naming converion (compareNodeScoreEle -> zslCompareNodeScoreEle, etc.). Then I think it's good to merge.

I don't really get why you want to delete zslGetRank. If it's an optimization beyond the replacement of dict, then it can as well be a follow up, right? As you want.

[rainsupreme]

I poked around and found that all uses of zslGetRank could more efficiently use zslGetRankByNode, and it became dead code. I already made the revision at any rate but you're right, it might've been a separate small cleanup PR 😅

[ranshid]

I poked around and found that all uses of zslGetRank could more efficiently use zslGetRankByNode, and it became dead code. I already made the revision at any rate but you're right, it might've been a separate small cleanup PR 😅

@SoftlyRaining My only concern is a potential small degradation in performance of zcount and zlexcount. can we just make sure to verify we have no impact on these operations?

[rainsupreme]

I poked around and found that all uses of zslGetRank could more efficiently use zslGetRankByNode, and it became dead code. I already made the revision at any rate but you're right, it might've been a separate small cleanup PR 😅

@SoftlyRaining My only concern is a potential small degradation in performance of zcount and zlexcount. can we just make sure to verify we have no impact on these operations?

I will remove that aspect of the change so it can be more thoroughly investigated and benchmarked as a separate PR. I'd prefer to avoid delaying the core hashtable work. :)

[zuiderkwast]

LGTM

@ranshid The zslGetRank changes have been removed, so I guess it's safe to merge. WDYT?

[ranshid]

Small comment which I think we could skip handling for now and maybe only extend the comment.
I did not rescan the entire change so LGTM

[ranshid]

Small mark: it is still possible that a node position change will NOT take place after this check. For example in case we update the score to something that exactly matches the score of the prev or next node.
The check can be extended to check also equality of the score (but will also need to compare the key order).
I guess this is fine for now, but maybe extend the comment above to explain that?

[zuiderkwast]

Right, the edge cases. Now, we remove and re-insert the node at the same position in this case, which is OK. It's not introduced in this PR anyway. We can convert the comment to a follow-up issue.

We could use zslCompareNodes here, but then we'd need to set the new score before we compare and then revert it if the check fails. Or use a stack-allocated temporary node for comparing, just to be able to use zslCompareNodes.

Suggested change

	if ((node->backward == NULL || node->backward->score < newscore) &&
	(node->level[0].forward == NULL || node->level[0].forward->score > newscore)) {
	node->score = newscore;
	return NULL;
	}
	double oldscore = node->score;
	node->score = newscore;
	if ((node->backward == NULL || zslCompareNodes(node->backward, node) <= 0) &&
	zslCompareNodes(node->level[0].forward, node) >= 0) {
	return NULL;
	} else {
	/* Restore score to restore skiplist order. */
	node->score = oldscore;
	}