Document the sharing map, and move sharing map unit tests to catch #1899

danpoe · 2018-03-01T17:24:15Z

No description provided.

tautschnig · 2018-03-02T08:42:34Z

Build failures are genuine:

../src/util/sharing_map.h:486:15: error: unused variable 'c' [-Werror,-Wunused-variable]

It seems that necessitates some changes to sharing_mapt. While at that: would it be possible to please add some documentation that in particular covers 1) complexity of operations and 2) reference counting/garbage collection semantics?

danpoe · 2018-03-02T10:29:50Z

@tautschnig Ok, I'll add some documentation.

hannes-steffenhagen-diffblue · 2018-03-06T14:56:04Z

unit/sharing_map.cpp is currently excluded from the cmake build (unit/CMakeLists.txt, line 11).

Also, the test for sharing_map and sharing_node should probably be under the unit/util directory.

tautschnig · 2018-03-25T19:11:32Z

unit/Makefile

@@ -28,6 +28,7 @@ SRC += unit_tests.cpp \
       java_bytecode/inherited_static_fields/inherited_static_fields.cpp \
       pointer-analysis/custom_value_set_analysis.cpp \
       sharing_node.cpp \
+       sharing_map.cpp \


As @hannes-steffenhagen-diffblue said: these should likely go in the util folder.

tautschnig · 2018-03-25T19:16:11Z

src/util/sharing_map.h

@@ -450,7 +450,7 @@ SHARING_MAPT2(, size_type)::erase(
    return 0;

  node_type *del=nullptr;
-  unsigned del_bit;
+  unsigned del_bit=0;


I'm pretty sure unsigned should not be used here at all and any such use should be a std::size_t instead.

tautschnig · 2018-03-25T19:18:57Z

src/util/sharing_map.h

+///
+/// When inserting a key-value pair into the map, first the hash of its key is
+/// computed. The `bits` number of lower order bits of the hash are deemed
+/// significant, and are grouped into `bits` / `chunk` chunks). The hash is then


The ) does not have a matching opening parenthesis.

tautschnig · 2018-03-25T19:19:49Z

src/util/sharing_map.h

+/// purposes of determining the position of the key-value pair in the trie. The
+/// actual key-value pairs are stored in the leaf nodes. Collisions (i.e., two
+/// different keys yield the same "string"), are handled by chaining the
+/// corresponding key-value pairs in a `std::list`.


Would a std::forward_list be sufficient?

Yes, this is fixed in an upcoming PR.

tautschnig · 2018-03-25T19:32:17Z

src/util/sharing_map.h

@@ -39,6 +39,66 @@ Author: Daniel Poetzl
  CV typename sharing_mapt<keyT, valueT, hashT, predT>::ST \
  sharing_mapt<keyT, valueT, hashT, predT>

+/// A map implemented as a tree where subtrees can be shared between different
+/// maps.


Can you somewhere explain how to generate sharing (likely via copy and/or assignment operations)?

Yes, I'll add an explanation for that.

tautschnig · 2018-03-25T19:41:58Z

Is is possible to add iterator and const_iterator members, or actually provide the same interface that a std::map provides?

danpoe · 2018-04-23T15:47:41Z

A few differences between the interfaces of sharing_mapt and std::map are necessary I think. I'd like to avoid having methods return/take iterators for the reasons below.

std::map is implemented as an RB tree, consisting of nodes that also have pointers to their parents. An iterator thus just needs to keep a pointer to a node. In our tree the nodes do not have parent pointers. An iterator would thus need to keep a stack of parent pointers. Moreover, dereferencing a std::map iterator yields a std::pair, which matches the implementation of std::map which stores key-value pairs as std::pair's internally, whereas sharing_mapt does not store them as std::pair's.

Since an iterator would need to contain a stack, it would cause unnecessary overhead to return an iterator on e.g. a call to find(). The iterator would also be less useful than a std::map iterator:

The elements are not stored in any particular order in sharing_mapt, thus idioms like first retrieving an iterator with find() and then starting to iterate from that position are not meaningful.
Passing an iterator to e.g. insert() in addition to a key as a hint would not be more efficient than just using the key, as when the map is modified all nodes on the path from the root of the tree to the newly inserted element need to be copied if they have use_count > 1 (thus a traversal from the root to the insert position needs to be done in any case).
Using an iterator (i.e. not a const_iterator) to iterate over sharing_mapt would break all the sharing as it would copy all the shared nodes on the assumption that they may be modified which is likely undesirable.

