MailRuChamps / raic-2018

Russian AI Cup — artificial intelligence programming contest. Official website: http://russianaicup.ru
43 stars 32 forks source link

Общий интерфейс для Ball и Robot #9

Closed Oglan closed 5 years ago

Oglan commented 5 years ago

С общим интерфейсом не надо будет писать свои обертки и прокси для работы с точками и векторами. Всю математику каждый уже сам сможет добавить через расширения.

MelnikovIG commented 5 years ago

My approach to not use properties for critical parts. Its increase allocated object size because of backing fields.

MelnikovIG commented 5 years ago

You custom Vector is not optimized well as internal System.Numerics.Vector3, and it is class and as result it increase pressure to Garbage Collector

Oglan commented 5 years ago

We need fields to store data anyway (with or without properties). Also, properties will be inlined in this case. I guess CPU will be the bottleneck during simulation rather than memory. Strategy itself will allocate much more memory. As for Vector - agree, it's better not to introduce it at all in codebase. But we need shared interfaces for 3D objects (see first two commits). If every byte counts then you can introduce properties for interface members only and leave fields for other data.

By the way, if we're talking about performance then it's better not to use reflection serialisation at all. Better use manual serialisation: https://www.newtonsoft.com/json/help/html/Performance.htm#ManuallySerialize Or at least make serialisation directly from/to stream: https://www.newtonsoft.com/json/help/html/Performance.htm#MemoryUsage

I can send one more PR with interfaces only.

P.S. Обязательно на английском общаться? :)

MelnikovIG commented 5 years ago

So, there is no true reason to add autoproperties and custom Vector class? Even if autoproperties inlined, it increase object size, i think commutication with server should be as fast and lightweight as possible. So you can implement PR behavior in you code by extension methods for youself.

MelnikovIG commented 5 years ago

It will be good if you can improve json parsing. Recently i found intresting json parsers with optimizations, maybe we can experiment with them? https://github.com/Tornhoof/SpanJson https://github.com/neuecc/Utf8Json

Oglan commented 5 years ago

There is a "true reason" to introduce base interface for 3D objects (Ball, Nytro, Robot) actually - the convenient way to use model in strategy. Auto properties will not increase object size (they are methods in origin -> will slightly increase type size). Auto properties do not affect speed and size for server communication.

Of course I can implement it as extensions on my side. But 1) with interfaces this code will be better, 2) it's a beta time, I guess we can make this code a little bit better for newcomers (it introduces non breakable changes for original code).

MelnikovIG commented 5 years ago

1) I made some perfomance tests with BenckmarkDotnet Autoproperties vs Fields. Parsing object with fields faster from 1% to 3%, so its not critical, but fact. You can avoid force use of autroproperties if you will use abstract base class with fields instead of intefrace, but i am not sure is it good idea. 2) I think custom vector should be removed or replaced with System.Numerics.Vector3

Oglan commented 5 years ago

Base abstract class is good idea.

I can send PR with interfaces or base classes only. As option we can add Vector3 props.

Let's wait for project owner's decision.

kuviman commented 5 years ago

Language packs are made simplistic on purpose, I don't think this is necessary. You can use extension methods or create wrappers if you want in your code.