Faster access for configurables #6058
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xunilrj
force-pushed
the
xunilrj/configurables-faster-access
branch
from
dc4306e
to
18d2ef8
Compare
Benchmark for 6871543Click to view benchmark
|
Benchmark for b1f8679Click to view benchmark
|
Benchmark for 791f062Click to view benchmark
|
Benchmark for 5e71998Click to view benchmark
|
Benchmark for e07caafClick to view benchmark
|
Benchmark for ddb3e5eClick to view benchmark
|
xunilrj
force-pushed
the
xunilrj/configurables-faster-access
branch
from
f941425
to
58b20cd
Compare
Benchmark for 3ce7a03Click to view benchmark
|
xunilrj
force-pushed
the
xunilrj/configurables-faster-access
branch
from
58b20cd
to
745b813
Compare
Benchmark for 5288600Click to view benchmark
|
Benchmark for 6de0df3Click to view benchmark
|
IGI-111
reviewed
May 30, 2024
IGI-111
reviewed
May 30, 2024
IGI-111
reviewed
May 30, 2024
IGI-111
reviewed
May 30, 2024
Benchmark for 78807b1Click to view benchmark
|
xunilrj
force-pushed
the
xunilrj/configurables-faster-access
branch
from
ab23d8d
to
858cfcc
Compare
tritao
reviewed
May 31, 2024
tritao
reviewed
May 31, 2024
tritao
reviewed
May 31, 2024
Benchmark for 6a24095Click to view benchmark
|
tritao
reviewed
May 31, 2024
tritao
previously approved these changes
May 31, 2024
xunilrj
force-pushed
the
xunilrj/configurables-faster-access
branch
from
1789030
to
a71997b
Compare
Benchmark for 4482f5bClick to view benchmark
|
Benchmark for eb64706Click to view benchmark
|
sdankel
approved these changes
Jun 10, 2024
Benchmark for f104691Click to view benchmark
|
IGI-111
approved these changes
Jun 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR revamps how configurables are accessed when encoding v1 is used. Any changes for configurables with encoding v0 should be treated as bugs.
After encoding V1 was merged, every single time you accessed a configurable, we would "decode it". Apart from obvious performance problems, this also introduced semantic problems: two different references, would not point to the same memory address, which is something one would expect from sway syntax.
This was addressed in four steps:
1 - Configurable lowering to IR now comes with more data, The IR below is for a simple
u8configurable namedU8.1.1 - config name comes from the configurable itself (before it was auto-named as
c0,c1etc...);1.2 - config type correctly follows the configurable type (after encoding v1, it was always byte slice);
1.3 - there is a new decode function that will be called to initialize this configurable (more details below);
1.4 - at last, we have the encoded bytes of the configurable value.
2 - This PR also splits
TyConfigurableDeclfromTyConstantDecl, andConfigurabletExpressionfromConfigurableExpressions.This is needed because we now compile
ConfigurableExpressionto a new IR instruction calledget_config, which is similar toget_local. So a reference to aconfigurablewill be compiled into:3 - Given that
configs are not writeable, and there is no way to set their value in IR, this is done when we lower IR to ASM, more specifically we are using a new ASM section called "globals", which are the writeable counterpart of the read-only data section.The physical difference is that the read-only data section is appended at the end of the final binary which puts all the data sections outside of the writeable memory; To circumvent this and enable some globals to be writeable, we need to allocate them in the stack (the only writeable memory inside FuelVM). So, as soon as possible we allocate space for all globals with
cfei, but we do not initialize them in any way, shape or form. We cannot expect their value to be zero!In one of our tests, this is the allocation that is generated:
After this, for each
config, the following virtual ASM is appended to the prologue (before the__entryfn).There are two interesting parts here:
3.1 - There is a new
addrvirtual ASM opcode that returns the address of an item in the data section, in this case, it is theconfigencoded bytes.3.2 - The address where the
configlives in runtime is calculated at compile time and it is represented asaddi $$arg2 $ssp i0. In this case, the firstconfigis at$ssp + 0. The second one will packed after the first, and so on. They are NOT aligned.The final FuelVM bytecode is as packed as one could expect:
Major code changes
1 - I had to split Constant and Configurable. This happened in both AST and the typed tree.
2 - I completely removed
Configurableand a possible IR value. As a matter of fact, configurables do not exist in IR. We do have the concept ofconfig, which only exists in two forms: theconfigdeclaration, and theget_configinstruction.3 - I moved the folder "programs" to inside "fuel" because they were not being used for other targets. I have also move the abstraction of targets away from them to the function
compile_ir_context_to_finalized_asm. So the lowering from IR to asm is generic on the target as before, but the intermediary data are specific to each target.The only "leak" is
InstructionSet, which still is aenumwith a variant for each target. I have changed this as this PR does not need it. But I think would make sense for its parent structureFinalizedAsm, to be target ignorant.4 - I have also improved bytecode printing as shown above. Not we print the FuelVM instruction, registers and arguments, together with the binary in a more readable form. Together with absolute address.
5 - Up until now, all the compilation of
swayto IR was done from the entry function. Any code not reachable from here would not be in the final binary. That was also true for how the suffix "_0", "_1" etc... was inserted. They were chosen as these functions were being called.This created a problem because the configurable initialization was not necessarily also called in normal sway code. For this, I started to also use the
configdecode fn field as entry functions. To avoid these two generating colliding suffixes, I move the shared code intoCompiledFunctionCache. This "cache" is something that we should revisit soon, but I left it untouched for now.Internal Debugging
We have some very hard to debug algorithms. I tend to leave some commented code when I find a good place and a good "print" that helps to debug these parts. For example:
We may want to find a better solution for this. Maybe a combination of cargo feature and environment variable that prints these debugging snippets.
I am fine with removing them for now, but I think they would be very helpful, not only for debug, but also for snapshot tests in the future.
Binary Size
Since the introduction of encoding v1 for configurable, the binary size of
test/src/e2e_vm_tests/test_programs/should_pass/language/configurable_constshas exploded. Before it was around 5k, and skyrocketed to 14k. This PR alleviates this a little bit, and brings it back to 10k.Checklist
Breaking*orNew Featurelabels where relevant.