-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ApplicationMessenger separate queue and target locking #15356
Conversation
…ue from that for protecting targets
4070463
to
c5a2af9
Compare
Of course, sorry. This happens when I suggest code without the help of an IDE :-)
|
Should this be merged? |
It has been fine in a test build, so I would say yes merge |
This has also been in Millhouse and tested separately. The gains may be small, but good to go into v18.1 |
} | ||
if (target != nullptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did the same mistake again. A copy is worth nothing. At the time you use the pointer, the target might have disappeared already.
(I know that there is no Unregister atm, but this needs fixing)
I bet you can't show any measurable improvement of this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FernetMenta the copy is not new, just the way the lock applied. Other guys suggested a loop change and that seemed reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the lock (used whren register etc,) is not sufficient to protect the list of targets then what is?
My change is simply to use a separate lock for adding events to the queue, no reason for sending messages to prevent other messages being queued.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dispatching massages here is done by the main/render thread. Nothing must block or hijack this thread for processing intensive tasks. Hence the benefit of having separate locks is nil and I bet that you can't prove the opposite. Even if you could measure any improvement, it would just show that some other code blocks the main thread.
In other words, you code for something that must not happen. In addition you don't fix the obvious errors that is leaving the lock for dispatching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hence the benefit of having separate locks is nil and I bet that you can't prove the opposite.
On a very slow system I did see improvements. Does that mean some hidden issue elsewhere, I don't know.
In other words, you code for something that must not happen.
Does that mean is does not happen? Can you prove it?
In addition you don't fix the obvious errors that is leaving the lock for dispatching.
You blame me for not fixing enough? Thanks, but when did I become solely responsible. Please either be constructive or butt out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FernetMenta AFAIK I have not added errors (check the original code), but if more can be done then do share what.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It must not happen <- if it happens, fix the root cause and don't introduce more complexity via an additional lock.
If something can be done without the lock, please keep using SingleExit.
While your communication is not optimal - the code change is - in my eyes - not an optimization because "optimization of locking" does not mean add more fine grain locking, but REMOVE not needed locking ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify: "your" <- yours and fernet's communication.
This has been in the Millhouse builds since Jan without causing any problems. Holding it back from v18 RC1 may have been overly cautious afterall, but I respect the philosophy. My choice of title was bad - "optimise" was not intended literally, and I have no proof this performs better than before - so I have rephrased. Lack of performance evidence not withstanding, I still think that it is better design to use separate locks for queue from that for protecting targets.
I don't understand what this is refering to, and accept I don't know enough about these aspects of Kodi to dare to meddle, but it makes me think it is worth leaving this open as WIP to see if greater minds than mine have something to add. @jimfcarroll perhaps you can take a look on the back of your threading changes? |
@DaveTBlake this needs a rebase |
@DaveTBlake, do you mind if I apply the backburner label and close? Or would you rather keep it open? |
Optimize locking in ApplicationMessenger by using separate locks for queue from that for protecting targets.
Edit: "optimise" was a bad choice of word, it implied far more than I intended