Skip to content

Conversation

@alexd1971
Copy link

Problem: The current process for creating a JS representation of the AST requires an intermediate conversion to S-expression. This hurts performance and makes it impossible to annotate all nodes with types.

Solution: Implement a library for converting the original AST directly to JS, eliminating the intermediate step.

@alexd1971 alexd1971 requested a review from a team as a code owner October 14, 2025 07:40
@cla-idx-bot
Copy link

cla-idx-bot bot commented Oct 14, 2025

Dear @alexd1971,

In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA.

If you decide to agree with it, please visit this issue and read the instructions there. Once you have signed it, re-trigger the workflow on this PR to see if your code can be merged.

— The DFINITY Foundation

Copy link
Contributor

@rvanasa rvanasa left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements! Apologies for the delay in reviewing; a lot is happening internally at the moment.

It could be worth investigating whether it's possible to cache / share type representations to reduce the memory and conversion overhead. This would be relevant for larger-scale projects which tend to have a lot of duplicated complex type nodes.

@alexd1971 alexd1971 force-pushed the grant-2-milestone-1 branch from 3101997 to 81e68a6 Compare November 4, 2025 11:33
@alexd1971
Copy link
Author

Thanks for the improvements! Apologies for the delay in reviewing; a lot is happening internally at the moment.

It could be worth investigating whether it's possible to cache / share type representations to reduce the memory and conversion overhead. This would be relevant for larger-scale projects which tend to have a lot of duplicated complex type nodes.

Thank you for your review and your suggestion for further improvement. We think that introducing typeRep cache is reasonable and actually will allow us to reduce consuming resources. We could implement it as follows:

  1. Introduce a JS Map as a typeRep cache storage.
  2. Use string representation of type as a key and typeRep object as a value.
  3. During serialization if we already have a key in the cache Map we don't serialize type representation again.
  4. To make AST compatible with current implementation we need to define typeRep as the Node object property with the getter taking the value from the cache.

However, it sounds like a non-trivial change that is quite large and requires thorough testing. That's why we propose postponing this change until later and merging this PR now, given that it is already an improvement on its own and satisfies the corresponding acceptance criterion from the first milestone.

@alexd1971 alexd1971 requested a review from rvanasa November 5, 2025 18:43
Problem: The current process for creating a JSON representation of the
AST requires an intermediate conversion to S-expression. This hurts
performance and makes it impossible to annotate all nodes with types.

Solution: Implement a library for converting the original AST directly
to JSON, eliminating the intermediate step.
@alexd1971 alexd1971 force-pushed the grant-2-milestone-1 branch from 81e68a6 to c5fd186 Compare November 6, 2025 06:39
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