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

Should we change the VM limits from reference counting to memory space? #245

Closed
shargon opened this issue Dec 3, 2019 · 6 comments
Closed

Comments

@shargon
Copy link
Member

shargon commented Dec 3, 2019

After #202 now we have a great optimizations with the neo limits, so.. this issue is more related to the standard, and the learning curve.

I think that we should have a limit memory for the SmartContract, instead of MaxItemSize. I think that for the developer, it will be easier if they only need to know that he have 8Mb of memory per execution. It'ts hard to explain to a new user that he can't have one byte array[2049] but he can have 1000 byte arrays of 2048.

The developer need to know how the VM it works. If we limit according to the used memory, we can unify the VM behaviour as the most common behaviors, and we can remove MaxItemSize limit.

Cons:

  • Define how much memory per reference (in a deterministic way)
  • Calculate the memory size for certains types (Maps)
  • Speed is not improved.
  • Backward incompatible.

Pros:

  • More fair.
  • Easy to understand for users and developers.
  • We can remove MaxItemSize limit.

It's worth?

@erikzhang
Copy link
Member

I think the developers don't know how much memory has been used as well.

@roman-khimov
Copy link
Contributor

I think it would be interesting if there was some easy way for the managed runtime to count that for you, but I've failed to find any way to do that in Go and I doubt there anything in dotnet also. And doing this kind of calculations manually is no fun. So while the idea seems appealing it probably isn't worth the implementation cost.

@shargon
Copy link
Member Author

shargon commented Dec 5, 2019

I think that the unique way is create all the items inside the ExecutionEngine, and Dispose them when they are not used.

I used Something like that in the past for neo-hypervm. Because in c++ you need to free the memory by yourself so you need this kind of control.

https://github.com/CityOfZion/neo-hypervm/blob/development/src/Neo.HyperVM/ExecutionEngine.h#L137-L270

But the code for manage it's more ugly.

https://github.com/CityOfZion/neo-hypervm/blob/e7c72b08b3594a07b3bee7f573cf71c170f4760d/tests/NeoSharp.VM.Interop.Tests/VMOpCode_PUSHES.cs#L30-L33

You need to Dispose the item every time you use the item outside the VM. It only will be disposed if his internal counter is 0. But you need to do that.

That was my solution for c++, but with shared pointers you can obtain the same behaviour

@lock9
Copy link
Contributor

lock9 commented Dec 13, 2019

Maybe we can leave that for the future, but I think it would be good to have this and also configure it as a parameter.

@roman-khimov
Copy link
Contributor

As the most problematic types wrt this are buffers and bytestrings, maybe we can tune their reference counting to reflect the size they have? Or apply size limitations to these types only (all the others are too small in general to care about them)?

2 GB that we allow now are too much for a smart contract IMO, given the cost of calculations/storage on Neo and block/tx size limitations I don't see any realistic scenarios for that (there probably are for single 1 MB element, but not 2048 of them) and I think running a full node with 2/4 GB of system memory should be absolutely possible.

So while doing full memory space calculation is too expensive we can do one of the two things:

  • make big buffers and bytestrings count for more than one reference. 64K is probably fine for a single reference (it's a bit special after Add ByteString.MaxComparableSize #367 anyway), so 1MB Buffer would use 16 references and filling up the memory with them would stop at 128MB of overall memory used
  • leave refcounting as is, but limit cumulative size of buffers and bytestrings specifically to some value (64MB?), proper accounting of memory used by integers or maps is not trivial, but they're comparatively small anyway, while it costs nothing to get a size of a buffer or bytestring and it's not hard to apply some limits for them

@erikzhang
Copy link
Member

Closed since we have no plan to implement it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants