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

xcvm: simplify instructions handling in interpreter contract #3668

Merged
merged 1 commit into from
Jun 8, 2023
Merged

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Jun 5, 2023

XCVMProgram::instructions is a VecDeque. The simplest way to split it into first element and tail containing the rest of the instruction is to use pop_front method. In fact, this is a constant-time operation.

However, handle_execute_step does a strange dance where it converts the instructions into an iterator and then collects the iterator into a new VecDeque. That’s an O(n) operation with an additional allocation.

Simplify the code to use the pop_front instead.

  • PR title is my best effort to provide summary of changes and has clear text to be part of release notes
  • I marked PR by misc label if it should not be in release notes
  • I have linked Zenhub/Github or any other reference item if one exists
  • I was clear on what type of deployment required to release my changes (node, runtime, contract, indexer, on chain operation, frontend, infrastructure) if any in PR title or description
  • I waited and did best effort for pr-workflow-check / draft-release-check to finish with success(green check mark) with my changes
  • I have added at least one reviewer in reviewers list
  • I tagged(@) or used other form of notification of one person who I think can handle best review of this PR
  • I have proved that PR has no general regressions of relevant features and processes required to release into production

@mina86
Copy link
Contributor Author

mina86 commented Jun 7, 2023

@dzmitry-lahoda, low priority but if you have a moment feel free to take a look.

@blasrodri
Copy link
Contributor

Are we running XCVM on CI?

@dzmitry-lahoda
Copy link
Contributor

no, we do not run

@dzmitry-lahoda
Copy link
Contributor

@mina86 did you run xcvm contracts tests? i doubt these part of CI, so need to run manually

I have proved that PR has no general regressions of relevant features and processes required to release into production

just to ensure all as was before (i guess tests could already fail before, hope not :) )

@dzmitry-lahoda dzmitry-lahoda added the Misc I marked PR by `misc` label if it should not be in release notes #owned:terraform label Jun 8, 2023
@dzmitry-lahoda dzmitry-lahoda self-requested a review June 8, 2023 07:57
@mina86
Copy link
Contributor Author

mina86 commented Jun 8, 2023

@mina86 did you run xcvm contracts tests? i doubt these part of CI, so need to run manually

I have proved that PR has no general regressions of relevant features and processes required to release into production

just to ensure all as was before (i guess tests could already fail before, hope not :) )

Yeah, I’m kinda confused interpreting the CI results. Like bunch of them fail but all that GitHub gives me is ‘This job failed’. That’s not a useful information which I can use to figure out the problem.

XCVMProgram::instructions is a VecDeque.  The simplest way to split it
into first element and tail containing the rest of the instruction is
to use pop_front method.  In fact, this is a constant-time operation.

However, handle_execute_step does a strange dance where it converts
the instructions into an iterator and then collects the iterator into
a new VecDeque.  That’s an O(n) operation with an additional
allocation.

Simplify the code to use the pop_front instead.
@dzmitry-lahoda
Copy link
Contributor

@mina86 please press

image

it is GH queue

@dzmitry-lahoda
Copy link
Contributor

@mina86 Your care about this draft-release-check job and its dependencies (see needs). In your case it seem green.

@mina86 mina86 added this pull request to the merge queue Jun 8, 2023
Merged via the queue into ComposableFi:main with commit 4445447 Jun 8, 2023
@mina86 mina86 deleted the a branch June 8, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Misc I marked PR by `misc` label if it should not be in release notes #owned:terraform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants