0

I need to update InstrumentInfo class frequently. I update this class from one thread and access (read) from another.

I have Instrument class. For each Instrument class I need to maintain InstrumentInfo:

// omit class Instrument as not improtant

public class InstrumentInfo
{
    public string Name { get; set; }
    public TradingStatus Status { get; set; }
    public decimal MinStep;
    public double ValToday;
    public decimal BestBuy;
    public decimal BestSell;
}

public class DerivativeInfo : InstrumentInfo
{
    public DateTime LastTradeDate { get; set; }
    public DateTime ExpirationDate { get; set; }
    public string UnderlyingTicker { get; set; }
}

// i do have several more subclasses

I do have two options:

  1. Create only one InstrumentInfo for each Instrument. When some field updates, for example BestBuy just update value of this field. Clients should obtain InstrumentInfo only once and use it during entire application lifetime.
  2. On each update create new instance of InstrumentInfo. Clients should obtain every time the most recent copy of InstrumentInfo.

With 1 I do need to lock, because decimal DateTime string update is not guaranteed to be atomic. But I don't need to reinstatiate object.

With 2 I don't need to lock at all, as reference update is atomic. But I likely will use more memory and I will probably create more work for GC because every time I need to instatiate new object (and initialize all fields).

1 implementation

    private InstrumentInfo[] instrumentInfos = new InstrumentInfo[Constants.MAX_INSTRUMENTS_NUMBER_IN_SYSTEM];

    // invoked from different threads
    public InstrumentInfo GetInstrumentInfo(Instrument instrument)
    {
        lock (instrumentInfos) {
            var result = instrumentInfos[instrument.Id];
            if (result == null) {
                result = new InstrumentInfo();
                instrumentInfos[instrument.Id] = result;
            }
            return result;
        }
    }

    ...........
    InstrumentInfo ii = GetInstrumentInfo(instrument);
    lock (ii) {
        ii.BestSell = BestSell;
    }

2 implementation:

    private InstrumentInfo[] instrumentInfos = new InstrumentInfo[Constants.MAX_INSTRUMENTS_NUMBER_IN_SYSTEM];

    // get and set are invoked from different threads
    // but i don't need to lock at all!!! as reference update is atomic
    public void SetInstrumentInfo(Instrument instrument, InstrumentInfo info)
    {
        if (instrument == null || info == null)
        {
            return;
        }
        instrumentInfos[instrument.Id] = info;
    }

    // get and set are invoked from different threads
    public InstrumentInfo GetInstrumentInfo(Instrument instrument)
    {
        return instrumentInfos[instrument.Id];
    }
    ....
    InstrumentInfo ii = new InstrumentInfo {
        Name = ..
        TradingStatus = ...
        ...
        BestSell = 
    }

    SetInstrumentInfo(instrument, ii); // replace InstrumentInfo

So what do you think? I want to use approach 2 because I like code without locks! Am I correct that I do not need lock at all as I just replace reference? Do you aggree that 2 is preferred? Any suggestions are welcome.

Oleg Vazhnev
  • 23,239
  • 54
  • 171
  • 305
  • just in case you *do* lock, lock on a `private static readonly object` and not on your array of instrument infos - just as a defensive coding practice. – Adam Aug 14 '12 at 09:13
  • @codesparkle - Locking on a `static` object would lead to (much) more contention than necessary. The current approach (`ii` is not the array) looks OK. – H H Aug 14 '12 at 09:15
  • @HenkHolterman he is locking on `lock (instrumentInfos) {` which is an array. `ii` has nothing to do with it. But you're right, it's unnecessary and bad to make it static. readonly would do. – Adam Aug 14 '12 at 09:17
  • You're right, I was looking at the other lock. The `lock (instrumentInfos)` might not be needed at all. – H H Aug 14 '12 at 09:20
  • @HenkHolterman it need to guarantee that exactly one instance is created. If GetInstrumentInfo is called from different threads one thread may obtain copy which is not valid. Of course code may be refactored so I `lock` only if `result == null` – Oleg Vazhnev Aug 14 '12 at 09:29

2 Answers2

2

With 2 I don't need to lock at all, as reference update is atomic. But I likely will use more memory and I will probably create more work for GC because

No, your option 1 is just as likely to cause more load on the GC (by promoting more objects to the next generation).

  1. Use the most sensible, maintainable form. In this case, create new objects.

  2. Do not optimize based on what you 'think' might be slower. Use a profiler.

H H
  • 263,252
  • 30
  • 330
  • 514
2

You should consider several unrelated points.

  1. When you can go without locks, you should go without them, of course. And when you go for multithreading, prefer immutable objects.

  2. On the other side, immutable objects

    • strain memory
    • are considered "anti-OOP"
    • may be incorrectly consumed by client code (because people are not used working with them)
  3. Your second approach still requires some concurrency handling strategy, because several threads may set infos with different starting assumptions.

  4. I am not sure that reference assignment is atomic. If it were, why does CLR have Interlocked.Exchange<T>? Thanks to Henk Holterman for pointing this out.

Sergei Rogovtcev
  • 5,804
  • 2
  • 22
  • 35