google / wuffs

Wrangling Untrusted File Formats Safely
Other
4.06k stars 129 forks source link

optimize SDC #90

Closed sarastro-nl closed 1 year ago

sarastro-nl commented 1 year ago

So you once wrote about SDC:

It's not the fastest or cleverest algorithm, but a rarely-invoked fallback doesn't have to be, and there is value in simplicity.

I couldn't agree more, so I propose a very small change which makes the code even simpler but at the same moment saves a lot of calls to the 'expensive' lshift and rshift calls making the algorithm also faster as a bonus.

I do realize that 'simpler' is a bit of a personal judgement, but I think I may have a case because of these arguments:

Test results:

I ran the std/json tests with my change and made sure SDC is always used, so I temporarily disabled Eisel-Lemire and the code when the number is exactly representable by a double. The result was that it saved 73 calls to lshift and 32 calls to rshift.

About the change itself:

The change makes sure that once the lshift and rshift loops are done the decimal_point is either 0 or 1 (so the value is the range [0.1 .. 10]). It then looks at the digits to calculate the final lshift. If the number happens to be already in [1 .. 2] then the final lshift is just 52. This calculated shift will be between 56 (for values in the range [0.1 .. 0.125] and 49 (for values in the range [8 .. 10]).

Another very small change is this: the same entry from the powers table is used for the lshift and rshift. This entry is chosen in such a way that if the value is divided by (2 ** entry) you'll end up in [0.1 .. 2]. And since this is within [0.1 .. 10] it's the ideal candidate for the rshift. But the lshift does the opposite so with that same entry you will always end up in the range [0.05 .. 1]. Not good. So if you just add 1 to the entry the range will be [0.1 .. 2] again which is exactly within the target range. This simple addition alone will save needless calls to lshift.

nigeltao commented 1 year ago

Thanks. As previously mentioned (https://github.com/google/wuffs/pull/87#issuecomment-1321004504), Wuffs is in the "release candidate" stage of the version 0.3 release cycle, so I'll hold off on merging any performance changes (as opposed to bug-fixing changes). This PR will have to wait until version 0.4 opens. I'll ping this discussion after that happens.