Refactor array slicing code to reversed arrays with pop - #157
Conversation
| "prom-client": "15.0.0", | ||
| "react": "18.2.0", | ||
| "rescript": "11.1.0", | ||
| "rescript": "11.1.3", |
There was a problem hiding this comment.
The upgrade was needed because I came across a bug that appears to be fixed now. See this example: https://rescript-lang.org/try?version=v11.1.0&code=DYUwLgBAxgTiCGYQCFFQBYEkB2SbfmAgF4IAKAPwFt4APVMDAZQEsAvEAGggoHNwAciFpgAlCQB8EAN4AoCBFCQARmnQkIAbQC68xeAhwoigPYmADhrLjiEvQpYAzCKsboAtBICCMGPACeAHSg2Lxg6hKkNPRqrBwy9gouaokAvhAgwADOIAlJSVkA7ixuEPxgQiLWefkQAD4QAibYubbJbokKDUwmVCBkZCUgVNzmFphIVADyjo4AigCuIEuiNna1SWPmE8Mz80tL1p1JrhieAFJZgT5+-gBMgeYLWeiDk6KeLLzYJnDHCsAzOYjhtUmk9ICLEcwUA
It tries to set _param=undefined and breaks at runtime
There was a problem hiding this comment.
I'll do a release for Fuel indexer tomorrow to prevent it from breaking
| /** | ||
| Currently a bug in rescript if you ignore the return value of spliceInPlace | ||
| https://github.com/rescript-lang/rescript-compiler/issues/6991 | ||
| */ | ||
| @send | ||
| external spliceInPlace: (array<'a>, ~pos: int, ~remove: int) => array<'a> = "splice" |
There was a problem hiding this comment.
Also a bug in the compiler that isn't solved by upgrade: rescript-lang/rescript#6991
DZakh
left a comment
DZakh
left a comment
There was a problem hiding this comment.
A few nitpicks but looks really good!
| latestFetchedBlock, | ||
| firstEventBlockNumber, | ||
| fetchedEventQueue: Array.concat(self.fetchedEventQueue, newFetchedEvents), | ||
| fetchedEventQueue: Array.concat(newFetchedEvents->Array.reverse, self.fetchedEventQueue), |
There was a problem hiding this comment.
Letting you know that Array.reverse mutates the original array. I don't know whether it's a problem here
There was a problem hiding this comment.
Interesting, this is Belt.Array.reverse since it's opened at the top, is that the same?
There was a problem hiding this comment.
Checked the source code. Belt is fine, it's immutable
There was a problem hiding this comment.
Confirming I've tested and it does not mutate 👍🏼
| () => {...self, fetchedEventQueue: self.fetchedEventQueue->Array.sliceToEnd(1)}, | ||
| Item(head), | ||
| ) | ||
| let getEarliestEventInRegisterWithUpdatedQueue = (self: t) => |
There was a problem hiding this comment.
It's the same as getEarliestEventInRegister
| a->EventUtils.getEventComparatorFromQueueItem <= b->EventUtils.getEventComparatorFromQueueItem | ||
| }, unprocessedBatch, state.chainManager.arbitraryEventQueue) | ||
| a->EventUtils.getEventComparatorFromQueueItem > b->EventUtils.getEventComparatorFromQueueItem | ||
| }, unprocessedBatch->Array.reverse, state.chainManager.arbitraryEventQueue) |
There was a problem hiding this comment.
The same here. Array.reverse mutates the array
| ->Option.map(((item, index)) => { | ||
| FetchState.item, | ||
| popItemOffQueue: () => queue->Utils.Array.spliceInPlace(~pos=index, ~remove=1)->ignore, | ||
| }) |
There was a problem hiding this comment.
It also creates an object here, so theoretically, the solution with ArraySlice should be even faster. The only challenge would be to pop items somewhere in the middle, as you do here. It'd require additional changes in the queue datastructure, by having a separate queue by chain, so I agree that your current solution is better 👍
There was a problem hiding this comment.
Yeah, I thought about this. But this case is only hit during unorderd multichain mode after a dynamic contract registration. So we definitely shouldn't be optimizing for this case off the bat.
Co-authored-by: Dmitry Zakharov <dzakh.dev@gmail.com>
2 participants
Uh oh!
There was an error while loading. Please reload this page.