Avoid a hash key generation and lookup when inserting in the LockingMechanism #18243
Conversation
|
Hi there @Henr1k80, thank you for this contribution! 👍 While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
| { | ||
| // Since we've already checked that we're the parent in the WriteLockInner method, we don't need to check again. | ||
| // If it's the very first time a lock has been requested the WriteLocks dict hasn't been instantiated yet. | ||
| locks ??= new Dictionary<Guid, Dictionary<int, int>>(); | ||
|
|
||
| // Try and get the dict associated with the scope id. | ||
| var locksDictFound = locks.TryGetValue(instanceId, out Dictionary<int, int>? locksDict); | ||
| ref Dictionary<int, int>? locksDict = ref CollectionsMarshal.GetValueRefOrAddDefault(locks, instanceId, out bool locksDictFound); |
There was a problem hiding this comment.
This'll be quite a large comment on one line, but it's a good a place as any to trigger a discussion on the pros and cons of these types of changes!
So on the one had we need to be careful about these sort of "micro-optimisations". We have a lot of eyes on the code, both from HQ and even more from the community, so I do want to be careful we don't make things more obscure, and perhaps make it less accessible for contributions, without a non-negligible benefit. If you were scanning this function here, you might find it a bit harder now to understand what it's doing. Happy to confess I'd never come across the CollectionsMarshal call you'd used here, so I would have had to look it up to see what it was doing.
So perhaps we could look at these changes only for situation where:
- We're on "hot path", where an optimisation, even if small, will be valuable.
- We've considered and discounted any side-effects.
- We add a comment justifying why we are using this code construct... something like:
// We are retrieving the inner dictionary via CollectionsMarshal.GetValueRefOrAddDefault over locks.TryGetValue here
// for improved performance, as this code is called often from the backoffice as operations are made.
// It's safe to do so as items are not added to or removed from the locks dictionary while the ref value is in use.
There was a problem hiding this comment.
Thanks @AndyButland, regarding "hot path", the issue for me is that I would spend more time analyzing if it is a hot path, than just changing the code.
So for me, it would take less time to just do it everywhere possible.
There was a problem hiding this comment.
I have to agree with Andy here, it is really nice to have an optimized app but we do want to know that it matters.
I would spend more time analyzing if it is a hot path
I think that's exactly what we'd like to see 🙈 I understand you love to implement the optimizations, but maintainability is a crucial part for us as well.
In the past we've often asked people who want to do many of these optimizations to also include tests that show the level of improvement (I saw you did that in a different PR as well).
I fully agree with adding a good comment above code that's optimized like this, as it's completely unreadable to me as a proficient but not expert C# programmer. Knowing what code was replaced here in the comment will allow me to understand the new code better. Of course I could try and do a git blame but that's a lot of extra overhead.
There was a problem hiding this comment.
Understandable, I will do some analysis before and comment, if I do more of these types of changes.
I have added comments, is it acceptable now?
When writing to a dictionary, the hash has to be generated again, the bucket has to be looked up and searched for possible existing value. (exact same work for
TryGetValue)By getting a reference to any newly created value, this work is not needed, and the reference can simply be assigned the new value, without any hash generation, bucket lookup and searching.
This is a little change, to check if this kind of code is deemed acceptable.
This can be improved in a lot of places.
I would like to know the balance between having performing code and still have contributors reviewing.
Should everything be readable by graduates, even
privatestuff?Are you allowed to add one extra line for performance, proven, but probably negligible?
Is there a process or tag for performance/new language features, or are you mostly geared for handling new features & fixing bugs?
Prerequisites