30log strange deep-copy behavior

[airstruck]

30log does something strange.

Whenever a class is instantiated, it deep-copies everything from the prototype into the newly-created instance. This is troubling for a number of reasons:

• The whole point of a prototype is that fields that don't exist in the object can be resolved in the prototype. There's just no reason to copy everything. Maybe this makes sense when extending classes, if you want to prevent long prototype chains, but certainly not when instantiating them.

• Copying uses pairs, which is slow and can't be compiled by the JIT. Recursive (deep) copying compounds the problem.

  Consider ROT.Color, which defines about 150 preset color triplets. If you use ROT.Color as a class, that means every class instance has to copy each component of each of those preset colors -- 450 copies, one for each component -- and the instances will never need any of that, because it's meant to be used statically.

  Beyond that, it breaks the tests, because the tests expect colors to only have certain fields. If class methods and so on stay in the prototype where they belong, the tests work fine.

• It causes unexpected behavior. For example the current EventQueue only works because of this weird quirk. Why? Because some tables are defined in the prototype that shouldn't be there (_events and _eventTimes). If these aren't deep-copied, EventQueue will break, because all instances will share a single table when each one is supposed to have its own table.

  These should be defined in a constructor, so it's clear they belong to the instance, rather than putting them in the prototype and relying on peculiar class library behavior. What if the author of 30log arrives at the same conclusion about deep-copying on instantiation? Suddenly that code breaks and it's not clear why.

You can probably guess where I'm going with this. It might not be a bad idea to ditch 30log and show people how to patch it in themselves (something like package.loaded['rot.class'] = require('30log')() before loading rotLove should do it).

[airstruck]

This ticket is starting to remind me of that ear of corn in the back of my fridge I keep meaning to throw out.

Possible ways forward:

• Scrap 30log.
• Get 30log to stop doing that by filing a ticket on their issue tracker or using our own modified copy.
• Give up on trying to use classes where instantiation performance counts.
• Pretend the weird behavior doesn't exist, performance be damned. Also write new tests.

For what it's worth, I like the first option. Any thoughts on this?

[Zireael07]

I'd do 1 and 2 at the same time (file a ticket on 30log and scrap 30 log from rotLove).

[paulofmandown]

I'm ok with ditching 30log, especially after this commit duplicated the functionality we were using.

Sent from my Lge LG-VS985 using FastHub

[timothymtorres]

I'd agree to tossing 30log in the trash. We might be better off using middleclass or writing our own Metatables. Personally I'm a fan of middleclass, good documentation and easy to use!

[airstruck]

Sounds good, thanks for the feedback! I've opened Yonaba/30log#35.

30log can be plugged in with a single line of code, so people can still use it if they want:

package.loaded['src.rot.class'] = require('30log')()

@timothymtorres, we've already got a solution in class.lua that mimics the parts of 30log's API that we use. Middleclass can also be plugged in by doing something like the above, but it needs to be shimmed with extend, and initialize needs to be mapped onto init (see #25 (comment)).

I'll write a PR soon with 30log removed, and we can write something up in the docs with snippets for plugging in other class libraries.

[airstruck]

With 30log gone, I've added some special-purpose collection classes (called "PointSet" and "Grid") to my local copy to use instead of the foo[x..','..y] and foo[x][y] idioms. These store data in a densely-packed format that can be iterated quickly, along with a hash part for quick retrieval. The calling code is neater and more concise, and the tests run roughly twice as fast (not a proper benchmark, but a good sign anyway).

Will push a PR soon, just waiting on #53 to make sure it gets rebased properly.

[cpeosphoros]

I'm working on a 30log fork which aims to rehaul and streamline the whole inheritance/instantiation process: https://github.com/cpeosphoros/30log-plus

The main reason I choose to fork from 30 log was ditch the minimalist philosophy in exchange for stronger class identity, less code duplication in applications and better performance for heavy classes' instantiation.

The second main reason was having stronger mixin support.

The trade over I choose was having a heavier OO library in exchange for better application code.

One of my TODO list's item is exactly removing deepcopy processing for both cases inheritance/instantiation.

It is still very early WIP and it doesn't currently address your issue, but I'll be glad to hear from you guys if there are other features you would like to see implemented.