changed addReplyHumanLongDouble to addReplyDouble in georadiusGeneric and geoposCommand #13494

fcostaoliveira · 2024-08-27T09:57:31Z

Summary

Addresses addReplyHumanLongDouble has a large overhead/weight on GEOPOS command performance #11565
Measured improvements of 30% and 37% on the simple use-case (GEOSEARCH and GEOPOS) (check changed addReplyHumanLongDouble to addReplyDouble in georadiusGeneric and geoposCommand #13494 (comment)), and of 66% on a dataset with >60M datapoints and pipeline 10 benchmark.

TODO:

Cover this on benchmark spec, and trigger it here.
Ensure all functional tests are green after the change. check changed addReplyHumanLongDouble to addReplyDouble in georadiusGeneric and geoposCommand #13494 (comment)

Manual check on 60M datapoints dataset:

unstable

$ memtier_benchmark --pipeline 10 --test-time 60 --command "GEOPOS key 1 2" --hide-histogram
Writing results to stdout
[RUN #1] Preparing benchmark client...
[RUN #1] Launching threads now...
[RUN #1 100%,  60 secs]  0 threads:    18822810 ops,  310818 (avg:  313635) ops/sec, 47.13MB/sec (avg: 47.56MB/sec),  6.42 (avg:  6.36) msec latency

4         Threads
50        Connections per thread
60        Seconds


ALL STATS
==================================================================================================
Type         Ops/sec    Avg. Latency     p50 Latency     p99 Latency   p99.9 Latency       KB/sec 
--------------------------------------------------------------------------------------------------
Geoposs    313631.33         6.36543         6.52700        13.31100        20.22300     48698.62 
Totals     313631.33         6.36543         6.52700        13.31100        20.22300     97397.23

this PR

$ memtier_benchmark --pipeline 10 --test-time 60 --command "GEOPOS key 1 2" --hide-histogram
Writing results to stdout
[RUN #1] Preparing benchmark client...
[RUN #1] Launching threads now...
[RUN #1 100%,  60 secs]  0 threads:    31251690 ops,  529909 (avg:  520743) ops/sec, 77.32MB/sec (avg: 75.98MB/sec),  3.76 (avg:  3.83) msec latency

4         Threads
50        Connections per thread
60        Seconds


ALL STATS
==================================================================================================
Type         Ops/sec    Avg. Latency     p50 Latency     p99 Latency   p99.9 Latency       KB/sec 
--------------------------------------------------------------------------------------------------
Geoposs    520719.80         3.82928         3.80700         8.51100        14.59100     77802.86 
Totals     520719.80         3.82928         3.80700         8.51100        14.59100    155605.72

… and geoposCommand

fcostaoliveira · 2024-08-27T15:16:32Z

CE Performance Automation : step 1 of 2 (build) DONE.

This comment was automatically generated given a benchmark was triggered.
Started building at 2024-08-28 13:44:38.209611 and took 85 seconds.
You can check each build/benchmark progress in grafana:

git hash: 9722096
git branch: filipecosta90:perf.geo
commit date and time: n/a
commit summary: n/a
test filters:
- command priority lower limit: 0
- command priority upper limit: 10000
- test name regex: .*
- command group regex: .*

You can check a comparison in detail via the grafana link

fcostaoliveira · 2024-08-27T15:17:59Z

CE Performance Automation : step 2 of 2 (benchmark) FINISHED.

This comment was automatically generated given a benchmark was triggered.

Started benchmark suite at 2024-10-20 00:10:33.000811 and took 4992.265434 seconds to finish.
Status: [################################################################################] 100.0% completed.

In total will run 135 benchmarks.
- 0 pending.
- 135 completed:
- 0 successful.
- 135 failed.
You can check a the status in detail via the grafana link

fcostaoliveira · 2024-08-27T22:15:03Z

Automated performance analysis summary

This comment was automatically generated given there is performance data available.

Using platform named: intel64-ubuntu22.04-redis-icx1 to do the comparison.

In summary:

Detected a total of 2 improvements above the improvement water line.

You can check a comparison in detail via the grafana link

Comparison between unstable and perf.geo.

Time Period from 5 months ago. (environment used: oss-standalone)

Improvements Table

Test Case	Baseline unstable (median obs. +- std.dev)	Comparison perf.geo (median obs. +- std.dev)	% change (higher-better)	Note
memtier_benchmark-1key-geo-2-elements-geosearch-fromlonlat-withcoord	70609	96810	37.1%	IMPROVEMENT
memtier_benchmark-1key-geo-2-elements-geopos	121128	157873	30.3%	IMPROVEMENT

Improvements test regexp names: memtier_benchmark-1key-geo-2-elements-geosearch-fromlonlat-withcoord|memtier_benchmark-1key-geo-2-elements-geopos

Full Results table:

Test Case	Baseline unstable (median obs. +- std.dev)	Comparison perf.geo (median obs. +- std.dev)	% change (higher-better)	Note
memtier_benchmark-1key-geo-2-elements-geosearch-fromlonlat-withcoord	70609	96810	37.1%	IMPROVEMENT
memtier_benchmark-1key-geo-2-elements-geopos	121128	157873	30.3%	IMPROVEMENT

sundb · 2024-08-28T13:12:38Z

all green: https://github.com/sundb/redis/actions/runs/10588976983

oranagra

so we decided to give up the "human" part that Itamar was after, for the performance gain?
LGTM, but maybe we wanna consider using fixedpoint_d2string here?
i.e. the other thing he said was that geo isn't precise enough anyway to justify so many digits anyway.

fcostaoliveira · 2024-09-03T10:05:17Z

@oranagra i've tested locally doing the change in geopos from addReplyDouble to fixedpoint_d2string + addReplyBulkCBuffer

-            addReplyDouble(c,xy[0]);
-            addReplyDouble(c,xy[1]);
+            int dlen = fixedpoint_d2string(dbuf, sizeof(dbuf), xy[0], 17);
+            addReplyBulkCBuffer(c, dbuf, dlen);
+            dlen = fixedpoint_d2string(dbuf, sizeof(dbuf), xy[1], 17);
+            addReplyBulkCBuffer(c, dbuf, dlen);

and we can see that the results between fixed_point and this PR are very close (slight improvement on addReplyDouble). I believe we should avoid specific double reply logic for this command. it will be easier to maintain and follow the same practices of all other double replies.

redis variation	memtier_benchmark-1key-geo-2-elements-geopos benchmark ops/sec
unstable (`ed10f73`)	71742
fixed_point	91070
this PR (`9722096`)	92572

agree?

oranagra · 2024-09-03T12:26:12Z

Ok. Fine with me

### New Features in binary distributions - 7 new data structures: JSON, Time series, Bloom filter, Cuckoo filter, Count-min sketch, Top-k, t-digest - Redis scalable query engine (including vector search) ### Potentially breaking changes - #12272 `GETRANGE` returns an empty bulk when the negative end index is out of range - #12395 Optimize `SCAN` command when matching data type ### Bug fixes - #13510 Fix `RM_RdbLoad` to enable AOF after RDB loading is completed - #13489 `ACL CAT` - return module commands - #13476 Fix a race condition in the `cache_memory` of `functionsLibCtx` - #13473 Fix incorrect lag due to trimming stream via `XTRIM` command - #13338 Fix incorrect lag field in `XINFO` when tombstone is after the `last_id` of the consume group - #13470 On `HDEL` of last field - update the global hash field expiration data structure - #13465 Cluster: Pass extensions to node if extension processing is handled by it - #13443 Cluster: Ensure validity of myself when loading cluster config - #13422 Cluster: Fix `CLUSTER SHARDS` command returns empty array ### Modules API - #13509 New API calls: `RM_DefragAllocRaw`, `RM_DefragFreeRaw`, and `RM_RegisterDefragCallbacks` - defrag API to allocate and free raw memory ### Performance and resource utilization improvements - #13503 Avoid overhead of comparison function pointer calls in listpack `lpFind` - #13505 Optimize `STRING` datatype write commands - #13499 Optimize `SMEMBERS` command - #13494 Optimize `GEO*` commands reply - #13490 Optimize `HELLO` command - #13488 Optimize client query buffer - #12395 Optimize `SCAN` command when matching data type - #13529 Optimize `LREM`, `LPOS`, `LINSERT`, and `LINDEX` commands - #13516 Optimize `LRANGE` and other commands that perform several writes to client buffers per call - #13431 Avoid `used_memory` contention when updating from multiple threads ### Other general improvements - #13495 Reply `-LOADING` on replica while flushing the db ### CLI tools - #13411 redis-cli: Fix wrong `dbnum` showed after the client reconnected ### Notes - No backward compatibility for replication or persistence. - Additional distributions, upgrade paths, features, and improvements will be introduced in upcoming pre-releases. - With the GA release of 8.0 we will deprecate Redis Stack.

guybe7 · 2025-04-15T09:39:21Z

8.0:

127.0.0.1:6379> GEOADD Sicily 13.361389 38.115556 Palermo
(integer) 1
127.0.0.1:6379> GEORADIUS Sicily 15 37 200 km withcoord withdist withhash 
1) 1) "Palermo"
   2) "190.4424"
   3) (integer) 3479099956230698
   4) 1) "13.361389338970184"
      2) "38.1155563954963"

7.4:

127.0.0.1:6379> GEOADD Sicily 13.361389 38.115556 Palermo
(integer) 1
127.0.0.1:6379> GEORADIUS Sicily 15 37 200 km withcoord withdist withhash 
1) 1) "Palermo"
   2) "190.4424"
   3) (integer) 3479099956230698
   4) 1) "13.36138933897018433"
      2) "38.11555639549629859"

