Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hotfix bound threads for workers unsafe calls for llvm #4081

Conversation

jberthold
Copy link
Member

Fixes a problem with recently-introduced thread-local storage in the LLVM backend.
We have to make sure that all foreign calls relating to an LLVM-based term evaluation use the same OS thread, which can only be achieved via bound threads .

This PR

  • adds a flag to the generic RPC server in kore-rpc-types to run request worker threads in bound threads (using forkOS)
  • kore-rpc-booster uses this flag for bound threads when an LLVM backend library is used
  • declares the LLVM backend calls unsafe to make executing OS threads block instead of having new OS threads created for concurrent Haskell execution (they won't read HS heap data and never call back into Haskell).

This should protect us against problems related to using thread-local storage in the LLVM backend. Needs to be thoroughly tested before merging, because issues only materialised in proofs with substantial workload and parallel exploration.

PR #4080 uses runInBoundThread on the individual request processing calls instead of running the whole worker thread in the server as a bound thread. This was not solving the problem.

When using foreign libraries that use thread-local state, a program must ensure that foreign calls are always made from the same OS thread, by way of making these calls in _bound threads_, see https://downloads.haskell.org/ghc/latest/docs/libraries/base-4.20.0.0-1f57/Control-Concurrent.html#g:8

This PR
* adds a flag to the generic RPC server in `kore-rpc-types` to request running workers in bound threads. The server will use `forkOS` instead of `forkIO` in this case.
* requests running with bound threads when an LLVM backend library is used.

This _should_ protect us against problems related to using thread-local storage in the LLVM backend. Needs to be thoroughly tested before merging, because issues only materialised in proofs with substantial workload and parallel exploration.
@jberthold
Copy link
Member Author

This branch seems to fix the issues reported by @PetarMax for the Lido proof with parallel exploration.
Running standard KEVM and Kontrol performance tests (KEVM with a version using the updated LLVM backend that was rolled back).
Once we are convinced this does not cause issues on larger proofs we can merge this.

@jberthold jberthold marked this pull request as ready for review December 19, 2024 23:03
Copy link
Member

@ehildenb ehildenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with a question.

@jberthold
Copy link
Member Author

kevm tests ran without noticeable performance changes (using v0.1.760).
kontrol tests (using v1.0.85 ) with the usual noise:

Test HOTFIX-bound-threads-for-workers-unsafe-calls-for-llvm time master-64ad8705f time (HOTFIX-bound-threads-for-workers-unsafe-calls-for-llvm/master-64ad8705f) time
ExternalLibTest.testSum() 15.9 18.91 0.8408249603384452
StoreTest.testLoadNonExistent() 12.05 14.33 0.8408932309839497
ExpectRevertTest.testFail_ExpectRevert_failAndSuccess() 18.14 21.19 0.8560641812175555
ConstructorTest.run_constructor() 11.53 13.42 0.8591654247391952
BytesTypeTest.test_bytes32(bytes32) 11.96 13.6 0.8794117647058824
AddrTest.test_addr_true()-trace_options1 18.85 21.29 0.8853922029121655
BlockParamsTest.testCoinBase() 12.17 13.64 0.8922287390029325
EmitContractTest.testExpectEmitDoNotCheckData() 15.68 17.49 0.8965122927387079
kontrol/src/tests/integration/test_foundry_prove.py::test_constructor_with_symbolic_args 58.44 65.15 0.8970069071373752
ExpectRevertTest.test_expectRevert_returnValue() 20.86 23.23 0.8979767541971588
ImmutableVarsTest.test_run_deployment(uint256) 144.78 156.61 0.9244620394610816
CounterTest.testSetNumber(uint256) 21.21 22.9 0.9262008733624455
SymbolicStorageTest.testFail_SymbolicStorage1(uint256) 30.18 32.39 0.9317690645260883
AllowChangesTest.testFailAllowChangesToStorage() 15.51 16.54 0.9377267230955261
PrankTestOrigin.test_origin_setup() 27.34 29.0 0.9427586206896552
SetUpDeployTest.test_extcodesize() 40.24 42.32 0.950850661625709
kontrol/src/tests/integration/test_foundry_prove.py::test_foundry_refute_node 7.96 8.35 0.9532934131736527
AssertTest.test_assert_true_branch(uint256) 25.99 27.14 0.9576271186440677
AllowChangesTest.testAllow() 31.13 32.22 0.9661700806952204
AddrTest.test_notBuiltinAddress_symbolic(address) 16.41 15.67 1.0472239948947033
AllowChangesTest.testFailAllowCallsToAddress() 16.44 15.68 1.0484693877551021
]) 30.83 29.37 1.0497105890364316
PrankTestMsgSender.test_msgsender_setup() 29.32 27.8 1.0546762589928058
AssertTest.checkFail_assert_false() 17.65 16.48 1.0709951456310678
CoinBaseTest.test_coinbase_setup() 25.39 23.49 1.0808854831843338
BlockParamsTest.testChainId(uint256) 14.03 12.68 1.1064668769716088
BlockParamsTest.testRoll(uint256) 14.17 12.79 1.1078967943706022
ExpectRevertTest.testFail_expectRevert_false() 20.63 18.3 1.1273224043715846
FeeTest.test_fee_setup() 26.56 23.55 1.1278131634819533
kontrol/src/tests/integration/test_foundry_prove.py::test_foundry_xml_report 23.39 20.63 1.1337857489093555
EmitContractTest.testExpectEmitLessTopics() 17.68 15.43 1.1458198314970836
Setup2Test.test_setup() 13.84 12.01 1.1523730224812656
StoreTest.testGasStoreWarmUp() 18.3 15.86 1.153846153846154
ExpectRevertTest.test_expectRevert_true() 18.16 15.55 1.1678456591639872
kontrol/src/tests/integration/test_foundry_prove.py::test_foundry_merge_loop_heads 87.74 59.55 1.4733837111670864
TOTAL 930.46 924.56 1.006381413861729

@jberthold
Copy link
Member Author

jberthold commented Dec 20, 2024

Tests with booster-dev without dramatic regression

Test HOTFIX-bound-threads-for-workers-unsafe-calls-for-llvm time master-64ad8705f time (HOTFIX-bound-threads-for-workers-unsafe-calls-for-llvm/master-64ad8705f) time
benchmarks/overflow00-overflow-spec.k 26.95 29.15 0.9245283018867925
mcd-structured/flipper-tau-pass-spec.k 27.86 29.57 0.9421711193777477
mcd-structured/flipper-ttl-pass-spec.k 32.13 33.51 0.9588182632050135
mcd-structured/dsvalue-read-pass-spec.k 33.79 32.57 1.0374577832361067
TOTAL 120.73 124.8 0.9673878205128206

(this is from running make test-prove-rules ... PYTEST_ARGS='... --use-booster-dev')

@automergerpr-permission-manager automergerpr-permission-manager bot merged commit 3021727 into master Dec 20, 2024
6 checks passed
@automergerpr-permission-manager automergerpr-permission-manager bot deleted the HOTFIX-bound-threads-for-workers-unsafe-calls-for-llvm branch December 20, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants