Remove temporary buffer from AddConvexPolyFilled

[G-glop]

Math stays the same, only other change is the early exit, to prevent reading a zero sized array when initializing n0.

When i have it in my head: could it also auto detect winding order?
Currently it assumes clockwise winding, which is ok, but isn't documented anywhere.

Long overdue update:

Firt i want to say that your code has pretty much optimal performace, as in number of operations and throughput. That's why my first proposal failed abysmally, it both needed to compute one more sqrt, which matters when half of the inputs have 3 or 4 points, and was painfully serial.

So an intermediate buffer is necassary. But what about alloca? Well the key realization is that while another buffer is needed, more memory is not. That's what i did last year, re-use the vertex buffer to also store the normals. What i didn't do, and which prevented me from matching the original performace, was this:
ImVec2* temp_normals = (ImVec2*)((size_t)(_VtxWritePtr + vtx_count) & ~(alignof(ImVec2) - 1)) - points_count;
Which is again reusing the vertex buffer, but now it's nice, transparent and tightly packed.

My question to you is: "Should ImGui do this type of memory aliasing?".
(and, ugh, how to do it in a portable way - just say that ImVec2 is always 4 byte aligned?)

I also tried reusing the vertex buffer in the same way, and just inlining all the vector operators in the original.

Performance testing:
Using example_null as a base, a loop of NewFrame, 3x ShowDemoWindow, Render. Additonaly ImDrawList::AddPolyline and ImFont::RenderText are stubbed out to amplify the effect of AddConvexPolyFilled. The loop runs until 5 seconds have passed (batched to avoid the overhead of getting current time). Both Visual Studio 2017 and 2015 are tested (v141 and v140)

Signed,
an annoying GitHub user.

[ocornut]

Hello,

Thanks for submitting this.
What are you trying to solve? Assuming it is meant to be a performance improvement: Do you have a process and measurements to share comparing the speed of this version and the current one? I typically compare performance on both fully optimized and debug builds.

