New version of execution_result #219

axic · 2020-02-28T16:50:50Z

This re-implements the execution_result struct. Now it has three valid
states: value, void and trap. The struct is simplified, close to POD
type, std::vector is not used any more.

In benchmarks for certain cases (such as ecpairing) deleting an std::vector took quite a big chunk of time. I thought perhaps the moved stack for the return value could be the culprit, hence did this change. However my local benchmarks are inconclusive.

chfast · 2020-02-28T19:20:30Z

Benchmarking before cepairing landing does not make sense then.

axic · 2020-02-28T19:57:19Z

Benchmarking before cepairing landing does not make sense then.

Hm? It is in the bench-ecpairing branch.

axic · 2020-04-24T11:09:13Z

Rebased so we can run another set of benchmark comparison post 0.1.0.

lib/fizzy/execute.cpp

codecov · 2020-04-24T11:14:48Z

Codecov Report

Merging #219 into master will increase coverage by 0.11%.
The diff coverage is 94.33%.

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
+ Coverage   99.17%   99.28%   +0.11%     
==========================================
  Files          49       49              
  Lines       13237    13227      -10     
==========================================
+ Hits        13128    13133       +5     
+ Misses        109       94      -15

axic · 2020-06-01T16:38:14Z

I think this can be revived in a simplified way (e.g. only affected "public api") once the span changes are pushed.

chfast

There is a problem with struct layout of bool + std::optional<uint64_t>. In whatever order you put these the end struct size will be 24 bytes. Therefore it cannot be returned by registers from a function.

We should not use std::optional<uint64_t>, just make flat struct as

struct execution_result
{
    bool trapped;
    bool has_value;
    uint64_t value;
};

https://godbolt.org/z/m3GQsE

axic · 2020-06-09T09:28:30Z

struct execution_result
{
    bool trapped;
    bool has_value;
    uint64_t value;
};

Would an enum make sense?

enum class execution_status
{
   Trapped,
   WithResult,
   WithoutResult,
}
struct execution_result
{
    execution_status status;
    uint64_t value;
};

chfast · 2020-06-09T10:19:39Z

Would an enum make sense?

Yes, performance wise this should be the same.

Although, you will probably need additional helpers like result.trapped() and make the enum "private".

chfast · 2020-06-09T11:18:32Z

Also there is Result in WasmEngine, so unify them.

    struct Result
    {
        bool trapped = false;
        std::optional<uint64_t> value;
    };

chfast · 2020-07-14T12:23:57Z

test/spectests/spectests.cpp

                            pass();
                        else
                            fail("Unexpected returned value.");
                        continue;
                    }

-                    if (result->stack.size() != 1)
+                    if (!result->has_value)


Bummer, actually should have removed this part of the code with #398.

lib/fizzy/execute.hpp

chfast · 2020-07-15T20:20:45Z

Benchmarks without "Simplify trap handling".
GCC10, Haswell 4.0 GHz

