Skip to content

thread_ptrace: use unbuffered IO by default#1290

Merged
sporksmith merged 2 commits intoshadow:devfrom
sporksmith:ptrace-unbuffered
Apr 14, 2021
Merged

thread_ptrace: use unbuffered IO by default#1290
sporksmith merged 2 commits intoshadow:devfrom
sporksmith:ptrace-unbuffered

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

Buffering only helps when doing small sequential accesses. For syscalls that do large accesses (read, write), buffering just adds an extra copy.

Smaller accesses are generally small structs; not many handlers access more than one, and even then it only helps if they happen to be sequential in memory.

@sporksmith sporksmith requested a review from robgjansen April 14, 2021 19:17
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 14, 2021

Codecov Report

Merging #1290 (60d6e9b) into dev (e81ae25) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1290      +/-   ##
==========================================
- Coverage   55.67%   55.65%   -0.02%     
==========================================
  Files         141      141              
  Lines       20616    20619       +3     
  Branches     5067     5068       +1     
==========================================
- Hits        11478    11476       -2     
- Misses       6017     6020       +3     
- Partials     3121     3123       +2     
Flag Coverage Δ
tests 55.65% <66.66%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/host/thread_ptrace.c 51.85% <66.66%> (+0.06%) ⬆️
src/main/core/worker.c 77.23% <0.00%> (-0.97%) ⬇️

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 e81ae25...60d6e9b. Read the comment docs.

"otherwise result in excessive detaching and reattaching",
NULL)

static bool _useBufferedIo = false;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Steve just renamed a bunch of these _useXXX switch variables, so maybe we should follow his pattern?

https://github.com/shadow/shadow/pull/1287/files#diff-458bdb269062900e2d0ac153268cbd70b89d5c811f1fd9445d2e49f21c4b5033R55

Although I think he was just aiming to make it so that the default for all of the options is false. Anyway, I guess _enableBufferedIo would be more consistent with his change?

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.

Good point; done

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.

We decided to remove the --disable and --enable prefixes from options, so this PR isn't needed anymore.

I guess I should still use the --enable prefix here, and they'll all be changed in another PR?

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.

I closed that PR since I thought we wanted to not use the --enable or --disable prefixes anymore.

@robgjansen robgjansen added Component: Main Composing the core Shadow executable Type: Enhancement New functionality or improved design labels Apr 14, 2021
@sporksmith sporksmith merged commit 0ee3581 into shadow:dev Apr 14, 2021
@sporksmith sporksmith deleted the ptrace-unbuffered branch April 14, 2021 21:13
@robgjansen
Copy link
Copy Markdown
Member

For posterity: I ran a Tor 5% net with memory manager disabled using Jim’s byte queue commit aed8b71.

run_time.pdf

It looks like unbuffered IO indeed is the winner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Main Composing the core Shadow executable Type: Enhancement New functionality or improved design

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants