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.

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

Integrating fast_float with Valkey to replace strtod invocation #1260

Open
wants to merge 48 commits into
base: unstable
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
42c1d0c
feat (deps): adds fast_float library and a c wrapper for from_chars f…
swaingotnochill Oct 15, 2024
d25e29b
Changes to complete the integration of fast_float into Redis codebase.
parthpatel Nov 5, 2024
939a766
Fixing code-style issue due to empty space
parthpatel Nov 5, 2024
c2d6ff6
Adding 32-bit c++ libraries to library path to fix 32-bit compilation…
parthpatel Nov 5, 2024
597af83
Trying another approach to fix the CI workflows by installing require…
parthpatel Nov 5, 2024
8d07687
Adding -v to linker for more output to debug issues.
parthpatel Nov 5, 2024
4f03150
32-bit stdc++ libraries are in a different folder, so needed to speci…
parthpatel Nov 5, 2024
e9e5209
Also fixing the library to be non-cross version of libstdc++
parthpatel Nov 5, 2024
d7c72cf
This attempt uses the cross-compile libraries provided by ubuntu for …
parthpatel Nov 5, 2024
d69ec21
Forwarding CFLAGS and LDFLAGS to fast_float Makefile to honor 32 bit …
parthpatel Nov 5, 2024
50abf50
Putting LDFLAGS in quotes to escape spaces and /
parthpatel Nov 5, 2024
c0ec94f
More fixes
parthpatel Nov 6, 2024
6900219
Removing unnecessary library path declaration
parthpatel Nov 6, 2024
63a2f42
Including all the headers from fast_float project under a directory f…
parthpatel Nov 6, 2024
ba9c99a
Adding back the code to set errno when conversion fails but reducing …
parthpatel Nov 6, 2024
1471233
Update deps/fast_float/Makefile
parthpatel Nov 7, 2024
87654b0
Update src/Makefile
parthpatel Nov 7, 2024
9f1b352
Replacing strtod in util.c file as well.
parthpatel Nov 7, 2024
3048954
Made fast_float an optional library for build. Turning it off by defa…
parthpatel Nov 11, 2024
0941aec
Cleaning up README and adding license information
parthpatel Nov 11, 2024
a92d636
Fixing formatting issues as per Clang-format output
parthpatel Nov 11, 2024
adb46cd
Fixing more formatting issues
parthpatel Nov 11, 2024
ce38546
Move CXX to c++ instead of g++
parthpatel Nov 11, 2024
ac3af73
Refactoring to simplify things
parthpatel Nov 11, 2024
ad0f785
Moving the fast_float integration code into src/fast_float
parthpatel Nov 12, 2024
ad93430
Consolidating all ifdefs into fast_float integration header by using …
parthpatel Nov 12, 2024
8357b03
Forgot to remove ifdefs around header files. This commit fixes it.
parthpatel Nov 12, 2024
f7efd98
Taking care of warnings
parthpatel Nov 12, 2024
87f979f
Fixing more CI related issues. Making build work with both fast_float…
parthpatel Nov 12, 2024
5d3268f
Fixed formatting issues from clang-format
parthpatel Nov 12, 2024
5bb9606
Merge branch 'valkey-io:unstable' into unstable
parthpatel Nov 12, 2024
05e12e9
Update src/Makefile
parthpatel Nov 12, 2024
d66bd37
Update src/fast_float/fast_float_strtod.h
parthpatel Nov 12, 2024
7280cb3
Consolidating all fast_float code into a single if block inside Makefile
parthpatel Nov 13, 2024
c99a80c
Combining Makefile and moving unit test to src/unit directory.
parthpatel Nov 13, 2024
ee1d369
Fixed unit tests and build
parthpatel Nov 13, 2024
f42e9c6
Add QUIET_CXX definition in Makefile.
parthpatel Nov 13, 2024
5be910d
Moving the valkey_strtod.* files back to deps folder.
parthpatel Nov 13, 2024
acb10e6
Merge branch 'valkey-io:unstable' into unstable
parthpatel Nov 14, 2024
8c1ab28
Update deps/Makefile
madolson Nov 14, 2024
2050389
Update deps/README.md
parthpatel Nov 14, 2024
836bfdb
Fixing Makefiles and CI workflows
parthpatel Nov 14, 2024
9450ebf
Fixing a few naming and compilation issues.
parthpatel Nov 15, 2024
9d681ac
Merge branch 'valkey-io:unstable' into unstable
parthpatel Nov 16, 2024
a69568f
Applying yamlfmt suggestions
parthpatel Nov 16, 2024
2cb45b5
Fixing 32-bit build by setting appropriate variables in fast_float_c_…
parthpatel Nov 16, 2024
129fdbf
Fixing Makefile related issues failing CI
parthpatel Nov 16, 2024
43696b5
Remove some unnecessary imports and use SPDX for license
madolson Nov 18, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ jobs:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: make
run: |
sudo apt-get update && sudo apt-get install libc6-dev-i386
sudo apt-get update && sudo apt-get install libc6-dev-i386 libstdc++-11-dev-i386-cross gcc-multilib g++-multilib
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to add this C++ stuff in jobs where we don't use USE_FAST_FLOAT=yes? It looks like a mistake.