Comparing master to execres
Benchmark                                                            Time             CPU      Time Old      Time New       CPU Old       CPU New
-------------------------------------------------------------------------------------------------------------------------------------------------
fizzy/execute/blake2b/512_bytes_rounds_1_mean                     -0.1125         -0.1125            87            77            87            77
fizzy/execute/blake2b/512_bytes_rounds_16_mean                    -0.1232         -0.1232          1307          1146          1307          1146
fizzy/execute/ecpairing/onepoint_mean                             -0.1092         -0.1092        435697        388114        435698        388117
fizzy/execute/keccak256/512_bytes_rounds_1_mean                   -0.0827         -0.0827           104            95           104            95
fizzy/execute/keccak256/512_bytes_rounds_16_mean                  -0.0838         -0.0838          1516          1388          1516          1389
fizzy/execute/memset/256_bytes_mean                               -0.1183         -0.1183             7             6             7             6
fizzy/execute/memset/60000_bytes_mean                             -0.1095         -0.1095          1564          1393          1564          1393
fizzy/execute/mul256_opt0/input0_mean                             -0.0695         -0.0695            27            25            27            25
fizzy/execute/mul256_opt0/input1_mean                             -0.0685         -0.0685            27            25            27            25
fizzy/execute/sha1/512_bytes_rounds_1_mean                        -0.0862         -0.0862            93            85            93            85
fizzy/execute/sha1/512_bytes_rounds_16_mean                       -0.0845         -0.0845          1288          1179          1288          1179
fizzy/execute/sha256/512_bytes_rounds_1_mean                      -0.0915         -0.0915            95            86            95            86
fizzy/execute/sha256/512_bytes_rounds_16_mean                     -0.0872         -0.0872          1299          1186          1299          1186
fizzy/execute/micro/eli_interpreter/halt_mean                     -0.2347         -0.2347             0             0             0             0
fizzy/execute/micro/eli_interpreter/exec105_mean                  -0.1350         -0.1350             5             4             5             4
fizzy/execute/micro/factorial/10_mean                             -0.2747         -0.2747             1             0             1             0
fizzy/execute/micro/factorial/20_mean                             -0.2645         -0.2645             1             1             1             1
fizzy/execute/micro/fibonacci/24_mean                             -0.2688         -0.2688          9929          7260          9929          7260
fizzy/execute/micro/host_adler32/1_mean                           -0.3320         -0.3320             0             0             0             0
fizzy/execute/micro/host_adler32/100_mean                         -0.3484         -0.3484             5             3             5             3
fizzy/execute/micro/host_adler32/1000_mean                        -0.3510         -0.3510            45            29            45            29
fizzy/execute/micro/spinner/1_mean                                -0.0660         -0.0663             0             0             0             0
fizzy/execute/micro/spinner/1000_mean                             -0.1597         -0.1597            10             9            10             9

The "Simplify trap handling" does not change the results.

axic · 2020-07-15T21:32:21Z

test/unittests/instantiate_test.cpp

