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

Add new PolyMethodCall node with type parameters #980

Closed
aslesarenko opened this issue May 13, 2024 · 4 comments
Closed

Add new PolyMethodCall node with type parameters #980

aslesarenko opened this issue May 13, 2024 · 4 comments
Labels
A-consensus Area: Code used in consensus (i.e. transaction validation) C-feature Category: Feature request or PR soft-fork Implementation requires soft-fork
Milestone

Comments

@aslesarenko
Copy link
Member

Problem

Current MethodCalls are limited (see this ScalaDoc)
In particular they cannot be used to represent CONTEXT.getVar[Int](1).get or box.getReg[Long](5).get, because the type parameter Int and Long respectively cannot be serialized in v5.0.

Solution

  • Add a new ErgoTree node PolyMethodCall with a new opcode which works exactly as MethodCall, but supports 1 or more type parameters.
  • The PolyMethodCall nodes would be introduced in ErgoTree by ErgoScript compiler when necessary, i.e. then concrete types parameters need to be specified.
  • The type parameters should be concrete types which are supported by TypeSerializer.
@aslesarenko aslesarenko added soft-fork Implementation requires soft-fork A-consensus Area: Code used in consensus (i.e. transaction validation) C-feature Category: Feature request or PR labels May 13, 2024
@aslesarenko aslesarenko added this to the v6.0 milestone May 13, 2024
@aslesarenko aslesarenko changed the title Add new MethodCall node with type parameters Add new PolyMethodCall node with type parameters May 13, 2024
@kushti
Copy link
Member

kushti commented May 13, 2024

I don't think the new node is needed:

  • MethodCallSerializer structurally is ready for type substitution, it is just always Map.empty now, but for new method you can fix that
  • Code for PolyMethodCall(Serializer) will mostly repeat the MethodCall(Serializer), abstraction would bring more complexity
  • Versioning-wise adding new node & serializer is more complex
  • New opcode will be introduces, which is also more complex (and there were few bugs already with introducing new ids)

So looks like unnecessary complication

In particular they cannot be used to represent CONTEXT.getVarInt.get or box.getRegLong.get, because the type parameter Int and Long respectively cannot be serialized in v5.0.

Not true, see how Global.deserializeRaw[T] is implemented

@aslesarenko
Copy link
Member Author

Not true, see how Global.deserializeRaw[T] is implemented

Well, to me the implementation looks more like workaround, but ok.
However, performance wise, this is a hotspot, so we add unnecessary and increasingly big overhead to most of simple method calls with the chain of if statements.

The improvement is to serialize a list of types:

  • read the size of the list first (will be 0 for most method calls)
  • then if non null read the items

Then subst can be added in a generic way, so no need to have ifs there at all.

This can be done in a separate branch, as it is local to MethodCall improvement, not related to a specific method.

@kushti
Copy link
Member

kushti commented May 13, 2024

For performance, you can add a flag to polymorphic methods, and do just one check during deserialization then, and then do other checks only for polymorphic methods

@aslesarenko
Copy link
Member Author

you can add a flag to polymorphic methods

Ok, make sense.

@kushti kushti closed this as completed May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Code used in consensus (i.e. transaction validation) C-feature Category: Feature request or PR soft-fork Implementation requires soft-fork
Projects
None yet
Development

No branches or pull requests

2 participants