Remove compacting of TxIn. Add Keyed instances for hashes #2530
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@redxaxder Here is the reason why we need to remove compacted @TimSheard if you are ok with this PR feel free to "Squash and Merge| it, it is opened into your |
lehins
force-pushed
the
lehins/ts-compact-integration
branch
2 times, most recently
from
277388c to
1a1496b
Compare
lehins
force-pushed
the
ts-compact
branch
3 times, most recently
from
3ed31b6 to
02f85c1
Compare
lehins
force-pushed
the
lehins/ts-compact-integration
branch
from
1a1496b to
835ce93
Compare
|
After some experiments we found out that using HashMap that was implemented in #2520 is qyuite a bit more memory efficient then other approaches that involved manual unpacking of In order for us to be able and use the most efficient from the top of the list, this PR is needed. |
lehins
force-pushed
the
lehins/ts-compact-integration
branch
from
835ce93 to
d7dde93
Compare
lehins
force-pushed
the
lehins/ts-compact-integration
branch
from
d7dde93 to
fa20ff3
Compare
nc6
approved these changes
Nov 2, 2021
There was a problem hiding this comment.
Everything looks reasonable. I have two questions, but they're not really review comments:
- The comment about needing a
Keyedinstance doesn't really motivate the non-unpacking. - Why the change from
Word64toIntfor the index? Is this motivated by wanting to useIntMap?
| {-# UNPACK #-} !Word64 -> -- Index | ||
| TxIn crypto | ||
| TxInCompactOther :: !(TxId crypto) -> {-# UNPACK #-} !Word64 -> TxIn crypto | ||
| data TxIn crypto = TxInCompact !(TxId crypto) {-# UNPACK #-} !Int |
There was a problem hiding this comment.
Because we will use it with IntMap, which needs an Int, rather than a Word64. I don't think it would a problem, right? Considering that max value on mainnet is 249 and I don't see tx index going larger than maxBound :: Int any time in the foreseeable future 😄
nc6
added a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Nov 4, 2021
Recent ledger changes (IntersectMBO/cardano-ledger#2530) have restricted the range for transaction input indices. Since these are unlikely to be large anyway, restrict the generated range.
erikd
pushed a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Nov 4, 2021
Recent ledger changes (IntersectMBO/cardano-ledger#2530) have restricted the range for transaction input indices. Since these are unlikely to be large anyway, restrict the generated range.
erikd
pushed a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Nov 4, 2021
Recent ledger changes (IntersectMBO/cardano-ledger#2530) have restricted the range for transaction input indices. Since these are unlikely to be large anyway, restrict the generated range.
erikd
pushed a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Nov 5, 2021
Recent ledger changes (IntersectMBO/cardano-ledger#2530) have restricted the range for transaction input indices. Since these are unlikely to be large anyway, restrict the generated range.
erikd
pushed a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Nov 6, 2021
Recent ledger changes (IntersectMBO/cardano-ledger#2530) have restricted the range for transaction input indices. Since these are unlikely to be large anyway, restrict the generated range.
Jimbo4350
pushed a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Nov 8, 2021
Recent ledger changes (IntersectMBO/cardano-ledger#2530) have restricted the range for transaction input indices. Since these are unlikely to be large anyway, restrict the generated range.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR requires: IntersectMBO/cardano-base#243