Skip to content

Optimize debug logs#1582

Merged
fd0 merged 2 commits intomasterfrom
optimize-debug-log
Jan 26, 2018
Merged

Optimize debug logs#1582
fd0 merged 2 commits intomasterfrom
optimize-debug-log

Conversation

@fd0
Copy link
Copy Markdown
Member

@fd0 fd0 commented Jan 25, 2018

The code in here optimizes debug.Log() calls when a restic ID is passed in. Before, the "shorten the string" method Str() was always called, which led to a function call and allocations happening even without the debug tag. Now, we're calling the Str() method via reflection on demand in the debug code, which is only active with the debug tag.

Idea by @MJDSys in #1549

@MJDSys
Copy link
Copy Markdown
Contributor

MJDSys commented Jan 26, 2018

This seems like a better implementation then what I had, so +1 from me!

I had two thoughts while I was taking a look over it:

  • would it make sense to keep a benchmark showing the cost of ID.Str versus just passing Id? Just to document this for the future?
  • Also, should the BenchmarkLogIDString function either keep its call to ID.String, or be removed? I don't think it is very useful being a duplicate of ID.Str's benchmark.

Otherwise I ran the micro benchmarks locally here, and as expected they run much faster. Everything else looks good, and a quick grep through the code suggests no tricky logging statements were missed.

@fd0
Copy link
Copy Markdown
Member Author

fd0 commented Jan 26, 2018

Yeah, we can improve the benchmarks. I'll have a look later.

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #1582 into master will decrease coverage by 0.14%.
The diff coverage is 70.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1582      +/-   ##
==========================================
- Coverage   51.76%   51.62%   -0.15%     
==========================================
  Files         142      142              
  Lines       11283    11283              
==========================================
- Hits         5841     5825      -16     
- Misses       4525     4546      +21     
+ Partials      917      912       -5
Impacted Files Coverage Δ
internal/fuse/dir.go 0% <0%> (ø) ⬆️
internal/repository/master_index.go 44.61% <0%> (ø) ⬆️
cmd/restic/cmd_tag.go 65.15% <100%> (ø) ⬆️
internal/restic/lock.go 69.84% <100%> (ø) ⬆️
internal/repository/packer_manager.go 70.83% <100%> (ø) ⬆️
internal/repository/index.go 67.48% <100%> (ø) ⬆️
internal/repository/repack.go 46.26% <100%> (ø) ⬆️
internal/walk/walk.go 75.82% <100%> (ø) ⬆️
cmd/restic/cmd_prune.go 63.15% <100%> (ø) ⬆️
cmd/restic/cmd_find.go 65.06% <50%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6212ee...663c57a. Read the comment docs.

@fd0 fd0 merged commit 663c57a into master Jan 26, 2018
fd0 added a commit that referenced this pull request Jan 26, 2018
@fd0 fd0 deleted the optimize-debug-log branch January 26, 2018 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants