-
Notifications
You must be signed in to change notification settings - Fork 432
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
Perf/spec binary search #3647
Perf/spec binary search #3647
Conversation
return spec; | ||
} | ||
public IReleaseSpec GetSpec(long blockNumber) => | ||
_transitions.TryGetSearchedItem(blockNumber, |
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.
unnecessary lambda allocation on GetSpec which is called very often
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.
fixed by static method
public IReleaseSpec GetSpec() | ||
{ | ||
long headerNumber = _blockFinder.FindBestSuggestedHeader()?.Number ?? 0; | ||
if (headerNumber == _lastHeader) |
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.
potential race condition
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.
fixed
if (headerNumber == _lastHeader) | ||
{ | ||
IReleaseSpec releaseSpec = _headerSpec; | ||
if (headerNumber == _lastHeader) |
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.
I don't know the functionality so I need some clarification :)
GetSpec returns the best cached spec.
Does it have to be the latest? We don't have a monitor here, so it can change between ln54 and ln58. Which means we don't care if it's the latest. So if we don't care that it's the latest then we don't need that double check.
} | ||
} | ||
|
||
lock (_lock) |
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.
Again, what if headerNumber changed between ln50 and ln65?
We will overwrite _headerSpec with the previous one.
Changes:
Binary search for spec transitions
Store head spec through calls
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that applyTesting
Requires testing
In case you checked yes, did you write tests??