jk-jeon / dragonbox

Reference implementation of Dragonbox in C++
Apache License 2.0
588 stars 37 forks source link

Simpler "reference implementation" #64

Closed jk-jeon closed 1 week ago

jk-jeon commented 2 months ago

The current implementation has grown too much to a point where it is nearly impossible to understand for people with not so much exposure to template black magics and other C++-specific nonsenses. The complication of the implementation can act as a significant barrier for people wanting to port the algorithm into languages other than C++, or even into C++, even though the algorithm itself is not that overcomplicated.

Those templates and other nonsenses are done for reasons, to make this library general and fast enough, but at the same time since this is the reference implementation, it should serve the role as such, i.e., to provide guidance to other people wanting to write their own implementations. Hence, I think it makes sense to provide the core algorithm written in simpler, non-esoteric C++ (or maybe even C99?) that everybody can understand.

To be clear, I do not intend to replace the current implementation with a simpler one, which I consider impossible without losing many utilities and performance. As pointed out earlier, those template hackeries are done for reasons. Instead, what I want to have is a standalone implementation of the algorithm that only serves the expository role, which does not need to be optimized-to-hell, rather just enough to demonstrate the algorithm's potential. There already is an implementation which can serve such a role, done by Alexander Bolz, but (1) there is no implementation for IEEE-754 binary32, (2) his implementation is quite outdated at this moment as the algorithm has been improved quite a lot since then, and (3) I think it's nicer to have one in this official repository for the reference implementation, kept in sync with whatever further improvements we bring in the future.

I may work on this someday if no one helps me, but PR is very welcome if anyone is interested!

tobybell commented 1 month ago

I’ve started working on this in my spare time. If someone wants to collaborate, leave a comment.

My general method will be to work backwards from the current implementation, aiming to simplify it internally while mostly preserving the external API and behavior of the test/verification executables in the repo.

I’m aiming to simplify the code based on the following assumptions:

Will also aim to minimize use of templates (not eliminate, just condense).

Will submit a PR once I have a first draft.

jk-jeon commented 1 month ago

@tobybell Thanks a lot for working on it!

I think it's better to have it a different symbol name at least, like having it on a different namespace, like jkj::smplfd_dragonbox. In that way we can keep the original implementation with the new one in the same executable, so that we don't need to write a new test for the new implementation.

I had a quick look at my test code, and it seems to me that the only tests that are relevant to the new implementation are these two. (Other tests are not unit tests nor the integrated test, rather they are just verifying some mathematical claims.)

Currently, these tests are directly calling jkj::dragonbox::to_chars, with forwarded policy parameters passed by main, but we can instead change it so that main just passes a lambda calling the jkj::dragonbox::to_chars with policy parameters already specified. In that case, we can simply plug-in the test code for the simplified implementation there by adding a new call in main with a lambda calling the new implementation.

Ideally, it would be great to throw away all policy shenanigans and just stick to the default ones. conversion_traits and nonsense like that too. Since we don't care about constexpr and also exotic floating-point types as you pointed out, we can just use std::memcpy and there is no need to introduce an additional layer of abstraction. I think std::bit_cast is also fine (as there is little reason to keep the simplified implementation C++11-compatible), but probably std::memcpy is better as it's more widely understood among people.

But anyway, we can start from something more minimal in terms of the divergence from the original implementation, so please feel free to ignore what I said for a while. Again, thanks a lot!

jk-jeon commented 1 week ago

Done, thanks to @tobybell. Further simplification may be made afterwards.