Skip to content
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

Remove OrderedTransactions #511

Merged

Conversation

@moreal
Copy link
Contributor

commented Sep 11, 2019

This resolves #244.

  • Block<T>.Transactions became to present ordered transactions
  • Removed Block<T>.OrderedTransactions
@moreal moreal self-assigned this Sep 11, 2019
@moreal moreal added this to the 0.6.0 milestone Sep 11, 2019
@moreal moreal force-pushed the moreal:reduce-usage-of-memory-in-block-transactions branch from ba6e062 to 0457836 Sep 11, 2019
@moreal moreal changed the title [WIP] Remove OrderedTransactions Remove OrderedTransactions Sep 11, 2019
@moreal moreal marked this pull request as ready for review Sep 11, 2019
@moreal moreal requested review from libplanet, planetarium/libplanet, dahlia, earlbread, limebell and longfin and removed request for libplanet and planetarium/libplanet Sep 11, 2019
Libplanet/Blocks/Block.cs Show resolved Hide resolved
Libplanet/Tx/TxId.cs Show resolved Hide resolved
Libplanet.Tests/Blockchain/BlockChainTest.cs Outdated Show resolved Hide resolved
Libplanet/Tx/TxId.cs Show resolved Hide resolved
@@ -548,7 +548,8 @@ bool renderActions
HashDigest<SHA256>? tip = Store.IndexBlockHash(Id, -1);

var nonceDeltas = new Dictionary<Address, long>();
foreach (Transaction<T> tx1 in block.Transactions)
var orderedTransactions = block.Transactions.OrderBy(tx => tx.Nonce);

This comment has been minimized.

Copy link
@longfin

longfin Sep 11, 2019

Member

There is a bit of concern about preload performance. can we check if sorting on each addition affects it?

This comment has been minimized.

Copy link
@moreal

moreal Sep 11, 2019

Author Contributor

Yes, this changes can cause to affect it.
But, IMHO I think the changes are a trade with memory.

@moreal moreal force-pushed the moreal:reduce-usage-of-memory-in-block-transactions branch 2 times, most recently from 9f7e440 to 1f5a1c4 Sep 11, 2019
@@ -110,16 +107,15 @@ private Block(RawBlock rb)
DateTimeOffset timestamp,
IEnumerable<Transaction<T>> transactions)
{
Transaction<T>[] orderedTxs = transactions.OrderBy(tx => tx.Nonce).ToArray();
var txs = transactions.OrderBy(tx => tx.Id).ToImmutableArray();

This comment has been minimized.

Copy link
@earlbread

earlbread Sep 16, 2019

Member

We sort transactions in line43 again. Is this line required?

This comment has been minimized.

Copy link
@moreal

moreal Sep 16, 2019

Author Contributor

Yes, it's because the process to order transactions determines Nonce. Isn't it?

@moreal moreal requested a review from limebell Sep 17, 2019
@moreal moreal dismissed stale reviews from earlbread, dahlia, and longfin via 3a543e0 Sep 18, 2019
@moreal moreal force-pushed the moreal:reduce-usage-of-memory-in-block-transactions branch from 1f5a1c4 to 3a543e0 Sep 18, 2019
@dahlia dahlia requested review from earlbread and longfin Sep 18, 2019
@dahlia
dahlia approved these changes Sep 18, 2019
@dahlia dahlia merged commit 3b23270 into planetarium:master Sep 18, 2019
13 of 15 checks passed
13 of 15 checks passed
planetarium.libplanet Build #20190918.6 failed
Details
planetarium.libplanet (Windows_NETCore_coverage) Windows_NETCore_coverage failed
Details
WIP Ready for review
Details
changelog This contains self-describing changelog.
Details
docs Libplanet docs generated by DocFX
Details
license/cla Contributor License Agreement is signed.
Details
planetarium.libplanet (Linux_Mono) Linux_Mono succeeded
Details
planetarium.libplanet (Linux_NETCore) Linux_NETCore succeeded
Details
planetarium.libplanet (Windows_Mono) Windows_Mono succeeded
Details
planetarium.libplanet (Windows_NETCore) Windows_NETCore succeeded
Details
planetarium.libplanet (Windows_NETCore_Benchmark) Windows_NETCore_Benchmark succeeded
Details
planetarium.libplanet (Windows_NETFramework) Windows_NETFramework succeeded
Details
planetarium.libplanet (macOS_Mono) macOS_Mono succeeded
Details
planetarium.libplanet (macOS_NETCore) macOS_NETCore succeeded
Details
planetarium.libplanet (macOS_Unity) macOS_Unity succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.