-
Notifications
You must be signed in to change notification settings - Fork 151
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
fix(/blocks
): cache extrinsic base weight constants to improve performance
#478
Conversation
…ons for extrinsicBaseweight metadata
Benchmarks for the final additions. /blocks
|
src/services/blocks/BlocksService.ts
Outdated
const len = block.extrinsics[idx].encodedLength; | ||
const weight = weightInfo.weight; | ||
|
||
const partialFee = calcFee.calc_fee( | ||
BigInt(weight.toString()), | ||
len, | ||
extrinsicBaseWeight.toBigInt() | ||
calcFeeExBaseWeight | ||
); | ||
|
||
extrinsics[idx].info = api.createType('RuntimeDispatchInfo', { |
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.
@TarikGul ^^^ didn't pan out? Or am I wrong?
src/types/util/Option.ts
Outdated
@@ -0,0 +1 @@ | |||
export type Option<T> = T | null; |
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.
So there is no such type in Typescript itself?
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.
Yea it doesn't exist out of the box.
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.
Maybe we should make a PR for Option
to TypeScript ;) Native support for this would be very nice.
Co-authored-by: David <dvdplm@gmail.com>
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.
LMK when its ready for a thorough review - just mostly focused on responding to open comments for this one
src/types/util/Option.ts
Outdated
@@ -0,0 +1 @@ | |||
export type Option<T> = T | null; |
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.
Maybe we should make a PR for Option
to TypeScript ;) Native support for this would be very nice.
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
src/services/blocks/BlocksService.ts
Outdated
extrinsicBaseWeightExists === undefined || | ||
!perByte || | ||
!extrinsicBaseWeightExists || | ||
(this.minCalcFeeRuntime && specVersion < this.minCalcFeeRuntime) || | ||
typeof api.query.transactionPayment?.nextFeeMultiplier?.at !== 'function' |
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.
We try and query this on line 491 and with the ?
it should just be undefined
if it does not exist, so we can should just be able to check multiplier
const metadata = await api.rpc.state.getMetadata(blockHash); | ||
const { | ||
consts: { system }, | ||
} = expandMetadata(api.registry, metadata); |
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.
We should keep an eye on what registry we use here in the future. There is a chance it may be better to pull the registry off block
we get from the rpc response since that should be guranteed to have the exact types for the runtime. For now I think this is fine.
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.
Everything looks good to me.
I tried making suggestions via the github vscode extension but botched it somewhere along the way and ended up just making a commit: 20feb8e
Mostly just updates to comments, and variable names. But I did take out that giant try catch
when we loop through extrinsics. I honestly think if that is catching we are not doing the correct checks prior so my proposal is to remove it and if it becomes a production issue we can address it
/blocks
): cache extrinsic base weight constants in order to improve performance
/blocks
): cache extrinsic base weight constants in order to improve performance/blocks
): cache extrinsic base weight constants to improve performance
@@ -508,7 +503,7 @@ export class BlocksService extends AbstractService { | |||
!perByte || | |||
!extrinsicBaseWeightExists || | |||
(this.minCalcFeeRuntime && specVersion < this.minCalcFeeRuntime) || | |||
typeof api.query.transactionPayment?.nextFeeMultiplier?.at !== 'function' | |||
!multiplier |
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.
👍
import { polkadotMetadataRpcV16 } from './polkadotV16Metadata'; | ||
import { polkadotMetadataRpcV29 } from './polkadotV29Metadata'; |
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.
👍
@@ -1 +1,2 @@ | |||
export type Option<T> = T | null; | |||
// Not to be confused with the polkadot-js `Codec` extending `Option` type. |
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.
good pt!
@@ -1 +1,2 @@ | |||
export type Option<T> = T | null; | |||
// Not to be confused with the polkadot-js `Codec` extending `Option` type. | |||
export type IOption<T> = T | null; |
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.
So what is the TS convention for when to use the I
prefix?
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 thinks its more just a me convention that I have implicitly picked up from reading other TS
code - I
is really for interface, but I use it both for interface or types to avoid name collisions with classes / functions etc.
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.
So in typescript (this is quite the debate apparently), "I" should refer to interface's only, and when its not there it will refer to an impl
. Even though this example doesn't fit those parameters the use of 'I' here is to make sure there is a distinguishing difference as stated in the comment.
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.
lgtm, good job.
This PR aims to fix a regression that happened when v2.1.2 was released. ie: commit.
getBlockWeights
intoBlockService
with passing tests before passing in a cache config.expandMetadata
calls for unknown runtimes.Update on benchmarks: After integrating
getBlockWeights
I made sure to do some benchmarking. I chose to target block1907499
which has a staking bondExtra tx (Note I am querying a single block here). I ran a 30s Benchmark with 4 threads and 30 concurrent connections. (This was also done on my local Macbook pro which is extremely fast)To wrap this PR I ran a new set of benchmarks from our standard collection of 39 blocks that are used in
Sidecar-Bench
. I also ran it inside of our GCP instance, to recreate a standard environment. This benchmark is 2 minute's with 4 threads and 12 concurrent connections (Which is our standard test for Sidecar in GCP).