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

Reduce object churn for instruction types #37

Open
pipermerriam opened this issue Jan 25, 2019 · 3 comments
Open

Reduce object churn for instruction types #37

pipermerriam opened this issue Jan 25, 2019 · 3 comments

Comments

@pipermerriam
Copy link
Member

pipermerriam commented Jan 25, 2019

What is wrong

The Instruction classes use a metaclass pattern to intern instance values for re-use in order to reduce the overall number of objects that need to be instantiated.

How can it be fixed

  • remove the pattern entirely as it may not be needed.
  • change to a registry pattern that manages the instance caching.
@pipermerriam
Copy link
Member Author

Worth noting that @davesque and I talked this over today and my conclusion is that this should just be removed entirely for the time being.

  • premature optimization without any real benchmarking
  • a collision on the cache key for interning which is currently possible would have incredibly unfortunate results (and could in theory be an attack vector if it was possible to do this deterministically).

@davesque
Copy link
Contributor

davesque commented Jan 29, 2019

If this library should be usable to run typical web assembly modules, then I can imagine that the average module in the broader world of WASM (not just the Ethereum world) could be quite large. So it may actually be necessary to reduce the memory footprint of all those instruction instances. However, in the Ethereum world, this probably does still count as premature optimization.

I've also been an advocate of using __slots__ where appropriate and I see a lot of potential to benefit from that in this library. That would reduce the memory footprint of each instance and also speed up access times for instance properties.

@pipermerriam
Copy link
Member Author

After some profiling we should remove the Interned base class and implement caching at a different level (as @cburgdorf pushed for from the beginning 😺 )

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

No branches or pull requests

2 participants