Bunch of microoptimizations #1600
Conversation
olefirenque
force-pushed
the
bunch-of-microoptimizations
branch
from
ca140d7
to
b846569
Compare
|
|
||
| // avoiding extra allocations with a static storage for m_covers | ||
| static xr_vector<std::optional<CCoverPoint>> quadTreeStaticStorage; |
There was a problem hiding this comment.
I'm not sure if this is a good trade-off between allocs/RSS. It would be nice to hear an opinion on this
There was a problem hiding this comment.
It's not recommended to use global/static objects which allocate memory dynamically.
| if (!actual()) | ||
| prepare(); | ||
| { | ||
| static std::mutex prepareMutex; | ||
| std::lock_guard lock(prepareMutex); | ||
|
|
||
| // Double-checked locking | ||
| if (!actual()) | ||
| prepare(); | ||
| } |
There was a problem hiding this comment.
I believe it was only non-concurrent part for xr_parallel_for in CSpaceRestrictionShape::fill_shape. Since it is called only once for a sequence of CSpaceRestrictor::inside calls, maybe it would be better to extract this to reduce contention
| xr_vector<bool> m_temp; | ||
| // vector<bool> is not applicable for `m_temp` | ||
| // since it is filled in parallel_for (https://timsong-cpp.github.io/cppwp/container.requirements.dataraces). | ||
| xr_vector<int> m_temp; |
There was a problem hiding this comment.
I suppose it wasn't thread-safe before
| if (m_temp[i] && critical_cover(i)) | ||
| m_covers->insert(xr_new<CCoverPoint>(ai().level_graph().vertex_position(ai().level_graph().vertex(i)), i)); | ||
| for (auto &p : quadTreeStaticStorage) | ||
| if (p.has_value()) |
There was a problem hiding this comment.
It seemed that accessing m_temp and calling critical_cover took some time here
for (u32 i = 0; i < levelVertexCount; ++i)
if (m_temp[i] && critical_cover(i))
m_covers->insert(xr_new<CCoverPoint>(ai().level_graph().vertex_position(ai().level_graph().vertex(i)), i));
olefirenque
force-pushed
the
bunch-of-microoptimizations
branch
from
baf96ec
to
ec99fb2
Compare
olefirenque
force-pushed
the
bunch-of-microoptimizations
branch
from
ec99fb2
to
d9da42f
Compare
| { | ||
| if (inside(graph.vertex_id(&vertex), true) && | ||
| !inside(graph.vertex_id(&vertex), false)) | ||
| m_border_chunk.push_back(graph.vertex_id(&vertex)); | ||
| } | ||
| std::lock_guard lock(mergeMutex); | ||
| if (m_border.capacity() < m_border.size() + m_border_chunk.size()) | ||
| m_border.reserve(m_border.size() + m_border_chunk.size()); | ||
| for (auto x : m_border_chunk) | ||
| m_border.push_back(x); |
There was a problem hiding this comment.
I did a little research on this, and it seemed to me that the body of CBorderMergePredicate::operator(...) is a bigger problem than merging a certain number of chunks under lock
There was a problem hiding this comment.
Ok, it might be irrelevant since I accidentally used UI Freeze option.
I'm sorry for the misinformation. Apparently, this flame graph represents only those samples that were sampled when the NPC's spawning was lagging.
The UI Freeze event indicates time intervals where the application was unable to respond to user input. More specifically, these are time intervals where window messages were not pumped for more than 200 ms or processing of a particular message took more than 200 ms.
There was a problem hiding this comment.
@olefirenque, what profiler did you use?
There was a problem hiding this comment.
To me this strongly looks like tracy but I might be wrong there
olefirenque
force-pushed
the
bunch-of-microoptimizations
branch
from
df2b8ae
to
6f1c24d
Compare
Xottab-DUTY
force-pushed
the
dev
branch
2 times, most recently
from
5b2ec76
to
6fffce9
Compare
|
@olefirenque, hi! Sorry for a big delay in review & merging! |
No description provided.