hannes-steffenhagen-diffblue

Looks good to me.

danpoe · 2018-05-17T15:15:33Z

@tautschnig Would you mind if we merge this PR? (it's documentation, plus unit test refactoring, and some smaller fixes). We can move potential further discussions to #2126 or another upcoming PR.

tautschnig · 2018-05-17T15:18:07Z

Looking.

danpoe requested a review from smowton March 1, 2018 17:24

danpoe force-pushed the sharing-map-catch-unit-tests branch from 9547dde to ed0918d Compare March 1, 2018 18:07

danpoe force-pushed the sharing-map-catch-unit-tests branch from ed0918d to f721a5f Compare March 2, 2018 10:53

danpoe requested review from kroening, peterschrammel and tautschnig as code owners March 2, 2018 10:53

danpoe force-pushed the sharing-map-catch-unit-tests branch from f721a5f to 88ac580 Compare March 5, 2018 16:30

danpoe changed the title ~~Move sharing map unit tests to catch~~ Document the sharing map, and move sharing map unit tests to catch Mar 5, 2018

danpoe force-pushed the sharing-map-catch-unit-tests branch from 88ac580 to f2afe15 Compare March 5, 2018 18:25

tautschnig reviewed Mar 25, 2018

View reviewed changes

tautschnig assigned danpoe Apr 5, 2018

danpoe force-pushed the sharing-map-catch-unit-tests branch 2 times, most recently from 5f35d26 to ca9906e Compare April 23, 2018 17:03

danpoe added 6 commits April 25, 2018 11:21

Move sharing map unit tests to catch

bcc489c

Fix sharing map compiler warnings

e54f740

Sharing map documentation

3e22217

Use std::size_t instead of unsigned in the sharing map

47463a3

Move sharing map and sharing node unit tests to util

1f661f5

Workaround for Visual Studio loss of CV qualifier bug

e7b3ade

danpoe force-pushed the sharing-map-catch-unit-tests branch from 7728f95 to e7b3ade Compare April 25, 2018 11:21

danpoe mentioned this pull request Apr 26, 2018

Sharing map refactoring to reduce memory consumption #2126

Merged

hannes-steffenhagen-diffblue approved these changes May 17, 2018

View reviewed changes

tautschnig approved these changes May 17, 2018

View reviewed changes

tautschnig merged commit 9fe432b into diffblue:develop May 17, 2018

martin-cs mentioned this pull request May 17, 2018

Cleanup 'undefined-function' options to use 'generate-function-body' #2070

Open

danpoe deleted the sharing-map-catch-unit-tests branch June 2, 2020 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document the sharing map, and move sharing map unit tests to catch #1899

Document the sharing map, and move sharing map unit tests to catch #1899

danpoe commented Mar 1, 2018

tautschnig commented Mar 2, 2018

danpoe commented Mar 2, 2018

hannes-steffenhagen-diffblue commented Mar 6, 2018

tautschnig Mar 25, 2018

tautschnig Mar 25, 2018

tautschnig Mar 25, 2018

tautschnig Mar 25, 2018

danpoe Apr 23, 2018

tautschnig Mar 25, 2018

danpoe Apr 23, 2018

tautschnig commented Mar 25, 2018

danpoe commented Apr 23, 2018

hannes-steffenhagen-diffblue left a comment

danpoe commented May 17, 2018

tautschnig commented May 17, 2018

Document the sharing map, and move sharing map unit tests to catch #1899

Document the sharing map, and move sharing map unit tests to catch #1899

Conversation

danpoe commented Mar 1, 2018

tautschnig commented Mar 2, 2018

danpoe commented Mar 2, 2018

hannes-steffenhagen-diffblue commented Mar 6, 2018

tautschnig Mar 25, 2018

Choose a reason for hiding this comment

tautschnig Mar 25, 2018

Choose a reason for hiding this comment

tautschnig Mar 25, 2018

Choose a reason for hiding this comment

tautschnig Mar 25, 2018

Choose a reason for hiding this comment

danpoe Apr 23, 2018

Choose a reason for hiding this comment

tautschnig Mar 25, 2018

Choose a reason for hiding this comment

danpoe Apr 23, 2018

Choose a reason for hiding this comment

tautschnig commented Mar 25, 2018

danpoe commented Apr 23, 2018

hannes-steffenhagen-diffblue left a comment

Choose a reason for hiding this comment

danpoe commented May 17, 2018

tautschnig commented May 17, 2018