trjhtr

I have designed an objectservice for a single threaded game rpg server. It's purpose is to store the entire set of objects in a single instance shared by all connected clients. it's purpose it follows a singleton pattern but i would like some reviews on some the class as it does use alot of cpu and is a class that is accessed at 60 frames a second.

using System;
using System.Collections.Generic;
using System.Linq;
using Darkages.Common;
using Darkages.Storage;
using Darkages.Types;
using Newtonsoft.Json;

namespace Darkages.Network.Object

 [Serializable]
 public sealed class ObjectService : IDisposable
 
 [JsonIgnore] private static readonly object syncLock = new object();

 [JsonIgnore] private static ObjectService context;

 [JsonIgnore] private readonly HashSet<Sprite> _aislings = new HashSet<Sprite>();

 [JsonIgnore] private readonly HashSet<Sprite> _monsters = new HashSet<Sprite>();

 [JsonIgnore] private readonly HashSet<Sprite> _mundanes = new HashSet<Sprite>();

 [JsonProperty] private HashSet<Sprite> _items = new HashSet<Sprite>();

 [JsonProperty] private HashSet<Sprite> _money = new HashSet<Sprite>();

 private bool disposedValue; // To detect redundant calls


 [JsonIgnore] private static bool CacheLoaded get; set; 

 [JsonIgnore]
 private HashSet<Sprite> Aislings
 
 get
 
 lock (syncLock)
 
 return new HashSet<Sprite>(_aislings);
 
 
 

 [JsonIgnore]
 private HashSet<Sprite> Monsters
 
 get
 
 lock (syncLock)
 
 return new HashSet<Sprite>(_monsters);
 
 
 

 [JsonIgnore]
 private HashSet<Sprite> Mundanes
 
 get
 
 lock (syncLock)
 
 return new HashSet<Sprite>(_mundanes);
 
 
 

 [JsonIgnore]
 private HashSet<Sprite> Money
 
 get
 
 lock (syncLock)
 
 return new HashSet<Sprite>(_money);
 
 
 

 [JsonIgnore]
 private HashSet<Sprite> Items
 
 get
 
 lock (syncLock)
 
 return new HashSet<Sprite>(_items);
 
 
 

 [JsonIgnore]
 public static ObjectService Context
 
 get
 
 if (context == null)
 context = new ObjectService();

 return context;
 
 

 public void Dispose()
 
 disposedValue = !disposedValue;
 Dispose(true);
 

 public event ObjectEvent<Sprite> ObjectAdded;
 public event ObjectEvent<Sprite> ObjectChanged;
 public event ObjectEvent<Sprite> ObjectRemoved;


 public T Query<T>(Predicate<T> predicate) where T : Sprite, new()
 
 var obj = new T();


 if (obj is Aisling)
 
 if (Aislings == null)
 return null;

 return Aislings.Cast<T>().FirstOrDefault(i => predicate(i));
 

 if (obj is Monster)
 
 if (Monsters == null)
 return null;

 return Monsters.Cast<T>().FirstOrDefault(i => predicate(i));
 

 if (obj is Mundane)
 
 if (Mundanes == null)
 return null;

 return Mundanes.Cast<T>().FirstOrDefault(i => predicate(i));
 

 if (obj is Money)
 
 if (Money == null)
 return null;

 return Money.Cast<T>().FirstOrDefault(i => predicate(i));
 

 if (obj is Item)
 
 if (Items == null)
 return null;

 return Items.Cast<T>().FirstOrDefault(i => predicate(i));
 


 return null;
 

 public T QueryAll<T>(Predicate<T> predicate) where T : Sprite, new()
 
 var obj = new T();

 if (obj is Aisling)
 
 if (Aislings == null)
 return null;

 return Aislings.Cast<T>().Where(i => predicate(i)).ToArray();
 

 if (obj is Monster)
 
 if (Monsters == null)
 return null;

 return Monsters.Cast<T>().Where(i => predicate(i)).ToArray();
 

 if (obj is Mundane)
 
 if (Mundanes == null)
 return null;

 return Mundanes.Cast<T>().Where(i => predicate(i)).ToArray();
 

 if (obj is Money)
 
 if (Money == null)
 return null;

 return Money.Cast<T>().Where(i => predicate(i)).ToArray();
 

 if (obj is Item)
 
 if (Items == null)
 return null;

 return Items.Cast<T>().Where(i => predicate(i)).ToArray();
 


 return null;
 

 public void Save<T>(T reference, Predicate<T> predicate)
 where T : Sprite
 
 if (reference == null)
 return;

 ObjectChanged?.Invoke(reference);
 

 public void Insert<T>(T obj) where T : Sprite
 
 if (obj == null)
 return;

 lock (Generator.Random)
 
 obj.Serial = Generator.GenerateNumber();
 

 lock (syncLock)
 
 if (obj is Aisling)
 _aislings.Add(obj);

 if (obj is Monster)
 _monsters.Add(obj);

 if (obj is Mundane)
 _mundanes.Add(obj);

 if (obj is Money)
 _money.Add(obj);

 if (obj is Item)
 _items.Add(obj);

 ObjectAdded?.Invoke(obj);
 
 

 public void RemoveAll<T>(T objects) where T : Sprite
 
 if (objects == null)
 return;

 for (uint i = 0; i < objects.Length; i++)
 Remove(objects[i]);
 


 public void Remove<T>(T obj) where T : Sprite
 
 if (obj == null)
 return;

 lock (syncLock)
 
 if (obj is Aisling)
 _aislings.Remove(obj);

 if (obj is Monster)
 _monsters.Remove(obj);

 if (obj is Mundane)
 _mundanes.Remove(obj);

 if (obj is Money)
 _money.Remove(obj);

 if (obj is Item)
 _items.Remove(obj);

 ObjectRemoved?.Invoke(obj);
 
 

 public void Cache()
 
 StorageManager.Save(this);
 

 private void Dispose(bool disposing)
 
 if (!disposedValue)
 
 if (disposing)
 
 var removables = Aislings.Concat(Money).Concat(Mundanes).Concat(Monsters).Concat(Items)
 .Reverse();

 foreach (var obj in removables)
 obj.Remove();

 _items?.Clear();
 _money?.Clear();
 _monsters?.Clear();
 _aislings?.Clear();
 _mundanes?.Clear();
 

 disposedValue = true;
 
 

 internal static void Set(ObjectService cache_)
 
 if (CacheLoaded)
 return;

 foreach (var obj in cache_.Items)
 Context.Insert(obj);

 foreach (var obj in cache_.Money)
 Context.Insert(obj);

 CacheLoaded = true;
 
 