EDIT What was your intent with removing the alloca() ? alloca is probably one of the fastest possible thing one could possibly to do (it's 1 addition to increment the stack pointer) the data is in the stack, and the second loop will read back from data freshly written to the cache. However your single loop would also probably be a beneficial thing to explore.

As a crude performance test, I have bound the two variants of the code on the CTRL key (adding if (ImGui::GetIO().KeyCtrl) {} in that function to select the algorithm) and modified the ShowExampleAppCustomRendering() to keep only the filled primitives and display everything 200 times, then measured average time over 6 seconds:

200 iterations of rendering all the filled shapes:

Win32 Release Optimized:
Before: 1.48 ~ 1.50 ms
After: 1.66 ~ 1.69 ms (slower)

Win64 Release Optimized:
Before: 1.21 ~ 1.24 ms
After: 1.35 ~ 1.38 ms (slower)

Win32 Debug Non-Optimized (with MSVC debug cruft)
Before: 7.27 ~ 7.31 ms
After: 7.70 ~ 7.74 ms (slower)

etc.
So this would need to be reworked because right now unless I messed up my tests this is slower every time.

• avoid the modulo in the loop like the original code did
• for the Debug perf, avoiding calling ImVec2() constructors would probably also be beneficial
• the test for the last point should be moved outside the loop (the [2] [4] [5] operators would need to be offset probably by points_count >= 2 ? -9 : -6 (untested)
• etc.

Also smaller/other things that would need be fixed later:

• use imgui coding style (braces, spaces)
• squash the dozens commits into one (14 commits?!)

Thanks,
Omar

[ocornut]

About this:

When i have it in my head: could it also auto detect winding order?
Currently it assumes clockwise winding, which is ok, but isn't documented anywhere.

I have added a comment about anti-aliased filling requiring a path in clockwise order.
I don't think we can detect clockwiseness without a perf hit. We could eventually provide different primitives to either fill a CCW path or optional detect and apply AA properly in all situation but I don't think there is a need for it now.

[G-glop]

Removing alloca is mostly for compatibility. (it's only other use is in AddPolyline, which can be modified in the same way)

Performance shouldn't have changed. Doing register-register ops ins't a win when pipelining hides the latency of reading memory.

I made a benchmark of processing a rounded rectangle. (not yet pushed) And MSVC's sampling profiler shows 10% of the whole run time is spent on if(scale < 100.0f), but only in some configurations, otherwise performance is identical.

But i'm only measuring the pure run time of the function itself, maybe the fact that the fill triangles are interleaved with the outline degrades performance? If so, it's easy to de-interleave them, it also may be worthwhile to also use a different algorithm that doesn't produce long, thin triangles.

[ocornut]

Removing alloca is mostly for compatibility. (it's only other use is in AddPolyline, which can be modified in the same way)

Which compatibility issue do you have with alloca ?
I don't mind changing the rendering function especially if we get a small boost or a simplification, but if it's for compatibility we need to also state what the compatibility issue is.

But i'm only measuring the pure run time of the function itself, maybe the fact that the fill triangles are interleaved with the outline degrades performance? If so, it's easy to de-interleave them,

I guess it's more reasonable and representative of real use to keep them interleaved. Regardless, a simple manufactured/looping test would probably fit in the instruction cache anyway even with multiple functions running.

it also may be worthwhile to also use a different algorithm that doesn't produce long, thin triangles.

What's the problem with long thin triangles?

Mildly unrelated, but if you fancy looking at it, the non-AA thick strokes are broken (see #288, one of the oldest PR alive, it was submitted just at the time we switched to AA primitives).

[G-glop]

Sorry for taking so long, i got sidetracked trying (and failing) to install vTune, to see what's really slowing it down.

But reading the disassembly i think i know what's going on. In the old version the normal computing loop got unrolled to call sqrtf 5 times each iteration (nevermind that sqrtf is a wrapper around the AVX sqrtf instruction which computes 8 sqrtfs at once). Meanwhile my version has a critical path of computing the normals sequentially.

Without doing explicit SIMD (intrinsics/ispc) i don't think i can get a (reliable) performance boost. Maybe a compromise with a small fixed size buffer will yield the best result.

About the compatibility issue; alloca itself is fine (tho someone will be surprised when using a small stack and drawing complex polygons), the issue reolves around the implied __chkstk_ms. Windows only reserves one page above the stack to commit in when the stack grows. This all works fine untilyou have stack frames larger than 4kB, then you could access memory beyond the reserved page and cause a seqfault, so chkstk is inserted to sequentially touch the pages.

MinGW has (had?) a problem with properly handling it, causing link errors.

And lastly the short version why long thin traingles aren't so great:

GPU's first determine which 8x8 tiles a triangle overlaps, in those tiles it applies the same process but for 2x2 groups of pixels, if the traingle is overlapping a group then the shader is executed for all the pixels in that group and pixels outside the triangle are discarded.

Additinaly mobile/newer generation GPUs use tile renderers, which add another level with even larger tiles designed to fit in cache.

long source

So long thin triangles hit a lot of tiles and groups, but contribute little pixels to the output.

[G-glop]

Ok, i think i will leave it at this. Using the output as a buffer gets slightly worser performance when it's the only thing in cache and slightly better performace when there's cache pressure.

Should i do the same for AddPolyLine?

[ocornut]

Long overdue update:
Firt i want to say that your code has pretty much optimal performace, as in number of operations and throughput. That's why my first proposal failed abysmally, it both needed to compute one more sqrt, which matters when half of the inputs have 3 or 4 points, and was painfully serial.
So an intermediate buffer is necassary. But what about alloca? Well the key realization is that while another buffer is needed, more memory is not. That's what i did last year, re-use the vertex buffer to also store the normals. What i didn't do, and which prevented me from matching the original performace, was this:
ImVec2* temp_normals = (ImVec2*)((size_t)(_VtxWritePtr + vtx_count) & ~(alignof(ImVec2) - 1)) - points_count;
Which is again reusing the vertex buffer, but now it's nice, transparent and tightly packed.

While wandering into old issues I noticed you post an interesting update in Dec 2018 which was made as an edit of the original post therefore without any notification coming. We'll investigate that new idea you posted in your edit.

[G-glop]

I forgot to explain the different plotted quantities!

Quantity	Description
initial inline	Original function but with everything inlined, essentially what 9ad3419 did
joined	Compute normals on the fly (initial idea of the PR)
vtx reuse safe	Store normals in ImDrawVert::pos, iteration order avoids clobbering
idx reuse	Store normals in a buffer typecast from _IdxWritePtr
vtx reuse	Store normals in a buffer typecast from _VtxWritePtr and shifted such that writing to _VtxWritePtr doesn't clobber it too soon (what the PR currently does, modulo meaningless formatting changes)

More on compatibility: using alloca apparently requires a preprocessor include dance and silencing 3 warnings. Take your pick if it's better or worse than some cheeky buffer reuse, with or without typecasting.

I need to merge master into my branch and re-run the benchmarks. But overall I like the "idx reuse" strategy the most because it can't as easily suffer clobbering. Just generate all the vertices first and then the indexes. It's almost a drop-in replacement even for the much more complicated AddPolyline.

Thank you for taking a look at almost a 3 year idle PR!

[rokups]

I profiled it with Tracy. Profiler instrumentation was added only around two loops that use temp_normals. I do not see a meaningful difference here.

This trace is original code.
External trace is code from this PR.

Debug

Release

Unsure how to implement total time metrics, maybe they are different because profiling sessions were not exactly equal, but average execution times are nearly same.

[ocornut]

I was curious to have another look at this today, but it has become very confusing to follow because:

• posts have been edited + proposed PR have been changed multiple times, it's hard to make sense of any measurement and comparisons done.
• imgui code itself has been changed (we don't use alloca anymore)

As of today if I look at the leftover diff, the only difference becomes:

-- ImVec2* temp_normals = (ImVec2*)alloca(points_count * sizeof(ImVec2))
++ ImVec2* temp_normals = (ImVec2*)((size_t)(_VtxWritePtr + vtx_count) & ~(alignof(ImVec2) - 1)) - points_count;

But this doesn't apply the same to current code.

Considering the last posts from from April 2022 suggested no measurable difference I think we can close this.
At this point after this has been simplified down to moving that temporary buffer merely to a different location, I don't believe it would make any difference. Let me know if you don't agree.