Profile only SequenceTransactions [NIT-2578] #2366
Conversation
cla-bot
bot
added
the
s
anodar
changed the title
|
Sorry, I forgot we adjusted the threshold to 2 seconds. Let's keep it at 2 seconds (but still limit it to only SequenceTransactions) |
anodar
changed the title
…s/nitro into increase-profiling-threshold
|
No worries, changed to 2 seconds. |
execution/gethexec/sequencer.go
Outdated
| block *types.Block | ||
| err error | ||
| ) | ||
| if s.enableProfiling { |
There was a problem hiding this comment.
Can we use config.EnableProfiling here instead? Then we could make this option hot-reloadable.
Co-authored-by: Maciej Kulawik <10907694+magicxyyz@users.noreply.github.com>
Co-authored-by: Maciej Kulawik <10907694+magicxyyz@users.noreply.github.com>
…s/nitro into increase-profiling-threshold
execution/gethexec/sequencer.go
Outdated
| block *types.Block | ||
| err error | ||
| ) | ||
| if s.config().EnableProfiling { |
There was a problem hiding this comment.
nit: we could use a config copy that we already got in this scope to avoid calling the fetcher again
nitro/execution/gethexec/sequencer.go
Line 802 in 874bea0
There was a problem hiding this comment.
Done. Changed other methods as well where it's called more than once.
execution/gethexec/sequencer.go
Outdated
| madeBlock = s.createBlock(ctx) | ||
| } | ||
| if madeBlock { | ||
| nextBlock := time.Now().Add(config.MaxBlockSpeed) |
There was a problem hiding this comment.
here we need to get the config from the fetcher, it's called iteratively in a separate goroutine and we still want to hot-reload it
There was a problem hiding this comment.
Nice catch! Thanks!
(at this point I'm wondering how much overhead would fetching the config have if we were to never cache into a variable, since I can see how easy would it be to introduce a bug like this).
There was a problem hiding this comment.
hm... maybe the overhead is not that bad, but I'd still use the same config pointer for options that are e.g. thresholds and it might be good to keep them in sync (so as two or more of them are guaranteed to be taken from the same config object, without risk of reload race) - not sure if there are such sensitive options (?)
What do you think?
| if s.config().ExpectedSurplusSoftThreshold != "default" && expectedSurplus < int64(s.config().expectedSurplusSoftThreshold) { | ||
| log.Warn("expected surplus is below soft threshold", "value", expectedSurplus, "threshold", s.config().expectedSurplusSoftThreshold) | ||
| config := s.config() | ||
| if config.ExpectedSurplusSoftThreshold != "default" && expectedSurplus < int64(config.expectedSurplusSoftThreshold) { |
There was a problem hiding this comment.
this looks like an example when we need ExpectedSurplusSoftThreshold and expectedSurplusSoftThreshold read from the same config
No description provided.