@@ -18,7 +18,7 @@ uint64_t call_table_func(Instance& instance, size_t idx)
 {
    const auto& elem = (*instance.table)[idx];
    const auto res = elem->function(instance, {}, 0);
-    // ASSERT_TRUE(res.result.has_value());
+    assert(res.has_value);


Why not keeping ASSERT_TRUE? (I'm not sure why it was disabled in this PR in the first.)

Apparently the ASSERT_TRUE only works inside TEST as it use return for early exit.

Changed to EXPECT_TRUE.

gumb0 · 2020-07-16T12:54:58Z

test/unittests/instantiate_test.cpp

-    auto host_foo2 = [](Instance&, span<const uint64_t>, int) -> execution_result {
-        return {true, {}};
-    };
+    auto host_foo1 = [](Instance&, span<const uint64_t>, int) -> execution_result { return Void; };


These are not executed, but foo1 import returns a result (foo2 is void)

But both were also traps in the same time. You cannot have trap + value now.

Before: both were trapping, but foo1 supposed to return a result and foo2 not
After: foo1 does not trap and returns nothing, foo2 traps

@gumb0 is this the correct intended behaviour?

The implementation doesn't matter, I would just make them both trap.

axic · 2020-07-16T14:19:58Z

Looks good to merge apart form that single question.

And the header with trapped_tag, etc. does not particularly looks nice, but okay if it can't be made nicer.

chfast · 2020-07-16T14:45:18Z

lib/fizzy/execute.hpp

    constexpr execution_result(uint64_t _value) noexcept : has_value{true}, value{_value} {}

-    constexpr explicit execution_result(trapped_tag) noexcept : trapped{true} {}
+    constexpr explicit execution_result(bool success) noexcept : trapped{!success} {}


This constructor should not be used by users. We can also add default value bool success = true to allow {} as previously. But rather single way of doing things in internal code is better.

This re-implements the execution_result struct. Now it has three valid states: value, void and trap. The struct is simplified, close to POD type, std::vector is not used any more. Co-authored-by: Paweł Bylica <chfast@gmail.com>

axic force-pushed the execute-return branch 2 times, most recently from 49d3278 to 401c213 Compare April 24, 2020 11:08

axic force-pushed the execute-return branch from 401c213 to bc5de8a Compare April 24, 2020 11:10

axic commented Apr 24, 2020

View reviewed changes

lib/fizzy/execute.cpp Outdated Show resolved Hide resolved

axic mentioned this pull request Jun 1, 2020

bench: Fix execution pre-validation #340

Merged

chfast requested changes Jun 9, 2020

View reviewed changes

axic force-pushed the execute-return branch 3 times, most recently from f87078e to ae85c67 Compare June 30, 2020 09:37

axic force-pushed the execute-return branch from ae85c67 to 3301eac Compare July 10, 2020 09:48

chfast force-pushed the execute-return branch 3 times, most recently from 7401072 to f7ac7d1 Compare July 14, 2020 12:20

chfast reviewed Jul 14, 2020

View reviewed changes

gumb0 reviewed Jul 14, 2020

View reviewed changes

lib/fizzy/execute.hpp Outdated Show resolved Hide resolved

axic force-pushed the execute-return branch from f7ac7d1 to 2ee8473 Compare July 14, 2020 22:18

chfast force-pushed the execute-return branch from d460a10 to 010413a Compare July 15, 2020 18:54

axic mentioned this pull request Jul 15, 2020

Simplify trap handling #425

Merged

chfast force-pushed the execute-return branch 2 times, most recently from b9b0245 to 6a4386e Compare July 15, 2020 20:41

axic commented Jul 15, 2020

View reviewed changes

chfast force-pushed the execute-return branch 3 times, most recently from aa8eca4 to 5fce948 Compare July 16, 2020 06:48

chfast changed the title ~~Change the return value of execute() from std::vector to std::optional~~ New version of execution_result Jul 16, 2020

chfast force-pushed the execute-return branch from 5fce948 to feddce5 Compare July 16, 2020 07:50

chfast marked this pull request as ready for review July 16, 2020 07:53

chfast approved these changes Jul 16, 2020

View reviewed changes

gumb0 reviewed Jul 16, 2020

View reviewed changes

gumb0 approved these changes Jul 16, 2020

View reviewed changes

chfast reviewed Jul 16, 2020

View reviewed changes

chfast force-pushed the execute-return branch from f7711d4 to 93d21f6 Compare July 16, 2020 14:55

New version of execution_result

c8e22a1

This re-implements the execution_result struct. Now it has three valid states: value, void and trap. The struct is simplified, close to POD type, std::vector is not used any more. Co-authored-by: Paweł Bylica <chfast@gmail.com>

chfast force-pushed the execute-return branch from e8f324b to c8e22a1 Compare July 16, 2020 15:24

chfast merged commit 174ad31 into master Jul 16, 2020

chfast deleted the execute-return branch July 16, 2020 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New version of execution_result #219

New version of execution_result #219

axic commented Feb 28, 2020 •

edited by chfast

Loading

chfast commented Feb 28, 2020

axic commented Feb 28, 2020

axic commented Apr 24, 2020

codecov bot commented Apr 24, 2020 •

edited

Loading

axic commented Jun 1, 2020

chfast left a comment

axic commented Jun 9, 2020

chfast commented Jun 9, 2020

chfast commented Jun 9, 2020

chfast Jul 14, 2020

axic Jul 14, 2020

chfast commented Jul 15, 2020 •

edited

Loading

axic Jul 15, 2020

chfast Jul 16, 2020

chfast Jul 16, 2020

gumb0 Jul 16, 2020

chfast Jul 16, 2020

axic Jul 16, 2020

gumb0 Jul 16, 2020

axic commented Jul 16, 2020

chfast Jul 16, 2020

New version of execution_result #219

New version of execution_result #219

Conversation

axic commented Feb 28, 2020 • edited by chfast Loading

chfast commented Feb 28, 2020

axic commented Feb 28, 2020

axic commented Apr 24, 2020

codecov bot commented Apr 24, 2020 • edited Loading

Codecov Report

axic commented Jun 1, 2020

chfast left a comment

Choose a reason for hiding this comment

axic commented Jun 9, 2020

chfast commented Jun 9, 2020

chfast commented Jun 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Jul 16, 2020

Choose a reason for hiding this comment

axic commented Feb 28, 2020 •

edited by chfast

Loading

codecov bot commented Apr 24, 2020 •

edited

Loading

chfast commented Jul 15, 2020 •

edited

Loading