The fact I used a copy of MeshBufferCache mbc, is to be able to set the pointers to null for non requested buffers (like already filled ones). See DRW_TEST_ASSIGN_VBO & DRW_TEST_ASSIGN_IBO.

About passing it to all callbacks, I don't know. It's less clear what the callbacks should be working with to avoid race condition. One could think you can access the whole mbc in a thread safe manner.

Although I do agree the void * are less than ideal.

In D11375#290778, @Germano Cavalcante (mano-wii) wrote:

In D11375#290758, @Clément Foucault (fclem) wrote:

It's less clear what the callbacks should be working with to avoid race condition. One could think you can access the whole mbc in a thread safe manner.

In fact, this is less clear.
Passing a simple void *buf implies that the extract is an individual operation that only writes to that specific buffer. Thus, the pointer is entirely in charge of mesh_buffer_cache_create_requested.

Well isn't it the case?

If it is a mandatory design, I will think of an alternative solution. But something I noticed is that, if we join the different callbacks in the same respective loop (looptri, poly, ledges, lverts) it can result in a significant improvement in performance (Something around 34% faster).
So it might be a good idea to rethink the idea of keeping these extracts independent.

So you mean having a general loop and have the callbacks inside the loop?

In D11375#290790, @Clément Foucault (fclem) wrote:

Well isn't it the case?

Yes, I was just confirming!

In D11375#290790, @Clément Foucault (fclem) wrote:

So you mean having a general loop and have the callbacks inside the loop?

That is the idea.
I made a test patch for profiling this, it is not presentable, but I could see the benefit.
The file I am testing (high poly mesh in edit mode with several overlays), performance went from 53 to 36ms with room for improvement:

To make the patch presentable I am looking for an alternative solution (perhaps replacing all extracts since they can no longer be independent).

Ah I see. So eventually a future patch will improve data localization with the effect of more branch prediction misses.

In pseudo:

for batch (a scheduled task) {
  per loop type
     for multithreaded_requested_extractors {
        multithreaded_requested_extractors.loop type function()
     }
}

There are a lot of possibilities to increase the performance here. Like grouping and ordering. I assume you are referring to this when talking about room for improvements?

An idea we should discuss is to have a template class for this.

template<typename Buffer, typename Data=void> class MeshBatchExtractor {
    virtual Buffer* get_buffer(MeshBufferCache& mbc) = 0;
    virtual void finish(const MeshRenderData *mr,
                        MeshBatchCache *cache,
                        Buffer *ibo,
                        Data *data) = 0;

};

class MyExtractor: public MeshBatchExtractor<GPUIndexBuffer, void> {
    GPUIndexBuffer* get_buffer(MeshBufferCache& mbc) override {
        /* returns a reference of an indexbuffer in MBC, fe `&mbc.ibo.tris`;
         * this reference will be passed to the callback functions. */
        return nullptr;
    }

    void finish(const MeshRenderData *mr,
                MeshBatchCache *cache,
                GPUIndexBuffer *ibo,
                void *data) override {}
};

This patch optimize ram access before branch prediction and still leave room to optimize further. I think this patch is something we should strife for, great idea and initial patch! Although I have some concerns on readability and maintainability.

The part of the code that I don't like is that you need changes in many areas when adding or changing extractions. This adds unclarity to other devs. I would rather see an approach where an extraction is configured centrally but still can be used in this mechanism. As this file is getting big this we should we should be able to place all the relevant pieces of an extraction in separate compile units. I don't expect the compilation unit separation part of this patch, but would expect a solution that doesn't block us in doing this.

Idea: you can increase branch prediction and increase the code centralization by replacing the wall of if statements with a for loop over extractor structs that are enabled for that specific task.

In D11375#291763, @Germano Cavalcante (mano-wii) wrote:

• Cleanup: rename variables, rearrange code, supress warnings
• Simplify the task node that initializes extracts
• Alloc the main data of the extract only once

It would be nice to convert the macros to enums, but we have very high values like (1ULL << 37).

typedef enum eExtractionType: long long {
M_EXTRACT_POS_NOR = (1ULL << 0),
M_EXTRACT_POS_NOR_HQ = (1ULL << 1),
M_EXTRACT_LNOR = (1ULL << 2),
M_EXTRACT_LNOR_HQ = (1ULL << 3),
M_EXTRACT_ORCO = (1ULL << 4),
M_EXTRACT_WEIGHTS = (1ULL << 5),
M_EXTRACT_EDIT_DATA = (1ULL << 6),
M_EXTRACT_EDITUV_DATA = (1ULL << 7),
M_EXTRACT_FDOTS_POS = (1ULL << 8),
M_EXTRACT_FDOTS_UV = (1ULL << 9),
M_EXTRACT_FDOTS_EDITUV_DATA = (1ULL << 10),
M_EXTRACT_POLY_IDX = (1ULL << 11),
M_EXTRACT_EDGE_IDX = (1ULL << 12),
M_EXTRACT_VERT_IDX = (1ULL << 13),
M_EXTRACT_FDOT_IDX = (1ULL << 14),

/* Single Thread. */
M_EXTRACT_TRIS = (1ULL << 15),
M_EXTRACT_LINES = (1ULL << 16),
M_EXTRACT_LINES_WITH_LINES_LOOSE = (1ULL << 17),
M_EXTRACT_POINTS = (1ULL << 18),
M_EXTRACT_FDOTS = (1ULL << 19),
M_EXTRACT_LINES_PAINT_MASK = (1ULL << 20),
M_EXTRACT_LINES_ADJACENCY = (1ULL << 21),
M_EXTRACT_EDITUV_TRIS = (1ULL << 22),
M_EXTRACT_EDITUV_LINES = (1ULL << 23),
M_EXTRACT_EDITUV_POINTS = (1ULL << 24),
M_EXTRACT_EDITUV_FDOTS = (1ULL << 25),
M_EXTRACT_UV = (1ULL << 26),
M_EXTRACT_TAN = (1ULL << 27),
M_EXTRACT_TAN_HQ = (1ULL << 28),
M_EXTRACT_SCULPT_DATA = (1ULL << 29),
M_EXTRACT_VCOL = (1ULL << 30),
M_EXTRACT_EDGE_FAC = (1ULL << 31),
M_EXTRACT_EDITUV_STRETCH_AREA = (1ULL << 32),
M_EXTRACT_EDITUV_STRETCH_ANGLE = (1ULL << 33),
M_EXTRACT_FDOTS_NOR = (1ULL << 34),
M_EXTRACT_FDOTS_NOR_HQ = (1ULL << 35),
M_EXTRACT_MESH_ANALYSIS = (1ULL << 36),
M_EXTRACT_SKIN_ROOTS = (1ULL << 37),
} eExtractionType;

Seems to work in CPP, but I rather use a enum class. So we can just add a comment.
After this patch I want to convert the solution to CPP and do some clean up passes.

In order to make this patch good for master I would like the MeshExtract back in order to make the code more maintainable.
You can make a list of a struct containing a MeshExtract references + the result of the init function and iterate over them.
When converting to CPP we can get the typing back in the functions.

Not for this patch, but to make sure we are on the same page, the goal split for this file would be (scheduling, render mesh data, and a file per extractor type) ideally the extractor would be header only files. On planning I would have done this before you started with this overhaul, but did had time to draft the final solution.

Note that the loop macros may only be called once in this solution and we might want to get rid of them after this patch landed.