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

Implement t8ntool to use for execution-spec-tests #3603

Merged
merged 120 commits into from
Sep 14, 2024
Merged

Conversation

jochem-brouwer
Copy link
Member

@jochem-brouwer jochem-brouwer commented Aug 18, 2024

Resolves #3602

This PR implements T8NTool, used for execution-spec-tests to fill (and in the future: write) tests. To see how it works, check the t8ntool readme in vm/test/t8n/README.md.

@jochem-brouwer jochem-brouwer changed the base branch from master to 7702-update August 18, 2024 16:50
@jochem-brouwer jochem-brouwer marked this pull request as ready for review September 13, 2024 13:42
fi
SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
export NODE_OPTIONS="--max-old-space-size=4096"
tsx "$SCRIPT_DIR/launchT8N.ts" "$@"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tsx "$SCRIPT_DIR/launchT8N.ts" "$@"
npx tsx "$SCRIPT_DIR/launchT8N.ts" "$@"

This way it will run even if I only have tsx installed locally within ethjs and not globally.

acolytec3
acolytec3 previously approved these changes Sep 14, 2024
Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM. Was able to get the fill process to run.

@jochem-brouwer
Copy link
Member Author

Thanks a lot all, this was a huge process, but super happy to get this merged now 😄

Together with ethereum/execution-spec-tests#752 we are now ready "out-of-the-box" to fill tests 😄 🚀

@jochem-brouwer jochem-brouwer merged commit d4d9b37 into master Sep 14, 2024
39 checks passed
@holgerd77 holgerd77 deleted the t8ntool branch September 16, 2024 06:14
@holgerd77
Copy link
Member

🎉🎉🎉

@jochem-brouwer
Copy link
Member Author

Hi @holgerd77, the slow stuff of t8n has not much to do with using MCL. It is simply the load time of all our libraries. It is done each time t8n is called, which can be done multiple times per test (in fact, for each block t8n is called).

I have on the agenda a separate PR to open up a server daemon which will then remove this "cold load" time and instead "hot load" the VM. This will drastically speed up the runtime of this tool (for multiple tests).

You could think of this when running the Blockchain tests, except now you dont run it with the default settings but instead you run npm run test:blockchain -- --test=TEST. This is essentially what is happening right now with t8n, which is obviously very slow.

Note that for BLS tests MCL is indeed somewhat faster (we have seen in the Blockchain tests a decrease from ~7 seconds to something like ~6 seconds). However that is for all tests, this ~1 second change is already what would be saved if we would "hot load" the libraries.

I plan to add this server daemon asap!

@holgerd77
Copy link
Member

Hi @holgerd77, the slow stuff of t8n has not much to do with using MCL. It is simply the load time of all our libraries. It is done each time t8n is called, which can be done multiple times per test (in fact, for each block t8n is called).

I have on the agenda a separate PR to open up a server daemon which will then remove this "cold load" time and instead "hot load" the VM. This will drastically speed up the runtime of this tool (for multiple tests).

You could think of this when running the Blockchain tests, except now you dont run it with the default settings but instead you run npm run test:blockchain -- --test=TEST. This is essentially what is happening right now with t8n, which is obviously very slow.

Note that for BLS tests MCL is indeed somewhat faster (we have seen in the Blockchain tests a decrease from ~7 seconds to something like ~6 seconds). However that is for all tests, this ~1 second change is already what would be saved if we would "hot load" the libraries.

I plan to add this server daemon asap!

Ah ok, makes a lot of sense, thanks for the insight! 🤯

Also thanks for planning ahead on this already! 🙏

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.

Implement T8N support (tracking issue)
4 participants