planetarium / libplanet

Blockchain in C#/.NET for on-chain, decentralized gaming
https://docs.libplanet.io/
GNU Lesser General Public License v2.1
506 stars 142 forks source link

Let IRenderer<T> turn on and off evaluating actions #967

Closed dahlia closed 4 years ago

dahlia commented 4 years ago

BlockChain<T>'s render option, replaced by renderers option (in the patch #963), purposed to prevent unnecessary action evaluation if rendering is not needed. Although passing renderers: null or renderers: Enumerable.Empty<IRenderer<T>>() shares the same behavior to the previous usage render: false, two ways do not have the same performance. Further, even if actions do not need to be rendered at all and only new tips need to be rendered (i.e., only RenderBlock() is used), BlockChain<T> anyway evaluates actions because it cannot be aware whether {Render,Unrender}Action{,Error}() methods are empty or has any code…

IRenderer<T> should be able to entirely turn off evaluating actions when it needs. Here are my ideas:

  1. Add bool IRenderer<T>.RendersAction() method. However, it probably is prone to cause bugs like making RendersAction() returns false by mistake while filling out {Render,Unrender}Action{,Error}() methods.

  2. Extract IRenderer<T>.{Render,Unrender}Action{,Error}() methods into a new interface like IActionRenderer<T>, and add ActionRenderer property to IRenderer<T>. If IRenderer<T>.ActionRenderer is null it means actions entirely do not need to be evaluated at all. The downside is that it makes API more complex than now.

@planetarium/libplanet What's your preference? Or any ideas?

longfin commented 4 years ago

Extract IRenderer.{Render,Unrender}Action{,Error}() methods into a new interface like IActionRenderer, and add ActionRenderer property to IRenderer. If IRenderer.ActionRenderer is null it means actions entirely do not need to be evaluated at all. The downside is that it makes API more complex than now.

I think it's better to separate action rendering and block events. also, I'm not sure if we need a hierarchy...

dahlia commented 4 years ago

I'm not sure if we need a hierarchy...

You mean to make IRenderer<T> interface to have only block/reorg render methods, and add a new interface something like IActionRenderer<T> which inherits IRenderer<T> and has other action render methods?

longfin commented 4 years ago

I'm not sure if we need a hierarchy...

You mean to make IRenderer<T> interface to have only block/reorg render methods, and add a new interface something like IActionRenderer<T> which inherits IRenderer<T> and has other action render methods?

Right.

moreal commented 4 years ago

The second way seems better to me.

dahlia commented 4 years ago

On composition vs inheritance, one of key differences I immediately strike is about its calling-side.

Composition:

var actionRenderers = Renderers
    .Select(r => r.ActionRenderer)
    .OfType<IActionRenderer<T>>();  // Filter out nulls
if (actionRenderers.Any())
{
    // Evaluate actions only if there is any action renderers...
    var evaluations = EvaluateActions();

    foreach (var evaluation in evaluations)
    foreach (var actionRenderer in actionRenderers)
    {
        actionRenderer.RenderAction(evaluation.Action, evaluation.InputContext, evaluation.OutputStates);
    }
}

Inheritance:

var actionRenderers = Renderers.OfType<IActionRenderer<T>>();
if (actionRenderers.Any())
{
    // Evaluate actions only if there is any action renderers...
    var evaluations = EvaluateActions();

    foreach (var evaluation in evaluations)
    foreach (var actionRenderer in actionRenderers)
    {
        actionRenderer.RenderAction(evaluation.Action, evaluation.InputContext, evaluation.OutputStates);
    }
}

In this perspective, inheritance looks slightly conciser than composition… 🤔

dahlia commented 4 years ago

I'm going to the third way, inheritance.