Add append-vec subcommand & another env knob #9490

ryoqun · 2020-04-14T15:56:21Z

Problem

There is no handy way to display the content of AppendVec and there is no easy way to view historical old state of written accounts by solana-validator. Both are handy when debugging.

Summary of Changes

Add such a subcommand to ledger-tool and such a environment variable, respectively.

Needed this while investigation toward to #9423 / #9357

ryoqun · 2020-04-14T16:04:10Z

runtime/src/append_vec.rs

@@ -117,6 +117,10 @@ pub struct AppendVec {

 impl Drop for AppendVec {
    fn drop(&mut self) {
+        if env::var("SOLANA_DISABLE_DEAD_APPEND_VEC_REMOVAL").is_ok() {


Well, granted I'm fond of envs. ;)

@sakridge I guess this is neligible for the performance-wise.

I would prefer a member variable here.

Something like new_empty_map(len: usize, cleanup: bool) {} and fn drop(self) { if self.cleanup { remove_file(x); }

@sakridge @mvines

Well, that knob is needed for solana-validator not for solana-ledger-tool. So, I have to resort to an env var, otherwise, some considerable plumbing is needed or lazy_static!...

Btw, solana-ledger-tool append-vec wasn't oddly removing the specified AppendVecs. Then, I found a footgun: the exit(0) immediately after it caused the process to exit without calling Drop::drop()s altogether... So, it was working as intended by mistake.

A bit scared of the misstep, I've refactored and fixed that with tests, resulting rather bigger PR... :)

Also, I've chosen read_only (false, by default) instead of cleanup (true, by default) because I think the naming more directly conveys the behavior and I generally prefer that default behavior equates to Default::default() value for compatibility, less cognitive load, and readability.

codecov · 2020-04-14T17:20:17Z

Codecov Report

Merging #9490 into master will decrease coverage by 0.1%.
The diff coverage is 53.6%.

@@           Coverage Diff            @@
##           master   #9490     +/-   ##
========================================
- Coverage    80.4%   80.2%   -0.2%     
========================================
  Files         284     284             
  Lines       66167   66272    +105     
========================================
- Hits        53220   53211      -9     
- Misses      12947   13061    +114

ryoqun · 2020-04-17T06:51:40Z

ledger-tool/src/main.rs

+    println!("  - balance: {} SOL", lamports_to_sol(account.lamports));
+    println!("  - owner: '{}'", account.owner);
+    println!("  - executable: {}", account.executable);
+    println!("  - rent_epoch: {}", account.rent_epoch);


@mvines I also started to print rent_epoch as well, remotely related my recent other work #9527

ryoqun · 2020-04-17T06:55:58Z

ledger-tool/src/main.rs

+    println!("  - owner: '{}'", account.owner);
+    println!("  - executable: {}", account.executable);
+    println!("  - rent_epoch: {}", account.rent_epoch);
+    println!("  - data: '{}'", base64::encode(&account.data));


@mvines Also, I've started to encoding from base58 to base64 because I've experienced so slow encoding for large data (maybe quadric time; O(n^2))..

It seems that @nearprotocol people are also experiencing this: https://github.com/nearprotocol/nearcore/blob/master/Cargo.toml#L84 (I found this just by coincidence).

How much slower is bs58 in this case? Also do the opt-level = 3 do anything? It's going to get really confusing for the user if we mix base58 and base64, especially if there's no prefix.

How about we use the same output as solana account? That output could be factored out of solana account and put in a common function, like

solana/cli/src/display.rs

Line 67 in 9bba27a

pub fn println_transaction(

@mvines Ok, I'll do the solana account path. :) Hopefull, I'll also add base64 to the shiny --output as well. :)

Also do the opt-level = 3 do anything?

sadly, not much... ;)

How much slower is bs58 in this case?

For the record: well, it seems that it's well-known that base58 is slow algorithmically by design. And I believe this is correct to my understanding. Its computation cost is quadratic = O(n^2).

Pt 161616592 base64 encoding aeternity/aeternity#1733: The base58 encoding/decoding is hopelessly slow for large byte arrays.

https://news.ycombinator.com/item?id=18408008: but is still quadratically expensive and should be used for a reasonably short, known-length data like hashes.

https://github.com/tuupola/base58: That said, Base58 is well suited for decoding big integers but is not designed to decode long portions of binary data.

My quick observation also aligns with these quick findings:

$ cat runtime/src/bank.rs | env time -v base58 > /dev/null Command being timed: "base58" User time (seconds): 142.34 System time (seconds): 1.39 Percent of CPU this job got: 100% Elapsed (wall clock) time (h:mm:ss or m:ss): 2:23.73 Average shared text size (kbytes): 0 Average unshared data size (kbytes): 0 Average stack size (kbytes): 0 Average total size (kbytes): 0 Maximum resident set size (kbytes): 20044 Average resident set size (kbytes): 0 Major (requiring I/O) page faults: 0 Minor (reclaiming a frame) page faults: 1532691 Voluntary context switches: 3 Involuntary context switches: 162 Swaps: 0 File system inputs: 0 File system outputs: 0 Socket messages sent: 0 Socket messages received: 0 Signals delivered: 0 Page size (bytes): 4096 Exit status: 0 ==> 2020-04-28 16:43:20.819247997|0 $ cat runtime/src/bank.rs runtime/src/bank.rs | env time -v base58 > /dev/null Command being timed: "base58" User time (seconds): 485.85 System time (seconds): 6.86 Percent of CPU this job got: 100% Elapsed (wall clock) time (h:mm:ss or m:ss): 8:12.70 Average shared text size (kbytes): 0 Average unshared data size (kbytes): 0 Average stack size (kbytes): 0 Average total size (kbytes): 0 Maximum resident set size (kbytes): 21920 Average resident set size (kbytes): 0 Major (requiring I/O) page faults: 0 Minor (reclaiming a frame) page faults: 6581747 Voluntary context switches: 5 Involuntary context switches: 317 Swaps: 0 File system inputs: 0 File system outputs: 0 Socket messages sent: 0 Socket messages received: 0 Signals delivered: 0 Page size (bytes): 4096 Exit status: 0

ryoqun · 2020-04-17T07:08:36Z

runtime/src/append_vec.rs

        let _ignored = remove_file(&self.path);
    }
 }

 impl AppendVec {
    #[allow(clippy::mutex_atomic)]
-    pub fn new(file: &Path, create: bool, size: usize) -> Self {


this flag name always bothered me..

ryoqun · 2020-04-17T07:10:59Z

runtime/src/append_vec.rs

+            })
+            .unwrap();
+
+        data.seek(SeekFrom::Start((size - 1) as u64)).unwrap();


@sakridge I'm aware this should be fs::truncate or linux's fallocate but this is separate issue... :p

ryoqun · 2020-04-17T07:11:59Z

runtime/src/append_vec.rs

 }

 impl Drop for AppendVec {
    fn drop(&mut self) {
+        // the env knob is needed to make solana-validator retain retired AppendVecs for
+        // later inspection of the files


@sakridge Does this comment now make sense to justify my laziness of another env var? ;)

I'd vote for the following over an env var:

Considerable plumbing

A lazy_static like

solana/runtime/src/accounts_db.rs

Lines 61 to 65 in 40737e9

lazy_static! {

// FROZEN_ACCOUNT_PANIC is used to signal local_cluster that an AccountsDB panic has occurred,

// as |cargo test| cannot observe panics in other threads

pub static ref FROZEN_ACCOUNT_PANIC: Arc<AtomicBool> = { Arc::new(AtomicBool::new(false)) };

}

When I added (2), I declined (1) myself :)

hehe, thanks for the relevant info! I'll first try the (1). :)

stale · 2020-04-24T19:54:36Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale · 2020-05-07T09:15:59Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

mvines · 2020-05-11T15:03:46Z

cli/src/cli.rs

+                    Arg::with_name("account_data_output")
+                        .long("account-data-output")


What if we just called this format:

Suggested change

Arg::with_name("account_data_output")

.long("account-data-output")

Arg::with_name("format")

A .default_value("..") would be nice here too

@mvines Well, there already exists --output, so I wanted to avoid too similarly named argument..

mvines · 2020-05-11T15:05:41Z

cli/src/display.rs

+) -> String {
+    let rpc_account = match account_data_output {
+        AccountDataOutputFormat::Base58 => RpcAccount::encode_with_base58(account.clone()),
+        AccountDataOutputFormat::Hex => RpcAccount::encode_with_base58(account.clone()),


encode_with_base58 for AccountDataOutputFormat::Hex?

Nice catch! the last pushed commit is still being baked... ;)

mvines · 2020-05-11T15:05:50Z

cli/src/display.rs

+    for (pubkey, account) in accounts {
+        let rpc_account = match account_data_output {
+            AccountDataOutputFormat::Base58 => RpcAccount::encode_with_base58(account.clone()),
+            AccountDataOutputFormat::Hex => RpcAccount::encode_with_base58(account.clone()),


encode_with_base58 for AccountDataOutputFormat::Hex? (same)

mvines · 2020-05-11T15:07:36Z

ledger-tool/src/main.rs

+                Arg::with_name("account_data_output")
+                    .long("account-data-output")


same as above, "format" plus a .default_value()?

Suggested change

Arg::with_name("account_data_output")

.long("account-data-output")

Arg::with_name("format")

mvines · 2020-05-11T15:08:50Z

ledger-tool/src/main.rs

+                        .map(|value| match value {
+                            "base58" => AccountDataOutputFormat::Base58,
+                            "base64" => AccountDataOutputFormat::Base64,
+                            _ => unreachable!(),


This match is duplicated multiple times, time to promote it to clap-utils/

stale · 2020-05-20T06:23:16Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale · 2020-05-27T06:57:11Z

This stale pull request has been automatically closed. Thank you for your contributions.

ryoqun · 2021-05-07T15:05:14Z

for the record, I build this pr to short-cut investigation for #17083

ryoqun requested review from mvines and sakridge April 14, 2020 15:57

ryoqun commented Apr 14, 2020

View reviewed changes

ryoqun commented Apr 17, 2020

View reviewed changes

ryoqun added v1.0 labels Apr 17, 2020

ryoqun force-pushed the ledger-tool-append-vec branch from 8162931 to 7954f36 Compare April 17, 2020 07:40

stale bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Apr 24, 2020

stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label May 7, 2020

ryoqun added 6 commits May 11, 2020 09:59

Add append-vec subcommand & another env knob

c92729e

Use base64 (faster with big data) & rent_epoch

611ed29

Refactor by extracting print code into methods

5022b76

Suppress unintended AppendVec deletion explicitly

a3cd838

Tweak tests as a bonus after required rebase...

e152b74

Share account output between cli and ledger-tool

e0e2020

ryoqun force-pushed the ledger-tool-append-vec branch from 7954f36 to e0e2020 Compare May 11, 2020 07:19

stale bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label May 11, 2020

ryoqun added 2 commits May 11, 2020 16:22

Fix fixes

7b46f80

Fix...

d2f4a48

mvines reviewed May 11, 2020

View reviewed changes

stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label May 20, 2020

This was referenced May 20, 2020

RPC uses base58 for large binary leading to slow response #10140

Closed

Support opening an in-use rocksdb as secondary #10209

Merged

stale bot closed this May 27, 2020

ryoqun mentioned this pull request Jun 30, 2020

Add account-decoder utilities #10846

Merged

ryoqun mentioned this pull request Aug 26, 2020

Make ledger-tool accounts print rent_epoch and slot #11845

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add append-vec subcommand & another env knob #9490

Add append-vec subcommand & another env knob #9490

ryoqun commented Apr 14, 2020 •

edited

Loading

ryoqun Apr 14, 2020

sakridge Apr 14, 2020

sakridge Apr 14, 2020

ryoqun Apr 17, 2020 •

edited

Loading

codecov bot commented Apr 14, 2020 •

edited

Loading

ryoqun Apr 17, 2020 •

edited

Loading

ryoqun Apr 17, 2020 •

edited

Loading

mvines Apr 17, 2020

ryoqun Apr 28, 2020 •

edited

Loading

ryoqun Apr 17, 2020

ryoqun Apr 17, 2020

ryoqun Apr 17, 2020

mvines Apr 17, 2020

ryoqun Apr 28, 2020

stale bot commented Apr 24, 2020

stale bot commented May 7, 2020

mvines May 11, 2020

mvines May 11, 2020

ryoqun May 13, 2020

mvines May 11, 2020

ryoqun May 13, 2020

mvines May 11, 2020

mvines May 11, 2020

mvines May 11, 2020

stale bot commented May 20, 2020

stale bot commented May 27, 2020

ryoqun commented May 7, 2021

	lazy_static! {
	// FROZEN_ACCOUNT_PANIC is used to signal local_cluster that an AccountsDB panic has occurred,
	// as \|cargo test\| cannot observe panics in other threads
	pub static ref FROZEN_ACCOUNT_PANIC: Arc<AtomicBool> = { Arc::new(AtomicBool::new(false)) };
	}

		Arg::with_name("account_data_output")
		.long("account-data-output")

	Arg::with_name("account_data_output")
	.long("account-data-output")
	Arg::with_name("format")

Add append-vec subcommand & another env knob #9490

Add append-vec subcommand & another env knob #9490

Conversation

ryoqun commented Apr 14, 2020 • edited Loading

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Apr 14, 2020 • edited Loading

Codecov Report

ryoqun Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

ryoqun Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Apr 24, 2020

stale bot commented May 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented May 20, 2020

stale bot commented May 27, 2020

ryoqun commented May 7, 2021

ryoqun commented Apr 14, 2020 •

edited

Loading

ryoqun Apr 17, 2020 •

edited

Loading

codecov bot commented Apr 14, 2020 •

edited

Loading

ryoqun Apr 17, 2020 •

edited

Loading

ryoqun Apr 17, 2020 •

edited

Loading

ryoqun Apr 28, 2020 •

edited

Loading