Skip to content
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

Do not wait for WriteMessageLog #57

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JeremyMahieu
Copy link
Contributor

@JeremyMahieu JeremyMahieu commented May 21, 2024

This would lead to a very small time save, depending on the database speed, just a few miliseconds each message.

  1. Writing to the MessageLog table causes an insert which for me takes on average about 0.16ms. We would save this 0.16ms several times when starting a session if we just launch a task to do it and disregard the answer. _ = WriteMessageLog means we will fire and forget the task.
    If we get an exception in this task it gets logged anyway, nothing changes there.

  2. Some optimisation where you just need to check if a transaction exists, not actually need to fetch it.

  3. Chose not to add await ... async for all the database calls, becaues this only speeds it up when you have a significant server load. For this project it would indeed be usefull to do it but chose not to do it in this pull request. However now that more function are async it's easier to do it in a later pull request.

Note I did not test any of this yet. Just make sure it compiles. Will test in the coming weeks.

Optimisation when checking existing transactions
@dallmann-consulting
Copy link
Owner

Your implementation basically changes all code files. And async code can be more difficult to debug and you can't just switch this back and forth. I was thinking about this.
How about doing this internally in the WriteMessageLog method? This seems much simpler to me.

@JeremyMahieu
Copy link
Contributor Author

Yeah I can give it a try,

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.

2 participants