breaking change?
@yossigo @oranagra

LiorKogan · 2025-04-15T09:49:37Z

These strings are IEEE-754 FP64 equivalent.
cc @filipecosta90

guybe7 · 2025-04-15T10:05:17Z

@LiorKogan, maybe, but what if the user relies on the command to return this and that number of characters in the reply?
how come we allow this but not willing to turn off lazy expire when using SCAN with TYPE? whihc is not even a user-facing side effect of the command

oranagra · 2025-04-15T11:29:56Z

if the user relies on the actual string representation and the number of chars, IMHO he deserves a breakage (which he'll handle easily).
i personally think the SCAN type filter for lazy expire change should have been kept. 🤷

… and geoposCommand (redis#13494) # Summary - Addresses redis#11565 - Measured improvements of 30% and 37% on the simple use-case (GEOSEARCH and GEOPOS) (check redis#13494 (comment)), and of 66% on a dataset with >60M datapoints and pipeline 10 benchmark.

### New Features in binary distributions - 7 new data structures: JSON, Time series, Bloom filter, Cuckoo filter, Count-min sketch, Top-k, t-digest - Redis scalable query engine (including vector search) ### Potentially breaking changes - redis#12272 `GETRANGE` returns an empty bulk when the negative end index is out of range - redis#12395 Optimize `SCAN` command when matching data type ### Bug fixes - redis#13510 Fix `RM_RdbLoad` to enable AOF after RDB loading is completed - redis#13489 `ACL CAT` - return module commands - redis#13476 Fix a race condition in the `cache_memory` of `functionsLibCtx` - redis#13473 Fix incorrect lag due to trimming stream via `XTRIM` command - redis#13338 Fix incorrect lag field in `XINFO` when tombstone is after the `last_id` of the consume group - redis#13470 On `HDEL` of last field - update the global hash field expiration data structure - redis#13465 Cluster: Pass extensions to node if extension processing is handled by it - redis#13443 Cluster: Ensure validity of myself when loading cluster config - redis#13422 Cluster: Fix `CLUSTER SHARDS` command returns empty array ### Modules API - redis#13509 New API calls: `RM_DefragAllocRaw`, `RM_DefragFreeRaw`, and `RM_RegisterDefragCallbacks` - defrag API to allocate and free raw memory ### Performance and resource utilization improvements - redis#13503 Avoid overhead of comparison function pointer calls in listpack `lpFind` - redis#13505 Optimize `STRING` datatype write commands - redis#13499 Optimize `SMEMBERS` command - redis#13494 Optimize `GEO*` commands reply - redis#13490 Optimize `HELLO` command - redis#13488 Optimize client query buffer - redis#12395 Optimize `SCAN` command when matching data type - redis#13529 Optimize `LREM`, `LPOS`, `LINSERT`, and `LINDEX` commands - redis#13516 Optimize `LRANGE` and other commands that perform several writes to client buffers per call - redis#13431 Avoid `used_memory` contention when updating from multiple threads ### Other general improvements - redis#13495 Reply `-LOADING` on replica while flushing the db ### CLI tools - redis#13411 redis-cli: Fix wrong `dbnum` showed after the client reconnected ### Notes - No backward compatibility for replication or persistence. - Additional distributions, upgrade paths, features, and improvements will be introduced in upcoming pre-releases. - With the GA release of 8.0 we will deprecate Redis Stack.

changed addReplyHumanLongDouble to addReplyDouble in georadiusGeneric…

9722096

… and geoposCommand

fcostaoliveira added the action:run-benchmark Triggers the benchmark suite for this Pull Request label Aug 27, 2024

sundb added the release-notes indication that this issue needs to be mentioned in the release notes label Aug 28, 2024

fcostaoliveira requested a review from sundb August 29, 2024 08:29

sundb approved these changes Aug 29, 2024

View reviewed changes

fcostaoliveira requested a review from oranagra September 2, 2024 12:43

oranagra reviewed Sep 2, 2024

View reviewed changes

sundb merged commit fb8755a into redis:unstable Sep 3, 2024

YaacovHazan mentioned this pull request Sep 11, 2024

Redis 8.0 M01 #13538

Merged

fcostaoliveira mentioned this pull request Sep 12, 2024

addReplyHumanLongDouble has a large overhead/weight on GEOPOS command performance #11565

Closed

sundb added this to Redis 8.0 Aug 15, 2025

sundb moved this to Done in Redis 8.0 Aug 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

changed addReplyHumanLongDouble to addReplyDouble in georadiusGeneric and geoposCommand #13494

changed addReplyHumanLongDouble to addReplyDouble in georadiusGeneric and geoposCommand #13494

Uh oh!

fcostaoliveira commented Aug 27, 2024 •

edited

Loading

Uh oh!

fcostaoliveira commented Aug 27, 2024 •

edited

Loading

Uh oh!

fcostaoliveira commented Aug 27, 2024 •

edited

Loading

Uh oh!

fcostaoliveira commented Aug 27, 2024

Uh oh!

sundb commented Aug 28, 2024

Uh oh!

oranagra left a comment

Uh oh!

fcostaoliveira commented Sep 3, 2024

Uh oh!

oranagra commented Sep 3, 2024

Uh oh!

guybe7 commented Apr 15, 2025

Uh oh!

LiorKogan commented Apr 15, 2025

Uh oh!

guybe7 commented Apr 15, 2025

Uh oh!

oranagra commented Apr 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

changed addReplyHumanLongDouble to addReplyDouble in georadiusGeneric and geoposCommand #13494

changed addReplyHumanLongDouble to addReplyDouble in georadiusGeneric and geoposCommand #13494

Uh oh!

Conversation

fcostaoliveira commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Manual check on 60M datapoints dataset:

unstable

this PR

Uh oh!

fcostaoliveira commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CE Performance Automation : step 1 of 2 (build) DONE.

Uh oh!

fcostaoliveira commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CE Performance Automation : step 2 of 2 (benchmark) FINISHED.

Uh oh!

fcostaoliveira commented Aug 27, 2024

Automated performance analysis summary

Comparison between unstable and perf.geo.

Improvements Table

Uh oh!

sundb commented Aug 28, 2024

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

fcostaoliveira commented Sep 3, 2024

Uh oh!

oranagra commented Sep 3, 2024

Uh oh!

guybe7 commented Apr 15, 2025

Uh oh!

LiorKogan commented Apr 15, 2025

Uh oh!

guybe7 commented Apr 15, 2025

Uh oh!

oranagra commented Apr 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fcostaoliveira commented Aug 27, 2024 •

edited

Loading

fcostaoliveira commented Aug 27, 2024 •

edited

Loading

fcostaoliveira commented Aug 27, 2024 •

edited

Loading