1) What's the point of storing sprites on server-side? Sounds counterintuitive. Normally all such assets are located on the client. Sharing sprites over the network usually requires too much bandwidth. If those objects are not technically sprites, then you should use a different name, so it is less confusing.

2) Your singleton creation is not thread safe. Use Lazy<T> or non-lazy initialization. It is also not clear why do you need a singleton in the first place. Why can't you use a regular class for that.

3) Locking different clients on the same shared resource is a bad idea. There is no way to tell the lock which client to serve next, so it is possible that some clients will never get access to resource if it is permanently contested by other clients. This is probably an unacceptable design given the nature of multiplayer games.

4) Copying hashsets inside getters can hit performance, depending on how often those getters are accessed.

5) Dispose method is not thread-safe. Also you do not want to have a disposable singleton, because it is hard to tell when it is safe to dispose a static instance. If you need to dispose an object - don't make it a singleton.

6) Back-and-forth casting can introduce an unnecessary overhead. You can try using overloaded methods instead of generic ones, and see if it helps performance.

7) By .Net convention method name Insert implies the presence of index among the parameters. If there is no index - call it Add.

Witout questioning your design here are a couple of improvements you can make.

var obj = new T();

In your case creating this dummy and not actually used objects may hurt performance because you add a lot of garbage to collect. You could replace it with a simple typeof(T) instead and avoid the new entirely.

There is also a tremendous code duplication in your collection handling methods that should be addresed first. You can do it by implementing the collection operations in a new type, let's call it SpriteCollection<T>.

In this type you put such methods as Query, QueryAll, Add (old Insert) etc.

Then you should get rid of the three ObjectX events and replace them by implementing the same two intefaces INotifyCollectionChanged and INotifyPropertyChanged that the ObservableCollection also implements.

This way you'll have a nice new type that encapsulates the entire logic.

public class SpriteCollection<T> : IEnumerable<T>, INotifyCollectionChanged, INotifyPropertyChanged

 private readonly ISet<T> _values;

 public event NotifyCollectionChangedEventHandler CollectionChanged;

 public event PropertyChangedEventHandler PropertyChanged;

 public SpriteCollection(IEnumerable<T> values) => _values = new HashSet<T>(values);

 public T Query(Predicate<T> predicate)
 
 return _values.FirstOrDefault(i => predicate(i));
 

 public T QueryAll(Predicate<T> predicate)
 
 return _values.Where(i => predicate(i)).ToArray();
 

 public void Add(T obj)
 
 if (_values.Add(obj))
 
 CollectionChanged(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add));
 
 

 public IEnumerator<T> GetEnumerator() => _values.GetEnumerator();

 IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

Then you'll need to create a dictionary of these collections in your ObjectService with Type key and object as value.

private readonly IDictionary<Type, object> _spriteCollections = new Dictionary<Type, object>

 [typeof(Aisling)] = new SpriteCollection<Aisling>(Enumerable.Empty<Aisling>()) 

Now you don't need any of the old methods because they now belong to the SpriteCollection so you can replace them with a simple GetSprites method.

public SpriteCollection<T> GetSprites<T>() where T : Sprite

 return (SpriteCollection<T>)_spriteCollections[typeof(T)];

This changes will allow you to reduce the code by more then a half of it.

