parallelize mercury requests in pipeline #9744
Conversation
|
I see that you haven't updated any README files. Would it make sense to do so? |
| var wg sync.WaitGroup | ||
| for i, lookup := range lookups { | ||
| wg.Add(1) | ||
| r.doLookup(ctx, &wg, lookup, i, upkeepResults) |
There was a problem hiding this comment.
Will we be writing to upkeepResults in parallel? Not sure if we need to add a mutex on top of it?
There was a problem hiding this comment.
two quesions:
- it appears that
r.doLookup()is blocking, and therefore we don't actually do the lookups in parallel. A go routine should solve that as amir mentioned. - we only parallelize the the feed lookups on a per-upkeep basis. Would it be possible to parallelize all feed lookups across all upkeeps?
There was a problem hiding this comment.
fixed the go routine.
re @infiloop2
I think we are OK with concurrency bc we only access upkeep results at different indices in each go routine so technically we will not write/read the same memory location in two go routines.
re: @RyanRHall
what this PR tries to do is parallelizing all feed lookups in 1 pipeline run.
if plugin calls this pipeline with several upkeeps (say 10), this will find all the upkeeps which need feed lookup and spawn go routines for all of them.
then in each feed lookup go routine, it will also fetch mercury data for each asset pair requested by user's revert data in parallel. but this is already done in previous PR.
| @@ -96,52 +98,71 @@ func (r *EvmRegistry) feedLookup(ctx context.Context, upkeepResults []EVMAutomat | |||
| continue | |||
| } | |||
|
|
|||
| feedLookup, err := r.decodeFeedLookup(upkeepResults[i].PerformData) | |||
| r.lggr.Infof("[FeedLookup] upkeep %s block %d decodeFeedLookup performData=%s", upkeepId, block, hexutil.Encode(upkeepResults[i].PerformData)) | |||
There was a problem hiding this comment.
nit. will this always be 'block'? As i understand it can be timestamp also based on user revert data?
There was a problem hiding this comment.
this block is from block := upkeepResults[i].Block so this is the block which OCR3 has agreed upon.
the block/time from the revert data is denoted as time in FeedLookup struct
There was a problem hiding this comment.
ah might be better to rename to checkBlock to clarify
|
SonarQube Quality Gate
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
No description provided.