Migrate fc::static_variant to std::variant #9235

TimothyBanks · 2020-06-22T19:09:00Z

Change Description

This PR is to replace the use of fc::static_variant with std::variant. Based on performance metrics, std::variant is the better alternative.

Jira: https://blockone.atlassian.net/browse/EPE-17
Jira issue has links to benchmarks describing performance improvement of std::variant over fc::static_variant.

Change Type

Select ONE

Documentation

Stability bug fix

Other

Other - special case

Consensus Changes

Consensus Changes

API Changes

API Changes

Documentation Additions

Documentation Additions

heifner · 2020-06-22T19:21:10Z

Can we get a better description on this PR please. The standard template should be used. Specifically the check box sections are needed.
Also, when we stop using JIRA again we will have no information on this change.

TimothyBanks · 2020-06-23T20:39:02Z

This PR should be ready for review now.

I am currently unsure which boxes need to be check in the description.

allenhan2

std::get throws different exception from fc::static_variants, please check the exception handling has been updated or added handling for that.
exception handling is my biggest concern, and not much code coverage there. It may change the error handling path and error log printing.

libraries/chain/include/eosio/chain/abi_serializer.hpp

libraries/fc/include/fc/static_variant.hpp

libraries/fc/test/static_variant/test_static_variant.cpp

libraries/chain/transaction.cpp

libraries/fc/include/fc/static_variant.hpp

libraries/fc/include/fc/rpc/json_connection.hpp

TimothyBanks · 2020-06-25T18:07:05Z

PR is updated.

TimothyBanks · 2020-06-25T18:37:48Z

I'm open to suggestions as to what to call it. Or I can just call it get.

libraries/fc/include/fc/static_variant.hpp

allenhan2

std::visit has similar issue as std::get,
fc::static_variant throws fc exception, std::visit throws std::bad_variant_access.

Also, std::get, if it is not guarded with check, please use fc::get, so when people copy the code or make change later on, std::get won't be miss-used without checking.

libraries/chain/controller.cpp

libraries/chain/include/eosio/chain/authority.hpp

libraries/chain/include/eosio/chain/block.hpp

libraries/chain/include/eosio/chain/producer_schedule.hpp

TimothyBanks · 2020-06-26T18:38:39Z

Can you please show me where this new exception type is going to cause problems in the system? I'm adding overhead to the use of std::variant just to maintain throwing the same exception which seems a bit overkill. In the case of using std::get, I thought it was a good compromise to invoke std::get_if and if that fails, then throw a new exception. In the case of std::visit, I don't have a conditional equivalent and I don't like adding the overhead of another try/catch block just to repackage the exception that is bubbling out.

I would much prefer to update the parts of the system that are expecting this exact fc exception message to be thrown and move them over to the std equivalent.

allenhan2 · 2020-06-26T18:48:09Z

Can you please show me where this new exception type is going to cause problems in the system? I'm adding overhead to the use of std::variant just to maintain throwing the same exception which seems a bit overkill. In the case of using std::get, I thought it was a good compromise to invoke std::get_if and if that fails, then throw a new exception. In the case of std::visit, I don't have a conditional equivalent and I don't like adding the overhead of another try/catch block just to repackage the exception that is bubbling out.

I would much prefer to update the parts of the system that are expecting this exact fc exception message to be thrown and move them over to the std equivalent.

Appreciate your concern on the performance impact, however, logic correctness triumphs speed before we have good test coverage. The change should not jeopardize the error handling, which seems to me noway to verify.
For std::get, if it is not guarded with std::holds_alternative, please use fc::get, which will throw fc exception instead of std::exception.

heifner · 2020-06-26T19:33:06Z

I think if you eval everywhere std::variant is used and make sure exception handling does not introduce a change in behavior, then it would be really nice to complete remove dependency on anything in fc.

TimothyBanks · 2020-06-26T19:41:24Z

A quick survey of EOS (and submodules) shows only a couple of places that we explicitly catch the fc::assert_exception. Everywhere else is within tests. Are there any other places that I could check? Or is this too ignorant of an evaluation?

I guess you could be catching on a base type as well.

TimothyBanks · 2020-06-29T15:05:12Z

static_variant has been removed with the exception of a file named static_variant. It currently only holds functionality that is needed by EOS that isn't available with std::variant.

As for tracking down all code paths, I think everyone needs to come to an agreement on what is acceptable and what the path forward should be. @larryk85 Could you please chime in on this was well.

TimothyBanks · 2020-07-08T17:30:37Z

Could we move the exception part into another JIRA issue and push this PR in?

TimothyBanks · 2020-07-16T17:25:15Z

Updated.

TimothyBanks · 2020-07-20T15:04:42Z

