Skip to content

Conversation

necessarily-equal
Copy link
Contributor

@necessarily-equal necessarily-equal commented Oct 19, 2025

This is a fixed version of #1632, see #1632 (comment) for my own review.

@slipher
Copy link
Member

slipher commented Oct 20, 2025

I stand by my comment from #1632 that improve SendMsg performances by avoiding malloc calls is not needed. It doesn't make sense to micro-optimize something that is only used O(1) times per game. It's a rarely used facility for sending file handles; normal messages don't use it.

@necessarily-equal
Copy link
Contributor Author

necessarily-equal commented Oct 20, 2025

I think part of our disagreement might be because I measured it in DLL mode and you didn't?

With the following I still measure thousands of calls.

diff --git a/src/common/IPC/Primitives.cpp b/src/common/IPC/Primitives.cpp
index cbf9508d7..aae5cff2a 100644
--- a/src/common/IPC/Primitives.cpp
+++ b/src/common/IPC/Primitives.cpp
@@ -233,6 +233,8 @@ static void InternalSendMsg(Sys::OSHandle handle, bool more, const FileDesc* han
                Sys::Drop("IPC: Failed to send message: %s", error);
        }
 #else
+       static int count = 0;
+       printf("used %i times\n", ++count);
        size_t descBytes = 0;
        static std::unique_ptr<unsigned char[]> descBuffer;
        static size_t descBufferSize = 0;

Example output while loading Antares:

used 6850 times
used 65777 times
used 43370 times

Of course when __native_client__ is defined, this part of the code is compiled out, so that specific commit you mention (improve SendMsg performances by avoiding malloc calls) only improves performance in DLL mode.

@slipher
Copy link
Member

slipher commented Oct 20, 2025

I mean that descBuffer is populated with a nonzero number of items only O(1) times.

@necessarily-equal
Copy link
Contributor Author

Aha, okay I get it now, sorry. Fair enough, I've dropped the commit

@slipher
Copy link
Member

slipher commented Oct 21, 2025

InternalRecvMsg reserves before pushing should be dropped for the same reason of handles rarely being used. It turns a vector that only allocates 0.01% of the time into a vector that always allocates.

Other commits LGTM.

@sweet235
Copy link
Contributor

sweet235 commented Oct 21, 2025

I think this project should be more careful with contributed commits such as these. #1632 was openend by someone who is still on good terms with the original author. The participants in this PR thread burnt all bridges to the original author. I think it would be better to talk to people before using their work as is done here.

For the fun of it, I asked the original author about this PR. This is their reply:

I have been hurt badly in the past by him trying to "help" by porting some of my changes... he added random changes AND neglected to pick a bugfix that came later in the branch

and add to it: he squashed everything, so it became really hard to track down what he did exactly, or to solve git conflicts

thanks to that, I have lost many hours if not days

I am not quite sure you even have consensus with the author here. Please be so kind and talk to them.

Edit: I had forgot "hard" in "really hard". Originally, this had been "hard as f***", which I censored with permission, to adhere to the dictator's rules. Of course, I forgot a word while doing that.

@necessarily-equal
Copy link
Contributor Author

necessarily-equal commented Oct 21, 2025

Hmm, fair enough. I'll close this and let @VReaperV handle things in #1632, either by doing something or not. That's what I initially starting doing, then one thing leading to another I ended up opening this.

(had to fix the code to have it build, so I wanted to push a branch, which lead me to squash the commit as well while at it before pushing, then I wrote in my comment that I had a corrected branch, I sent the [comment](https://github.com//pull/1632#issuecomment-3419522985) and when github suggested to open a PR I thought a bit about it and realised I almost had a PR already, so might as well open one as it could be faster)

Thank you for trying to put it in a more diplomatic way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants