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.

Sign up for GitHub

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

Jump to bottom

changed addReplyHumanLongDouble to addReplyDouble in georadiusGeneric and geoposCommand #13494

Merged
merged 1 commit into from
Sep 3, 2024
Merged

changed addReplyHumanLongDouble to addReplyDouble in georadiusGeneric and geoposCommand #13494

merged 1 commit into from
Sep 3, 2024

Conversation

fcostaoliveira
Copy link
Collaborator

@fcostaoliveira fcostaoliveira commented Aug 27, 2024 •
edited
Loading

Summary

TODO:

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

@fcostaoliveira fcostaoliveira added the action:run-benchmark Triggers the benchmark suite for this Pull Request label Aug 27, 2024
@fcostaoliveira
Copy link
Collaborator Author

fcostaoliveira commented Aug 27, 2024 •
edited
Loading

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
Copy link
Collaborator Author

fcostaoliveira commented Aug 27, 2024 •
edited
Loading

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
Copy link
Collaborator Author

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
Copy link
Collaborator

sundb commented Aug 28, 2024

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

@sundb sundb added the release-notes indication that this issue needs to be mentioned in the release notes label Aug 28, 2024
@fcostaoliveira fcostaoliveira requested a review from sundb August 29, 2024 08:29
oranagra
oranagra reviewed Sep 2, 2024
View reviewed changes
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

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
Copy link
Collaborator Author

@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
Copy link
Member

oranagra commented Sep 3, 2024

Ok. Fine with me

@sundb sundb merged commit fb8755a into redis:unstable Sep 3, 2024
14 checks passed
@YaacovHazan YaacovHazan mentioned this pull request Sep 11, 2024
Merged
YaacovHazan added a commit that referenced this pull request Sep 12, 2024
@YaacovHazan
Redis 8.0 M01 (#13538)
5fe3e74 
### 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.
@fcostaoliveira fcostaoliveira mentioned this pull request Sep 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oranagra oranagra oranagra left review comments

@sundb sundb sundb approved these changes

Assignees
No one assigned
Labels
action:run-benchmark Triggers the benchmark suite for this Pull Request release-notes indication that this issue needs to be mentioned in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fcostaoliveira @sundb @oranagra