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

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

Closed
filipecosta90 opened this issue Dec 1, 2022 · 5 comments
Closed

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

filipecosta90 opened this issue Dec 1, 2022 · 5 comments
Assignees
@filipecosta90 @fcostaoliveira

Comments

@filipecosta90
Copy link
Contributor

filipecosta90 commented Dec 1, 2022 •
edited
Loading

Profiling GEOPOS command we see that addReplyHumanLongDouble takes 62.36% of the total CPU cycles of the process:

Samples: 29K of event 'cycles', Event count (approx.): 109040523666
  Children      Self  Command          Shared Object      Symbol
-   62.36%     0.53%  redis-server     redis-server       [.] addReplyHumanLongDouble                                                                                                                      ◆
   - 61.83% addReplyHumanLongDouble                                                                                                                                                                        ▒
      - 59.53% createStringObjectFromLongDouble                                                                                                                                                            ▒
         - 57.06% ld2string                                                                                                                                                                                ▒
            - 55.61% snprintf (inlined)                                                                                                                                                                    ▒
               + 55.51% ___snprintf_chk (inlined)                                                                                                                                                          ▒
         + 1.76% createStringObject (inlined)                                                                                                                                                              ▒
      + 2.26% addReplyBulk (inlined)                                                                                                                                                                       ▒
   + 0.51% _start

In the past we've optimized addReplyDouble in #10587 .
Wanted to open this issue and discuss with @itamarhaber / @oranagra why we're using long double input for 2 double:

typedef struct geoPoint {
    double longitude;
    double latitude;
    double dist;
    double score;
    char *member;
} geoPoint;

@itamarhaber I wanted to double check with you given the last change made on that geopos line was 7 years ago and exacly changing for double to long double reply "Eliminates engineers near the equator & prime meridian" :

b5149f0

Do you remember exactly why we need the long double? Or can we move to a double reply?

To reproduce:

Use the following RDB with 60M datapoints on the GEO key. https://s3.us-east-2.amazonaws.com/redis.benchmarks.spec/datasets/geopoint/dump.rdb

memtier_benchmark --pipeline 10 --test-time 60 --command "GEOPOS key 1 2" --hide-histogram
@oranagra
Copy link
Member

oranagra commented Dec 1, 2022

found the PR (#3101) thought it might have some details 😞

@oranagra
Copy link
Member

oranagra commented Dec 1, 2022

i think that what Itamar was after is the "human" part (using %f instead of %g, exponent notation), and not the "long" part.

@oranagra oranagra moved this to Todo in Redis 7.2 Dec 1, 2022
@itamarhaber
Copy link
Member

Exactly, and the fact that our geohash isn't precise enough to justify so many digits. I remember doing this when I suggested the change:

addReplyDouble:

127.0.0.1:6379> GEOPOS z m
1) 1) "2.682209014892578e-6"
   2) "1.2673605809254695e-6"

addReplyHumanLongDouble

127.0.0.1:6379> GEOADD z 0 0 m
(integer) 1
127.0.0.1:6379> GEOPOS z m
1) 1) "0.00000268220901489"
   2) "0.00000126736058093"

@oranagra
Copy link
Member

looking at this again, considering the performance difference, i wonder how important the human concern is, and maybe we can set it aside.
alternatively, maybe we can modify fpconv to avoid using scientific notation (it may produce longer strings)

@oranagra oranagra removed this from Redis 7.2 Sep 28, 2023
@sundb sundb added this to Redis 8.0 Aug 19, 2024
@sundb sundb removed this from Redis 7.4 Aug 19, 2024
@sundb sundb moved this from Todo to In Progress in Redis 8.0 Aug 19, 2024
@fcostaoliveira fcostaoliveira mentioned this issue Aug 27, 2024
Merged
2 tasks
sundb pushed a commit that referenced this issue Sep 3, 2024
@fcostaoliveira
changed addReplyHumanLongDouble to addReplyDouble in georadiusGeneric…
fb8755a 
… and geoposCommand (#13494)

# Summary 

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

This was addressed in PR #13494 and is already part of https://github.com/redis/redis/releases/tag/8.0-m01

@github-project-automation github-project-automation bot moved this from In Progress to Done in Redis 8.0 Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@filipecosta90 filipecosta90

@fcostaoliveira fcostaoliveira

Labels
None yet
Projects
Redis 8.0
Status: Done
Milestone
No milestone
Development

No branches or pull requests
4 participants
@filipecosta90 @itamarhaber @oranagra @fcostaoliveira