Disclimer: All examples are just examples, not tested and only for demonstration purposes so they lack null-checks etc. You should carefuly check everything before taking any parts into production.

I don't think that implementing the IDisposable interface by the ObjectService is necessary. Clearing all collections isn't of much use. If you abandon them, the garbage collector will eventually find them. You dispose resources. Clearing collections is just additional work that might hurt performance.

It's hard to comment on the thread safety because there is too little information about the environment the class is supposed to work in. Therefore my answer focus solely on the class construct.

Singleton

1) It is fine to have the static context object as private and the static Method Context. But a pure singleton class should hide the contructor(s) as private in order to prevent undesired instantiation:

// Singleton initialization
// Private because it is a singleton
private static NewObjectService _context;
// Private because it is a singleton
private NewObjectService()




public static NewObjectService GetContext()

 _context = _context ?? new NewObjectService();
 return _context;

2) The Set() method is strange for a singleton class since the ObjectService argument will always be the same (that is the whole idea of a singleton class):

internal static void Set(ObjectService cache_)

 if (CacheLoaded)
 return;

 foreach (var obj in cache_.Items)
 Context.Insert(obj);

 foreach (var obj in cache_.Money)
 Context.Insert(obj);

 CacheLoaded = true;

The Sprite class and inheritance

Since Money, Item, Monster etc. all are subclasses of Sprite, I would only have one set of objects containing all sprites:

private HashSet<Sprite> m_sprites = new HashSet<Sprite>();

This will reduce the code a lot and make it much more clean as shown below.

Events

In C# you should conform to the convention for events:

 public delegate void ObjectEvent(object sender, ObjectEventArgs e);

 public class ObjectEventArgs : EventArgs
 
 public ObjectEventArgs(Sprite sprite)
 
 Sprite = sprite;
 

 public Sprite Sprite get; private set; 
 

and invoke them like this:

public event ObjectEvent ObjectAdded;
private void OnObjectAdded(Sprite sprite)

 ObjectAdded?.Invoke(this, new ObjectEventArgs(sprite));

public event ObjectEvent ObjectChanged;
private void OnObjectChanged(Sprite sprite)

 ObjectChanged?.Invoke(this, new ObjectEventArgs(sprite));

public event ObjectEvent ObjectRemoved;
private void OnObjectRemoved(Sprite sprite)

 ObjectRemoved?.Invoke(this, new ObjectEventArgs(sprite));

Dispose

There is no need to implement IDisposable since the only kind of owned object type is HashSet which is not disposable. I suppose that the object classes aren't disposable since you don't dispose them here.

All in all

All in all I would implement the class something like this (without any threadsafety considerations):

 public sealed class NewObjectService
 
 // Singleton initialization
 // Private because it is a singleton
 private static NewObjectService _context;
 // Private because it is a singleton
 private NewObjectService()
 

 

 public static NewObjectService GetContext()
 
 _context = _context ?? new NewObjectService();
 return _context;
 

 // Fields
 private HashSet<Sprite> m_sprites = new HashSet<Sprite>();

 // Methods
 public T Query<T>(Predicate<T> predicate) where T: Sprite
 
 return m_sprites.Where(s => s is T).Cast<T>().FirstOrDefault(s => predicate(s));
 

 public IEnumerable<T> QueryAll<T>(Predicate<T> predicate) where T : Sprite
 
 return m_sprites.Where(s => s is T).Cast<T>().Where(s => predicate(s));
 

 public void Save(Sprite sprite)
 
 if (sprite == null)
 return;

 OnObjectChanged(sprite);
 

 public void Add(Sprite sprite) // Add instead of Insert
 
 if (sprite == null)
 return;

 // Only add sprites once
 if (!m_sprites.Contains(sprite))
 
 // You problaly need to check if this is already set as it supposedly isn't a good idea to change this value?
 if (sprite.Serial == null)
 
 sprite.Serial = Generator.GenerateNumber();
 

 m_sprites.Add(sprite);

 OnObjectAdded(sprite);
 
 

 public void RemoveAll(IEnumerable<Sprite> sprites)
 
 foreach (Sprite sprite in sprites)
 
 Remove(sprite);
 
 

 public bool Remove(Sprite sprite)
 
 bool result = m_sprites.Remove(sprite);
 if (result)
 
 OnObjectRemoved(sprite);
 

 return result;
 

 public void Cache()
 
 StorageManager.Save(this);
 

 // Events

 public event ObjectEvent ObjectAdded;
 private void OnObjectAdded(Sprite sprite)
 
 ObjectAdded?.Invoke(this, new ObjectEventArgs(sprite));
 
 public event ObjectEvent ObjectChanged;
 private void OnObjectChanged(Sprite sprite)
 
 ObjectChanged?.Invoke(this, new ObjectEventArgs(sprite));
 
 public event ObjectEvent ObjectRemoved;
 private void OnObjectRemoved(Sprite sprite)
 
 ObjectRemoved?.Invoke(this, new ObjectEventArgs(sprite));