@heifner @allenhan2 Please review again.

libraries/chain/controller.cpp

libraries/chain/apply_context.cpp

heifner · 2020-07-30T18:00:27Z

libraries/chain/apply_context.cpp

      throw;
+   } catch( const fc::exception& e ) {
+      handle_exception(e);
+   } catch ( const std::exception& e ) {


Why was catch of std::exception added here? Do we really want all std::exceptions to be ignored? Either this is not needed OR this is a consensus change since nodes without this code will fail transaction differently.

Looking at this more, it is not necessarily a consensus change since std::variant can throw. My concern would be the use of something like std::vector::at() or some other throw of std::exception that would be handled differently now.

An argument can be made that exceptions from something like std::vector::at() should be captured here as you have it. However, this could potentially be a consensus change. I'll leave it up to @b1bart to indicate if he is comfortable with this change. I have NOT reviewed all exception code paths to determine how safe this is. This comment goes for all the exception handling changes in this PR (not just here).

Would it be better to catch just the specific std errors being thrown by variant and optional? I can change the std::exception handler to std::bad_variant_access and whatever specific std exceptions that are thrown.

I think what you have is likely fine. I would only argue that someone does a thorough review of possible exceptions in code paths where this was added. I suspect there are not any other std exceptions we really need to worry about. So just need someone to review and give approval.

heifner · 2020-07-30T18:01:45Z

I think all the exception changes should be pulled out into a separate PR. They are very dangerous and deserve their own PR. If needed for the other changes then make sure it is called out in the PR description.

…od that will throw fc exception if variant does not currently hold requested type. Revived the static_variant tests.

…ceptions

TimothyBanks · 2020-08-24T17:15:17Z

Replay was successful.
https://buildkite.com/EOSIO/auto-eosio-full-replay-test/builds/245#_
https://buildkite.com/EOSIO/auto-eosio-replay-test/builds/246
Merging develop now.

Please take one last look.

TimothyBanks · 2020-08-25T13:42:24Z

@heifner @allenhan2 I think this is ready to go. Please take another look.

Cherry-picked from 9eaa571 (EOSIO/eos#9235) with additional fixes for later changes to 2.0.x.

TimothyBanks force-pushed the EPE-17-develop-variant branch from eb36fee to fc545e5 Compare June 23, 2020 19:23

TimothyBanks requested review from larryk85, johndebord and allenhan2 June 23, 2020 20:38

allenhan2 reviewed Jun 24, 2020

View reviewed changes

heifner reviewed Jun 24, 2020

View reviewed changes

libraries/chain/transaction.cpp Outdated Show resolved Hide resolved

heifner reviewed Jun 24, 2020

View reviewed changes

libraries/fc/include/fc/static_variant.hpp Outdated Show resolved Hide resolved

heifner reviewed Jun 24, 2020

View reviewed changes

libraries/fc/include/fc/rpc/json_connection.hpp Outdated Show resolved Hide resolved

heifner reviewed Jun 25, 2020

View reviewed changes

libraries/fc/include/fc/static_variant.hpp Outdated Show resolved Hide resolved

heifner reviewed Jun 25, 2020

View reviewed changes

libraries/fc/include/fc/static_variant.hpp Outdated Show resolved Hide resolved

allenhan2 reviewed Jun 26, 2020

View reviewed changes

heifner reviewed Jul 20, 2020

View reviewed changes

libraries/chain/controller.cpp Show resolved Hide resolved

heifner reviewed Jul 30, 2020

View reviewed changes

libraries/chain/apply_context.cpp Show resolved Hide resolved

heifner reviewed Jul 30, 2020

View reviewed changes

Timothy Banks added 3 commits August 14, 2020 13:17

Migrate to std::variant

66ff121

Added std namespace qualifier on a few methods.

ecbeb98

Remove static variant tests

f42e6bf

Timothy Banks added 16 commits August 14, 2020 13:18

Remove static variant test

f6b17e7

Added std namespace qualifer on a couple of calls to visit.

558582d

Remove json_connection.hpp

af49ca5

Fixup per code review comments. Fixed formatting. Created get_if meth…

a40ea7b

…od that will throw fc exception if variant does not currently hold requested type. Revived the static_variant tests.

Corrected failing test

899c2cc

Rename get_if to get.

e8d7027

Added comment to fc::get methods

f3237c9

Changed exception raised by get_index.

ab37946

Created fc::visit to maintain same exception semantics

6f0f7ef

Cleanup up documentation

ce2a8cf

Change fc::exception to derive from std::exception and handle std::ex…

be830d7

…ceptions

Throw std exceptions

a657bb8

Update variant tests

fec7470

Remove fc get/visit

54b9e73

Explicitly catch bad_alloc exceptions

f28792b

Additional variant fixes after rebase

521040b

TimothyBanks force-pushed the EPE-17-develop-variant branch from d99f933 to 521040b Compare August 14, 2020 18:28

Updated variant in webassembly types.

1facb68

Timothy Banks added 2 commits August 24, 2020 13:47

Merge develop

9851075

Fixup state_history_tests.cpp

dca2470

larryk85 approved these changes Aug 25, 2020

View reviewed changes

Timothy Banks added 2 commits August 25, 2020 16:35

Merge develop

26fc995

Merge develop

f1a24af

TimothyBanks merged commit 9eaa571 into develop Aug 25, 2020

TimothyBanks deleted the EPE-17-develop-variant branch August 25, 2020 23:42

swatanabe pushed a commit to eosnetworkfoundation/mandel that referenced this pull request Jan 28, 2022

Migrate fc::static_variant to std::variant

0abc2fb

Cherry-picked from 9eaa571 (EOSIO/eos#9235) with additional fixes for later changes to 2.0.x.

swatanabe added a commit to eosnetworkfoundation/mandel that referenced this pull request Jan 28, 2022

Migrate fc::static_variant to std::variant

0c4f36d

Cherry-picked from 9eaa571 (EOSIO/eos#9235) with additional fixes for later changes to 2.0.x.

swatanabe mentioned this pull request Jan 28, 2022

Remove fc::optional and fc::static_variant eosnetworkfoundation/mandel#26

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate fc::static_variant to std::variant #9235

Migrate fc::static_variant to std::variant #9235

TimothyBanks commented Jun 22, 2020 •

edited

Loading

heifner commented Jun 22, 2020

TimothyBanks commented Jun 23, 2020

allenhan2 left a comment

TimothyBanks commented Jun 25, 2020

TimothyBanks commented Jun 25, 2020 •

edited

Loading

allenhan2 left a comment

TimothyBanks commented Jun 26, 2020 •

edited

Loading

allenhan2 commented Jun 26, 2020

heifner commented Jun 26, 2020 •

edited

Loading

TimothyBanks commented Jun 26, 2020 •

edited

Loading

TimothyBanks commented Jun 29, 2020 •

edited

Loading

TimothyBanks commented Jul 8, 2020

TimothyBanks commented Jul 16, 2020

TimothyBanks commented Jul 20, 2020

heifner Jul 30, 2020 •

edited

Loading

heifner Jul 30, 2020

TimothyBanks Aug 3, 2020

heifner Aug 3, 2020

heifner commented Jul 30, 2020 •

edited

Loading

TimothyBanks commented Aug 24, 2020

TimothyBanks commented Aug 25, 2020

Migrate fc::static_variant to std::variant #9235

Migrate fc::static_variant to std::variant #9235

Conversation

TimothyBanks commented Jun 22, 2020 • edited Loading

Change Description

Change Type

Consensus Changes

API Changes

Documentation Additions

heifner commented Jun 22, 2020

TimothyBanks commented Jun 23, 2020

allenhan2 left a comment

Choose a reason for hiding this comment

TimothyBanks commented Jun 25, 2020

TimothyBanks commented Jun 25, 2020 • edited Loading

allenhan2 left a comment

Choose a reason for hiding this comment

TimothyBanks commented Jun 26, 2020 • edited Loading

allenhan2 commented Jun 26, 2020

heifner commented Jun 26, 2020 • edited Loading

TimothyBanks commented Jun 26, 2020 • edited Loading

TimothyBanks commented Jun 29, 2020 • edited Loading

TimothyBanks commented Jul 8, 2020

TimothyBanks commented Jul 16, 2020

TimothyBanks commented Jul 20, 2020

heifner Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

heifner Jul 30, 2020

Choose a reason for hiding this comment

TimothyBanks Aug 3, 2020

Choose a reason for hiding this comment

heifner Aug 3, 2020

Choose a reason for hiding this comment

heifner commented Jul 30, 2020 • edited Loading

TimothyBanks commented Aug 24, 2020

TimothyBanks commented Aug 25, 2020

TimothyBanks commented Jun 22, 2020 •

edited

Loading

TimothyBanks commented Jun 25, 2020 •

edited

Loading

TimothyBanks commented Jun 26, 2020 •

edited

Loading

heifner commented Jun 26, 2020 •

edited

Loading

TimothyBanks commented Jun 26, 2020 •

edited

Loading

TimothyBanks commented Jun 29, 2020 •

edited

Loading

heifner Jul 30, 2020 •

edited

Loading

heifner commented Jul 30, 2020 •

edited

Loading