Skip to content

Use shared_ptr in irept#685

Closed
reuk wants to merge 2 commits intodiffblue:masterfrom
reuk:irep-shared-ptr
Closed

Use shared_ptr in irept#685
reuk wants to merge 2 commits intodiffblue:masterfrom
reuk:irep-shared-ptr

Conversation

@reuk
Copy link
Contributor

@reuk reuk commented Mar 23, 2017

irept was doing manual reference-counting. Applying the single responsibility principle, we replace manual pointer/refcount manipulation with std::shared_ptr, so that the inner dt class does not have the dual responsibilities of maintaining a reference count and storing the object data members. Using shared_ptr improves the maintainability of the code, as it is a well-known and well-documented abstraction. shared_ptr also allows us to use the compiler-generated special member functions, so that there is less overall code to maintain.

@mgudemann
Copy link
Contributor

mgudemann commented Mar 23, 2017

@reuk would you mind checking whether this solves #569 ?

@reuk reuk force-pushed the irep-shared-ptr branch from 604c6fc to bbeea25 Compare March 23, 2017 12:50
@reuk
Copy link
Contributor Author

reuk commented Mar 23, 2017

Unfortunately, I get very similar results, so I don't think it solves #569.

@tautschnig
Copy link
Collaborator

tautschnig commented Mar 23, 2017

I'll take a look at this one, but it would have been very nice if the diff had been minimal rather than including a sleuth of other changes.

@kroening
Copy link
Collaborator

kroening commented Mar 23, 2017

I would be curious to learn whether there's any performance/memory usage difference.

@tautschnig
Copy link
Collaborator

tautschnig commented Mar 23, 2017

Using some simple irept tests I had built some time ago the only observable difference in memory consumption is the size of an irept, which now is the size of two pointers instead of just one:

shared_ptr:
$ /usr/bin/time -l ./a.out
  virtual size: 2669.56m
  max_size_in_use: 162.829m
  size_allocated: 184.549m
        0.67 real         0.61 user         0.04 sys
 163639296  maximum resident set size
         0  average shared memory size
         0  average unshared data size
         0  average unshared stack size
     39963  page reclaims
         0  page faults
         0  swaps
         0  block input operations
         0  block output operations
         0  messages sent
         0  messages received
         0  signals received
         0  voluntary context switches
        28  involuntary context switches
$ g++ -std=c++11 irep_size.cpp -I util/ -Dprivate=public
$ ./a.out
irept=16
irept::dt=80
unsigned=4
dstring=4
named_subt=24
subt=24

current code:
$ /usr/bin/time -l ./a.out
  virtual size: 2660.14m
  max_size_in_use: 162.791m
  size_allocated: 183.501m
        0.58 real         0.52 user         0.04 sys
 163606528  maximum resident set size
         0  average shared memory size
         0  average unshared data size
         0  average unshared stack size
     39902  page reclaims
        53  page faults
         0  swaps
         0  block input operations
         0  block output operations
         0  messages sent
         0  messages received
         0  signals received
         1  voluntary context switches
        15  involuntary context switches
$ g++ -std=c++11 irep_size.cpp -I util/ -Dprivate=public
$ ./a.out
irept=8
irept::dt=80
unsigned=4
dstring=4
named_subt=24
subt=24

@martin-cs
Copy link
Collaborator

martin-cs commented Mar 23, 2017 via email

@tautschnig
Copy link
Collaborator

tautschnig commented Mar 23, 2017

Well, as far as I understand we are just moving those extra bytes out of the inner structure, which would always get allocated together with an irept. (I am actually a bit surprised the size of irept::dt has not gone down.) In fact performance may be better due to cache locality.

@tautschnig
Copy link
Collaborator

tautschnig commented Mar 23, 2017

To clarify my surprise: the gap is now just replaced by padding, so we do end up with some overhead. Proper measurements at large scale are due, though.

@reuk reuk mentioned this pull request Apr 9, 2017
Closed
@reuk reuk closed this Apr 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

cleanup do not merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@reuk @mgudemann @tautschnig @kroening @martin-cs