I don't see that we use USE_FAST_FLOAT=yes anywhere in the CI job. I think we should use it in at least one of the CI test jobs. I suggest we use it in test-ubuntu-latest, or maybe in most of the jobs but not all.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is old commit. I will add a separate job.

make -j4 SERVER_CFLAGS='-Werror' 32bit

build-libc-malloc:
Expand All @@ -109,7 +109,7 @@ jobs:

- name: make
run: |
dnf -y install epel-release gcc make procps-ng which
dnf -y install epel-release gcc gcc-c++ make procps-ng which
make -j4 SERVER_CFLAGS='-Werror'

format-yaml:
Expand Down
14 changes: 14 additions & 0 deletions deps/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ should be provided by the operating system.
* **linenoise** is a readline replacement. It is developed by the same authors of Valkey but is managed as a separated project and updated as needed.
* **lua** is Lua 5.1 with minor changes for security and additional libraries.
* **hdr_histogram** Used for per-command latency tracking histograms.
* **fast_float** is a replacement for strtod to convert strings to floats efficiently.

How to upgrade the above dependencies
===
Expand Down Expand Up @@ -105,3 +106,16 @@ We use a customized version based on master branch commit e4448cf6d1cd08fff51981
2. Copy updated files from newer version onto files in /hdr_histogram.
3. Apply the changes from 1 above to the updated files.

fast_float
---
The fast_float library provides fast header-only implementations for the C++ from_chars
functions for `float` and `double` types as well as integer types. These functions convert ASCII strings representing decimal values (e.g., `1.3e10`) into binary types. We provide exact rounding (including
round to even). In our experience, these `fast_float` functions many times faster than comparable number-parsing functions from existing C++ standard libraries.
parthpatel marked this conversation as resolved.
Show resolved Hide resolved

Specifically, `fast_float` provides the following two functions to parse floating-point numbers with a C++17-like syntax (the library itself only requires C++11):
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem relevant.

But what I think is very important to mention is the distinction between the external dependency and our integration code. That was the source of this long debate.

Suggested change
Specifically, `fast_float` provides the following two functions to parse floating-point numbers with a C++17-like syntax (the library itself only requires C++11):
The `fast_float` directory contains the C++ header file from the fast float project.
The `fast_float_integration` directory contains code to be able to use fast float from C. The integration code is written for Valkey. It's not from the fast float project.


To upgrade the library,
1. Check out https://github.com/fastfloat/fast_float/tree/main
2. cd fast_float
3. Invoke "python3 ./script/amalgamate.py --output fast_float.h"
4. Copy fast_float.h file to "deps/fast_float/".
Loading
Loading