Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Allow StackItems to be deepcopied as immutable #473

Merged
merged 6 commits into from
May 24, 2022
Merged

Conversation

erikzhang
Copy link
Member

No description provided.

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

If we add a readOnly, we should be able to know if the StackItem is in this mode. I don't like it too much.

@erikzhang
Copy link
Member Author

It is useful if we send StackItems to a contract but don't want it to modify it.

@devhawk
Copy link
Contributor

devhawk commented May 22, 2022

It is useful if we send StackItems to a contract but don't want it to modify it.

If we're making a copy, who cares if the other contract modifies it?

@erikzhang
Copy link
Member Author

erikzhang commented May 22, 2022

After becoming an immutable type, many copies can be avoided.

See neo-project/neo#2747 (comment)

With immutable types, it only needs to be copied once when the notification is sent.

@erikzhang
Copy link
Member Author

And in the future, we can allow contracts to make an object immutable directly with a single instruction (e.g., OpCode.ASIMMUTABLE) without copying it.

@erikzhang
Copy link
Member Author

@devhawk @shargon In a Notify and GetNotifications scenario, if we don't make objects immutable when Notify, then we have to make a deep copy of all notifications every time we GetNotifications afterwards. This greatly affects performance.

@devhawk
Copy link
Contributor

devhawk commented May 22, 2022

I believe you, but I'd like to see benchmark, but I'd like to know exactly how much it impacts performance. Can we benchmark before and after?

@erikzhang
Copy link
Member Author

@devhawk After this is merged, we can easily benchmark in neo-project/neo#2748.

@erikzhang
Copy link
Member Author

erikzhang commented May 23, 2022

@devhawk I simply ran the benchmark from neo-project/neo#2748.

In the case of doing a deep copy every time, we get the following results:

Benchmark: NeoIssue2725,        Time: 00:00:11.1730555

But if I turn off deep copying (because deep copying is not needed if it's an immutable type), we get the following result:

Benchmark: NeoIssue2725,        Time: 00:00:08.0009047

The improvement is not obvious, that's because the attack code used there focuses on the GC, not the deep copy.

src/neo-vm/Types/Array.cs Outdated Show resolved Hide resolved
@erikzhang
Copy link
Member Author

Merge?

@erikzhang erikzhang merged commit 2a77d49 into master May 24, 2022
@erikzhang erikzhang deleted the deepcopy-immutable branch May 24, 2022 00:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants