Skip to content

feat: Print sourcemaps/files operations timings information #995

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.

Sign up for GitHub

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

Merged
kamilogorek merged 4 commits into from
Jul 7, 2021
Merged

feat: Print sourcemaps/files operations timings information #995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a5dec9
feat: Print sourcemaps/files operations timings information
kamilogorek Jul 6, 2021
929d2d4
Update to indicatiff 0.16
kamilogorek Jul 6, 2021
218d0c8
Use indicatiff built-in timers for elapsed logs
kamilogorek Jul 6, 2021
0c196bb
Fix correct timing calculations
kamilogorek Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ git2 = { version = "0.13.6", default-features = false }
glob = "0.3.0"
if_chain = "1.0.0"
ignore = "0.4.16"
indicatif = "0.14.0"
indicatif = "0.16.2"
itertools = "0.10.0"
java-properties = "1.2.0"
lazy_static = "1.4.0"
Expand Down Expand Up @@ -62,7 +62,7 @@ zip = "0.5.6"
assert_cmd = "1.0.1"
insta = { version = "1.7.1", features = ["redactions"] }
mockito = "0.30.0"
predicates = "1.0.4"
predicates = "2.0.0"
tempfile = "3.1.0"

[features]
Expand Down
2 changes: 1 addition & 1 deletion src/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,7 @@ impl Api {
let pb = make_progress_bar(sources.len() as u64);

for (url, path) in sources {
pb.set_message(&path.to_str().unwrap());
pb.set_message(path.to_str().unwrap().to_owned());

let upload_result = self.upload_release_file(
org,
Expand Down
4 changes: 2 additions & 2 deletions src/commands/difutil_find.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,10 @@ fn find_ids(
}

if let Some(p) = dirent.file_name().to_str() {
pb.set_message(&p);
pb.set_message(p.to_owned());
}
pb.tick();
pb.set_prefix(&format!("{}", found_files.len()));
pb.set_prefix(found_files.len().to_string());

let mut found = vec![];

Expand Down
2 changes: 1 addition & 1 deletion src/commands/upload_dif.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ fn execute_internal(matches: &ArgMatches<'_>, legacy: bool) -> Result<(), Error>
let progress = ProgressBar::new_spinner();
progress.enable_steady_tick(100);
progress.set_style(progress_style);
progress.set_message(&info_plist.to_string());
progress.set_message(info_plist.to_string());

let checksums = uploaded.iter().map(|dif| dif.checksum.clone()).collect();
let response = api.associate_apple_dsyms(&org, &project, info_plist, checksums)?;
Expand Down
3 changes: 2 additions & 1 deletion src/utils/chunks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ pub fn upload_chunks(
.collect::<Result<(), _>>()
})?;

progress.finish_and_clear();
progress.finish_with_duration("Uploading");

Ok(())
}
10 changes: 5 additions & 5 deletions src/utils/dif_upload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,7 @@ fn search_difs(options: &DifUpload) -> Result<Vec<DifMatch<'static>>, Error> {
}
walk_difs_directory(base_path, options, |source, name, buffer| {
debug!("trying to process {}", name);
progress.set_message(&name);
progress.set_message(name.to_owned());

@AbhiPrasad AbhiPrasad Jul 6, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rust Q, do we always prefer to_owned over clone because it's more generic? (to_owned might not clone right? - depends on the trait impl)

@kamilogorek kamilogorek Jul 7, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clone() on &str returns a &str, where in this case to_owned will return String which we need for set_message. I prefer to_owned as it's more explicit and obvious what are you want to get out of that call.


if Archive::peek(&buffer) != FileFormat::Unknown {
let mut difs =
Expand All @@ -671,7 +671,7 @@ fn search_difs(options: &DifUpload) -> Result<Vec<DifMatch<'static>>, Error> {
}
};

progress.set_prefix(&collected.len().to_string());
progress.set_prefix(collected.len().to_string());
Ok(())
})?;
}
Expand Down Expand Up @@ -941,7 +941,7 @@ where
let mut calculated = Vec::new();
for item in items {
progress.inc(1);
progress.set_message(item.path());
progress.set_message(item.path().to_owned());
calculated.push(func(item)?);
}

Expand Down Expand Up @@ -1001,7 +1001,7 @@ fn process_symbol_maps<'a>(

for dif in with_hidden {
progress.inc(1);
progress.set_message(dif.path());
progress.set_message(dif.path().to_owned());
without_hidden.push(resolve_hidden_symbols(dif, symbol_map)?);
}

Expand Down Expand Up @@ -1052,7 +1052,7 @@ fn create_source_bundles<'a>(difs: &[DifMatch<'a>]) -> Result<Vec<DifMatch<'a>>,

for dif in difs {
progress.inc(1);
progress.set_message(dif.path());
progress.set_message(dif.path().to_owned());

let object = match dif.object() {
Some(object) => object,
Expand Down
4 changes: 2 additions & 2 deletions src/utils/file_search.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ impl ReleaseFileSearch {
if file.file_type().map_or(false, |t| t.is_dir()) {
continue;
}
progress.set_message(&format!("{}", file.path().display()));
progress.set_message(format!("{}", file.path().display()));

info!(
"found: {} ({} bytes)",
Expand All @@ -158,7 +158,7 @@ impl ReleaseFileSearch {
};
collected.push(file_match);

progress.set_prefix(&collected.len().to_string());
progress.set_prefix(collected.len().to_string());
}

progress.finish_and_clear();
Expand Down
9 changes: 5 additions & 4 deletions src/utils/file_upload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ fn upload_files_chunked(
.map(|(data, checksum)| Chunk((*checksum, data)))
.collect::<Vec<_>>();

progress.finish_and_clear();
progress.finish_with_duration("Optimizing");

let progress_style = ProgressStyle::default_bar().template(&format!(
"{} Uploading release files...\
Expand Down Expand Up @@ -272,7 +272,7 @@ fn upload_files_chunked(
bail!("Failed to process uploaded files: {}", message);
}

progress.finish_and_clear();
progress.finish_with_duration("Processing");

if response.state.is_pending() {
if context.wait {
Expand Down Expand Up @@ -314,7 +314,7 @@ fn build_artifact_bundle(context: &UploadContext, files: &ReleaseFiles) -> Resul

for file in files.values() {
progress.inc(1);
progress.set_message(&file.url);
progress.set_message(file.url.to_owned());

let mut info = SourceFileInfo::new();
info.set_ty(file.ty);
Expand All @@ -329,7 +329,6 @@ fn build_artifact_bundle(context: &UploadContext, files: &ReleaseFiles) -> Resul

bundle.finish()?;

progress.finish_and_clear();
println!(
"{} Bundled {} {} for upload",
style(">").dim(),
Expand All @@ -340,6 +339,8 @@ fn build_artifact_bundle(context: &UploadContext, files: &ReleaseFiles) -> Resul
}
);

progress.finish_with_duration("Bundling");

Ok(archive)
}

Expand Down
13 changes: 12 additions & 1 deletion src/utils/progress.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,17 @@ impl ProgressBar {
}

pub fn finish_with_message(&self, msg: &str) {
self.inner.finish_with_message(msg.to_owned());
logging::set_progress_bar(None);
}

pub fn finish_with_duration(&self, op: &str) {
Comment thread
AbhiPrasad marked this conversation as resolved.
Resolved
let dur = self.inner.elapsed();
// We could use `dur.as_secs_f64()`, but its unnecessarily precise (micros). Millis are enough for our purpose.
let msg = format!("{} completed in {}s", op, dur.as_millis() as f64 / 1000.0);
let progress_style = ProgressStyle::default_bar().template("{prefix:.dim} {msg}");
self.inner.set_style(progress_style);
self.inner.set_prefix(">");
self.inner.finish_with_message(msg);
logging::set_progress_bar(None);

@AbhiPrasad AbhiPrasad Jul 6, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use self.finish_with_message(&msg) to avoid duplicating the logic here?

@kamilogorek kamilogorek Jul 7, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about it but decided no to do that to keep everything in line. This way, every method is a standalone thing that just proxies data to indicatif methods. It's very rarely touched part of code and adds 1 more line of code anyway.

}
Expand Down Expand Up @@ -76,7 +87,7 @@ impl Deref for ProgressBar {

pub fn make_progress_bar(len: u64) -> ProgressBar {
let pb = ProgressBar::new(len);
pb.set_draw_target(ProgressDrawTarget::to_term(Term::stdout(), None));
pb.set_draw_target(ProgressDrawTarget::term(Term::stdout(), None));
pb.set_style(ProgressStyle::default_bar().template(&format!(
"{} {{msg}}\n{{wide_bar}} {{pos}}/{{len}}",
style(">").cyan()
Expand Down
10 changes: 5 additions & 5 deletions src/utils/sourcemaps.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ impl SourceMapProcessor {
style(self.pending_sources.len()).yellow()
);
for (url, mut file) in self.pending_sources.drain() {
pb.set_message(&url);
pb.set_message(url.to_owned());
let ty = if sourcemap::is_sourcemap_slice(&file.contents) {
SourceFileType::SourceMap
} else if file
Expand Down Expand Up @@ -246,7 +246,7 @@ impl SourceMapProcessor {
pb.inc(1);
}

pb.finish_and_clear();
pb.finish_with_duration("Analyzing");
}

pub fn dump_log(&self, title: &str) {
Expand Down Expand Up @@ -305,7 +305,7 @@ impl SourceMapProcessor {
println!("{} Validating sources", style(">").dim());
let pb = make_progress_bar(sources.len() as u64);
for source in sources {
pb.set_message(&source.url);
pb.set_message(source.url.clone());
match source.ty {
SourceFileType::Source | SourceFileType::MinifiedSource => {
if let Err(err) = validate_script(source) {
Expand Down Expand Up @@ -462,7 +462,7 @@ impl SourceMapProcessor {

let pb = make_progress_bar(self.sources.len() as u64);
for source in self.sources.values_mut() {
pb.set_message(&source.url);
pb.set_message(source.url.clone());
if source.ty != SourceFileType::SourceMap {
pb.inc(1);
continue;
Expand All @@ -487,7 +487,7 @@ impl SourceMapProcessor {
source.contents = new_source;
pb.inc(1);
}
pb.finish_and_clear();
pb.finish_with_duration("Rewriting");
Ok(())
}

Expand Down