I have a question of understanding in ConcurrentDictionary

Question

I have a question of understanding. Which is the better option in terms of safety and performance?

private ConcurrentDictionary<string, ProfitTarget> chartTraderTP;

if (chartTraderTP.ContainsKey(orderId))
{
    //Store values for later use in Mouse Events
    chartTraderTP[orderId].OrderLabelRectText = rectTextOrderLabelTP;
    //We can draw the Rectangle based on the TextLayout used above
    if (!chartTraderTP[orderId].IsMovingOrder
        && (ChartTraderDisplayStyle == ChartTraderDisplayStyle.Own
            || ChartTraderDisplayStyle == ChartTraderDisplayStyle.Both))
    {
        RenderTarget.FillRectangle(rectTextOrderLabelTP, tpAreaBrushDx);
        RenderTarget.DrawRectangle(rectTextOrderLabelTP, tpOutlineBrushDx,
            LabelOutlineWidthTP);
        RenderTarget.DrawTextLayout(vectText, tpTextLayout, tpTextBrushDx,
            SharpDX.Direct2D1.DrawTextOptions.NoSnap);
    }
}

or

private ConcurrentDictionary<string, ProfitTarget> chartTraderTP;
//Store values for later use in Mouse Events
if (chartTraderTP.ContainsKey(orderId))
{
    chartTraderTP[orderId].OrderLabelRectText = rectTextOrderLabelTP;
}
//We can draw the Rectangle based on the TextLayout used above
if (chartTraderTP.ContainsKey(orderId) && !chartTraderTP[orderId].IsMovingOrder
    && (ChartTraderDisplayStyle == ChartTraderDisplayStyle.Own
        || ChartTraderDisplayStyle == ChartTraderDisplayStyle.Both))
{
    RenderTarget.FillRectangle(rectTextOrderLabelTP, tpAreaBrushDx);
    RenderTarget.DrawRectangle(rectTextOrderLabelTP, tpOutlineBrushDx,
        LabelOutlineWidthTP);
    RenderTarget.DrawTextLayout(vectText, tpTextLayout, tpTextBrushDx,
        SharpDX.Direct2D1.DrawTextOptions.NoSnap);
}

The code is executed very often, and I would like to make the access as fast as possible but still threadsafe.

I would advise to use `var found = chartTraderTp.TryGetValue(orderId, out var profitTarget);` and if it's found work with the instance you retrieved. — , Mar 02 '21 at 17:32
@Knoop That assumes no other threads are editing the objects in the collection, which doesn't seem to be the case. — Servy, Mar 02 '21 at 17:33
@Servy that's true, but then you just need to lock the object you retrieved instead of the entire `Dictionary`. It's hard to say anything specific without more information about what different threads are doing. — , Mar 02 '21 at 17:34
@Knoop But now you're taking out multiple different locks, which increases complexity quite a bit. It may be needed, it may not be, but saying they can just use `TryGetValue` and be done is pretty unlikely to be the case. — Servy, Mar 02 '21 at 17:37

score 2 · Answer 1 · answered Mar 02 '21 at 17:30

2

Neither are safe, as you're performing multiple operations while assuming the collection isn't changing between operations, which is not a safe assumption.

Since you want to use a number of different operations and need entire section to be logically atomic, just use a regular Dictionary and lock around the access to it.

answered Mar 02 '21 at 17:30

Servy

202,030
26
332
449

`Dictionary`, a `lock`, combined with a single `TryGetValue` should be performant enough. – Moo-Juice Mar 02 '21 at 17:31
you mean lock like below? sorry i am a newbie ;-) – Sidlercom Mar 02 '21 at 18:13
@Sidlercom You probably want to use the `lock` keyword, yes. What you lock on and what goes in it will depend on more than what you've shown. – Servy Mar 02 '21 at 18:17

score -3 · Answer 2 · answered Mar 02 '21 at 18:12

   lock (chartTraderTP)
            {
                //Store values for later use in Mouse Events
                chartTraderTP[orderId].OrderLabelRectText = rectTextOrderLabelTP;
                //We can draw the Rectangle based on the TextLayout used above
                if (!chartTraderTP[orderId].IsMovingOrder && (ChartTraderDisplayStyle == ChartTraderDisplayStyle.Own || ChartTraderDisplayStyle == ChartTraderDisplayStyle.Both))
                {
                    RenderTarget.FillRectangle(rectTextOrderLabelTP, tpAreaBrushDx);
                    RenderTarget.DrawRectangle(rectTextOrderLabelTP, tpOutlineBrushDx, LabelOutlineWidthTP);
                    RenderTarget.DrawTextLayout(vectText, tpTextLayout, tpTextBrushDx, SharpDX.Direct2D1.DrawTextOptions.NoSnap);
                }
            }

Using lock on a `ConcurrentDictionary` negates all its advantages, while preserving its disadvantages (awkward API, overhead). If you have to `lock`, you'd better do it on a normal `Dictionary` instead. — Theodor Zoulias, Mar 03 '21 at 00:06

I have a question of understanding in ConcurrentDictionary

2 Answers2