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 https://github.com/redis/redis/pull/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" :
https://github.com/redis/redis/commit/b5149f08684b55a11b158e12770d35d78d44b2f4
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
Comment From: oranagra
found the PR (#3101) thought it might have some details :disappointed:
Comment From: oranagra
i think that what Itamar was after is the "human" part (using %f instead of %g, exponent notation), and not the "long" part.
Comment From: itamarhaber
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"
Comment From: oranagra
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)
Comment From: fcostaoliveira
This was addressed in PR #13494 and is already part of https://github.com/redis/redis/releases/tag/8.0-m01