Use Slice.literal for fast_float when supported#15667
Use Slice.literal for fast_float when supported#15667straight-shoota merged 2 commits intocrystal-lang:masterfrom
Slice.literal for fast_float when supported#15667Conversation
|
The code (or rather: data) duplication is a bit concerning. The data isn't expected to change much (or at all), so I suppose maintenance shouldn't be much effort. We still might want to add a code comment point out the duplication and its reasoning, together with a TODO note for when we can drop the array version. |
| module Float::FastFloat | ||
| module Powers | ||
| POWER_OF_FIVE_128 = Slice(UInt64).literal( | ||
| 0xeef453d6923bd65a_u64, 0x113faa2906a13b3f_u64, |
There was a problem hiding this comment.
question: Theoretically, the type suffixes are unnecessary because the entire literal is typed to UInt64. Technically, we need suffixes for literals that don't fit into Int64 until we have actual literals with type-aware parsing (#2886).
Is there any specific reason to keep the suffixes for smaller number literals though? (except for keeping the transformation simple, of course).
There was a problem hiding this comment.
There isn't, the method call form will not require suffixes for these smaller integer types (such as the slices used for Unicode data)
| put(array, 0x8e679c2f5e44ff8f_u64); put(array, 0x570f09eaa7ea7648_u64) | ||
| array | ||
| end | ||
| {% if compare_versions(Crystal::VERSION, "1.16.0") < 0 %} |
There was a problem hiding this comment.
suggestion: Slice.literal is available since 1.10.0 in non-interpreted mode.
I presume the literal variant is generally better and we might want to use it whenever possible?
What about expanding the condition to compare_versions(Crystal::VERSION, "1.10.0") < 0 || ( flag?(:interpreted) && compare_versions(Crystal::VERSION, "1.16.0") < 0 )?
There was a problem hiding this comment.
Critical bugs like #15009 mean the actual lower bound is higher than 1.10 even for non-interpreted code, and also we could bump this version number if, say, we switch to a collection literal form that obviously 1.16.0 doesn't support. So I would leave this version check as-is and not worry about it too much
There was a problem hiding this comment.
Shouldn't it be in another file, so the compiler wouldn't even consider all of this, or is the macro if enough to tell the compiler to skip the macro contents?
There was a problem hiding this comment.
The parser reads the macro body as a MacroLiteral. It's only parsed as Crystal source after expansion. So this should be relatively inexpensive. It allocates a huge string, though.
There was a problem hiding this comment.
Moving both tables to separate files is straightforward for ECR-generated tables (e.g. Unicode)
There was a problem hiding this comment.
thought: I'm wondering about the separation into different files. I suppose it makes sense to keep individual files from growing too large.
But eventually we'll want to drop the array workaround, and then fast_table.cr would just declare three constants and include the slice literal file.
If that's the intended state, I appreciate the idea of outsourcing the entire slice literal to a separate file to keep it isolated.
ysbaddaden
left a comment
ysbaddaden
left a comment
There was a problem hiding this comment.
This is lovely, and definitely the direction we must go 😍
My only concern is related to the literal syntax: will it lead to have 3 versions of the same data? Will it replace this duplication to use the new form? Would it be better to introduce the syntax in the next release for example, and wait for the following release to start introducing the duplication?
|
I don't see any reason why we should not already start using the features that are available right now. When we get further enhancements in the literal syntax we should update the literal code accordingly, and increase the required version number. So 1.16 will eventually fall back to use array puts. But that's fine. |
straight-shoota
changed the title
Slice.literal for fast_float when supported
3 participants
A compiler version of 1.16.0 or above indicates support for slice literals, including in the interpreter, so we could start switching to them and dropping the old dynamic array initializers.
This is of course not the largest lookup table in the standard library, so the build time and binary size savings are rather limited. This table is chosen to give a taste of what's possible in subsequent patches.