-
Notifications
You must be signed in to change notification settings - Fork 12
New parser #158
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
base: rust/dev
Are you sure you want to change the base?
New parser #158
Conversation
…s; fix string leak
…n, restriction, drop_head, all as member functions.
…n from Rholang collection AST (normalizer support).
This reverts commit b66594d.
The PathMap crate depends on gxhash which requires AES and SSE2 CPU features. GitHub Actions runners support these features but Rust doesn't enable them by default for portability. This was causing the build to fail with: error: Gxhash requires aes and sse2 intrinsics
The new_parser branch adds circe-generic-extras auto-derivation for PathMap/Zipper types which requires heavy macro expansion. The node project uses forked compilation (Compile/fork := true) with -Xss8m stack, but the forked JVM was only getting 4GB heap from SBT_OPTS. This caused GC thrashing (56.9% time in GC) during node compilation, making it hang indefinitely. Increasing to 6GB provides enough memory for the macro expansion while leaving headroom on 7GB GitHub runners.
Increase SBT_OPTS stack from 2m to 8m to handle the circe-generic-extras macro expansion for PathMap types in the node project.
The forked JVM for Compile was causing 7+ minute compile times because it didn't inherit SBT_OPTS (including -Xss8m stack size). The forked JVM also incurs cold startup penalty. Now that SBT_OPTS has -Xss8m, the main SBT JVM can handle the circe-generic-extras macro expansion for PathMap types directly. This should restore node compile times to ~30 seconds.
The auto._ import triggered automatic implicit derivation for ALL types in scope, causing exponential macro expansion with recursive protobuf types (Par/Expr/Connective). This caused node compilation to take 2+ hours instead of 30 seconds. Changes: - Remove io.circe.generic.extras.auto._ import - Add explicit deriveConfiguredEncoder/Decoder for ExprInstance (sealed trait) - Add explicit encoders/decoders for new EPathMap and EZipper types
AndriiS-DevBrother
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a full context and good knowledge of bnfc/tree-sitter differences
so just few minor comments from my side
| @@ -0,0 +1,2 @@ | |||
| [toolchain] | |||
| channel = "nightly-2025-06-15" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this version?
| @@ -1,53 +0,0 @@ | |||
| //! This crate provides Rholang language support for the [tree-sitter][] parsing library. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to cleanup?
| return std::ptr::null(); | ||
| } | ||
| }; | ||
| // let result = match Compiler::source_to_adt_with_normalizer_env( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup?
| } | ||
|
|
||
| it should "return None for logs containing extra comm events" in withGenesis(genesisContext) { | ||
| it should "return None for logs containing extra comm events" ignore withGenesis(genesisContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a good description for other ignored tests
can we document this one too? why gets it ignored label?
| crypto = { path = "../crypto" } | ||
| rspace_plus_plus = { path = "../rspace++" } | ||
| shared = { path = "../shared" } | ||
| pathmap = { git = "https://github.com/Adam-Vandervorst/PathMap", branch = "cleanup_to_release" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, will we depends on a personal repo?
| @@ -49,8 +49,7 @@ class RholangOnlyDispatcher[M[_]](implicit s: Sync[M], reducer: Reduce[M]) | |||
| case ParBody(parWithRand) => | |||
| val env = Dispatch.buildEnv(dataList) | |||
| val randoms = parWithRand.randomState +: dataList.toVector.map(_.randomState) | |||
| // reducer.eval(parWithRand.body)(env, Blake2b512Random.merge(randoms)) | |||
| s.unit | |||
| reducer.eval(parWithRand.body)(env, Blake2b512Random.merge(randoms)) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert?
| checkpoint <- runtime.createCheckpoint | ||
| expectedHash = Blake2b256Hash.fromHex( | ||
| "10cce029738696f1e120a6bad4bdf3f18adca25ccf36133bd4916f607a6a50c0" | ||
| "1180006eedef0a38b891bd588e9aff630bcf6c83cd6b53db0375bb48f922f010" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that expected? I dont see what input changed, but the expected output get changed
YuriiO-DevBrother
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw a lot of changes related to Scala code. From this perspective If we still plan to modify Scala sources in the rust/dev and keep the hybrid node in working state I am thinking maybe we need to include CI checks for Scala. At least it should compile and unit tests should pass.
|
|
||
| env: | ||
| # Enable AES/SSE2 CPU features for gxhash dependency (required by PathMap crate) | ||
| # GitHub Actions runners support these features on x86_64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about arm64 target? we are supporting both arm64 and amd64
| prost = { workspace = true } | ||
| tonic-prost = { workspace = true } | ||
| colored = "3.0" | ||
| rholang-parser = { git = "https://github.com/F1R3FLY-io/rholang-rs", branch = "f1r3node_dependecies" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in branch name. Is this correct?
Replace internal parser with parser from
rholang-rs